[ENH]: Introduce junifer.api.generate_yaml - #498
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #498 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 1 1
=========================================
Hits 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
I have the impression that this way ( Can't we just look at what fields are defined in the class (not the superclass) and just pass those ones to the constructor? |
That's a fair argument and I see your point.
Not with how I understand the thing works. A model's fields consist of its own fields and superclass' fields (if it has one). So apart from defining what to exclude (or include), I don't see other way. I'll push some updates to make it better. |
a5c4e20 to
7e49b70
Compare
7e49b70 to
70c439b
Compare
|
@fraimondo I've updated the datagrabber dumping logic as discussed. Kindly review #499 before this. |
83f88ed to
56c1a85
Compare
|
Do we know how this function works when we have external imports ( |
It copies exactly what is passed. For julio, we copy exactly what is stored in h5. |
In order to create the yaml, we parse the metadata and instantiate the object. I'm not sure this will be possible if the definition of a datagrabber/marker is in an external package which is part of the |
Let me lay it down w.r.t. julio:
For using Does that solve your concern? |
Even if this means adding all variables int he meta dict and a huge comment stating that "since this is an external datagrabber/marker/etc not part of junifer core, some of this variables might not be needed or should definitely be removed." |
I don't follow. What happens if one adds |
Let's asume you open and HDF5 file, you load the meta and then you pass it to This use case should be considered. In the case that the object can't be instantiated, the fields should be extracted from the meta dict and a comment in the yaml should be added. |
Do the latest commits address your concern? |
| # Set datagrabber | ||
| meta_dg = meta["datagrabber"].copy() | ||
| a = meta_dg.pop("class") | ||
| try: |
There was a problem hiding this comment.
I would explicity check for the datagrabber being in the registry that relying on a ValueError.
Could be that because of versions mismatchs, some parameters are renamed and then we do have errors but because of other reasons.
There was a problem hiding this comment.
The check is updated to be precise now. Also, open to go the non-idiomatic route as well.
There was a problem hiding this comment.
We still rely on a ValueError. It should be something like
if component is registered:
Instantiate and dump
else:
- add ALL variables in the meta to the yaml
- Add the note: " is not a built-in component and thus could not be properly regenerated. Some of these entries in the YAML section might be redundant and not needed. Please check the documentation/implementation of this specific datagrabber and remove the unnecesary entries."
| meta_p = [meta_p] | ||
| for mp in meta_p: | ||
| b = mp.pop("class") | ||
| try: |
There was a problem hiding this comment.
Same here, explicit check
| c = meta_m.pop("class") | ||
| y["markers"] = [] | ||
| try: | ||
| m = PipelineComponentRegistry().get_class(step="marker", name=c) |
| "- `datadir` is ignored and not reproduced. " | ||
| "If `datadir` used was not a temporary directory, you will have to " | ||
| "manually edit this YAML.\n" | ||
| "- In case the dataset was 'dirty', there is no guarantee that the " |
There was a problem hiding this comment.
This note should only appear if the dataset was dirty (the meta said so)
| # Set datagrabber | ||
| meta_dg = meta["datagrabber"].copy() | ||
| a = meta_dg.pop("class") | ||
| try: |
There was a problem hiding this comment.
We still rely on a ValueError. It should be something like
if component is registered:
Instantiate and dump
else:
- add ALL variables in the meta to the yaml
- Add the note: " is not a built-in component and thus could not be properly regenerated. Some of these entries in the YAML section might be redundant and not needed. Please check the documentation/implementation of this specific datagrabber and remove the unnecesary entries."
|
In that case, if any part of the instantiation raises a ValueError (like would happen if a parameter changes options, or using an old junifer version), then we go to:
|
|
|
Can we validate? I'm worried about using different junifer versions than the one that generated the meta. Or we either go full strict and not allow any mismatch (which will create a problem with julio later on), or we validate the model. Otherwise, variables that do not match will be "ignored" and not "dumped", which might yield a different yaml than expected. I prefer to have a YAML with a note saying "check your datagrabber/marker/preprocessor due to possible changes in the API" than one without any message that actually works differently than expected. |
|
I still don't understand why it will fail for "most". As long as you choose the It should fail in case of:
All the rest should not fail. Otherwise we are dumping the wrong variables. |
Ignore my previous reply's "fail" part, it works as intended. |
| }, | ||
| ], | ||
| ) | ||
| def test_generate_yaml(m: dict) -> None: |
There was a problem hiding this comment.
So this tests that the actual function works. Can we test for correctness?
There was a problem hiding this comment.
The latest commit should check for basic correctness.
97a5c10 to
ca6cc8a
Compare
This PR adds
generate_yamlunderapito generate feature YAML from metadata. Its primary use-case is injulio's feature addition to registry.