-
Notifications
You must be signed in to change notification settings - Fork 267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add method to write default config to Tool and Component. #2378
base: main
Are you sure you want to change the base?
Conversation
In the new commits, I commented every parameter that is "traitlets.Undefined" by default. So this gives the user an idea of the possibilities, without having to changes too many parameters for no added value. |
can you add a space before each class? Or better yet, each class has help info as well (a docstring), so you could add that as a comment? |
What do you mean by "space"? In the recent commits, I added a newline before every class to make sure it's not close to the previous parameter. As for the class comment, this is certainly possible but a bit more difficult to do. I'll add that to the todo-list. |
Another thing missing is that the options for on-the-fly constructed classes do not appear, e.g. there is an option image_cleaner_type = ComponentName(
ImageCleaner, default_value="TailcutsImageCleaner"
).tag(config=True) You should then show the options for ImageCleaner and all of it's non-abstract subclasses (e.g. |
I've updated the example file at the top. Now:
|
The traitlets application already has a method |
This is a good question. Because:
# Configuration file for frac-eff-plots.
c = get_config() #noqa
#------------------------------------------------------------------------------
# Application(SingletonConfigurable) configuration
#------------------------------------------------------------------------------
## This is an application.
## The date format used by logging formatters for %(asctime)s
# Default: '%Y-%m-%d %H:%M:%S'
# c.Application.log_datefmt = '%Y-%m-%d %H:%M:%S'
## The Logging format template
# Default: '[%(name)s]%(highlevel)s %(message)s'
# c.Application.log_format = '[%(name)s]%(highlevel)s %(message)s'
For comparison, here is a short example of what this branch create (as opposed to the default traitlet output)/
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Discussed today at the ctapipe meeting, the workaround with dynamic default is to detect it's dynamic, then write "it's dynamic, check documentation". I need to dig into this again because I've mostly forgotten where I was with this. |
For now, I have an issue that I don't know how to solve. I comment traits when the default values is invalid. Problem is, for EventSource for instance, all traits are invalid, and I end up with an empty Section that fail on read with this error:
I tried to parse the string after creating it, but not only is this ugly design, this is also trickier than expected, because you need to check per indentation level, and that means the function need to be recursive to be doable. But this is also tricky to do when reading the traits, because you need to check ALL traits for that sections before even writing the section, to know if you need to comment the section or not. I'm open to suggestions on this one. PS: I already solved the dynamic default apparently, months ago, I didn't even recall doing it. |
I think something went wrong in your last push... |
While I understand the general issue, I don't understand why it happens for |
@maxnoe What went wrong? What is the issue that you noticed? |
In EventSource:
But in case, another example is QualityQuery. The only parameter, quality_criteria, has no default, and I end up with an empty Section that crash my code. |
Issues are: when default value is undefined, the default config is then non valid and make it difficult to understand what value we're supposed to use.
…mented but not crashing everything in case they're optional.
In practice, it's the first line of its docstring. Formatting is done in a separate function because I also need that for trait help, as it's sometime a multiline string that is very badly rendered otherwise.
…undefined. Note that get_default_value() was deprecated anyway, and they advise to use .default_value instead. But this is the static value, meaning you'll get more undefined that what really is.
…st of values is encountered (e.g. quality criteria)
isinstance(obj, list) was not working because traitlets.List and derivatives are not considered list type. config_files trait is also commented because it crashed with the empty list as default.
5146849
to
8642e35
Compare
The yaml representation of
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2378 +/- ##
==========================================
- Coverage 92.46% 92.07% -0.39%
==========================================
Files 234 235 +1
Lines 19868 19980 +112
==========================================
+ Hits 18370 18397 +27
- Misses 1498 1583 +85 ☔ View full report in Codecov by Sentry. |
Checking the ruamel.yaml docs again, this is how you can programatically create a commented yaml document: from ruamel.yaml import YAML, CommentedMap
import sys
yaml = YAML()
doc = CommentedMap()
doc["EventSource"] = CommentedMap()
doc.yaml_set_comment_before_after_key("EventSource", before="Config of EventSources")
doc["EventSource"]["max_events"] = None
doc["EventSource"].yaml_add_eol_comment("Maximum number of events", key="max_events")
doc["EventSource"]["allowed_tels"] = None
doc["EventSource"].yaml_add_eol_comment("Allowed telescopes", key="allowed_tels") Output:
|
Thanks. It took me time to understand that what I needed was CommentedMap, because I couldn't find that information from the documentation alone. But I've used yaml_set_comment_before_after_key even for the traits, as sometimes the comments are big, and assumed it would be problematic at the end of the line. I'm still in the debug phase, but even after that, comments are not indented as the key they are attached to, so that will be my next item on the list, once I can get it to run at all. |
After discussions wit Karl and Tomas, I worked on a way to extract default configuration .yaml file, recursively, directly from the class, without having to instanciate anything.
This is not yet ready for merging, I create the pull request more for discussing what are your thoughts and requests to make this usefull.
Current issues are:
Knowing that:
Here is how you can create a yaml config file from a given class:
ProcessorTool.yml
Example config file for cta-process is attached. The filename is the default name attributed if none is given to .write_default_config()
TODO:
Done: