[ENH]: Introduce junifer.api.generate_yaml #498
No reviewers
Labels
No labels
CRITICAL
Stale
WIP
bug
concept
coordinate
dataset
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
maintenance
maps
marker
mask
on hold
parcellation
preprocess
question
ready
storage
template-space
triage
wontfix
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
juaml/junifer!498
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/generate-yaml-api"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR adds
generate_yamlunderapito generate feature YAML from metadata. Its primary use-case is injulio's feature addition to registry.Codecov Report
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (
a8e9f31) to head (ca6cc8a).⚠️ Report is 8 commits behind head on main.
Additional details and impacted files
100.00% <ø> (ø)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 (
dump_exclude) adds a lot of maintenance work: if something changes in the superclass (new internal variable), we need to go and update every subclass, including non-junifer ones.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.
@fraimondo I've updated the datagrabber dumping logic as discussed. Kindly review #499 before this.
@ -143,6 +143,11 @@ class JuselessUCLA(PatternDataGrabber):replacements: list[str] = ["subject", "task"] # noqa: RUF012This should only be
typesandtasks. The rest is hard-coded in the parameters.@ -179,0 +179,4 @@@classmethoddef dump_fields(cls) -> list[str]:"""Fields to include when dumping model."""return ["types", "uri", "rootdir"]this is where it becomes a bit tricky.
If I used a datadir (not temp) and the dataset is dirty, the YAML should account for that? or not?
Maybe we should include some comments in the generated YAMLs indicating stuff like this:
eg. if we have a "dirty" dataset, then add a comment that while the yaml will reproduce the results, the original dataset was "dirty" and so there is no guarantee that the same results will be obtained as there is no strict data provenance.
@ -64,0 +64,4 @@@classmethoddef dump_fields(cls) -> list[str]:"""Fields to include when dumping model."""return ["types", "tasks", "phase_encodings", "ica_fix"]This can also be
super(HCP1200) - datadir. Thus any change in the super will also be accounted for here.@ -64,0 +65,4 @@@classmethoddef dump_fields(cls) -> list[str]:"""Fields to include when dumping model."""return [I thinks this should be both "super" fields.
@ -64,0 +64,4 @@@classmethoddef dump_fields(cls) -> list[str]:"""Fields to include when dumping model."""return ["types", "tasks", "phase_encodings", "ica_fix"]In that case, the MRO for this class will get
dump_fieldsfromDataladDataGrabberwhich will be incorrect.@ -64,0 +65,4 @@@classmethoddef dump_fields(cls) -> list[str]:"""Fields to include when dumping model."""return [It is "both" of them.
@ -179,0 +179,4 @@@classmethoddef dump_fields(cls) -> list[str]:"""Fields to include when dumping model."""return ["types", "uri", "rootdir"]We can add a general comment. Making it conditional would be quite tricky.
@ -179,0 +179,4 @@@classmethoddef dump_fields(cls) -> list[str]:"""Fields to include when dumping model."""return ["types", "uri", "rootdir"]I would like that the generated YAML is commented.
@ -64,0 +65,4 @@@classmethoddef dump_fields(cls) -> list[str]:"""Fields to include when dumping model."""return [I meant instead of manually placing the fields, to all super. But I understand that might be bothersome.
Do we know how this function works when we have external imports (
withstatements in the YAML)?@ -64,0 +65,4 @@@classmethoddef dump_fields(cls) -> list[str]:"""Fields to include when dumping model."""return [The MRO would stop at the first
dump_fieldswhich would give a partial list.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
withstatement.Let me lay it down w.r.t. julio:
junifer.api.parse_yamlwhich loads the external modules and registers componenets as needed.withis present in the junifer YAML, julio adds it directly to the generated YAML.junifer.api.generate_yamlcan load them from component registry and dump as needed.For using
junifer.api.generate_yamlwithout julio, one would need to already register the components beforehand. Now, as it's part ofjunifer.api, we can presume that the user will do it like so.Does that solve your concern?
junifer.api.generate_yamlhas one parameter that is ametadict. This meta dict can be extracted from and HDF5 file, without parsing any yaml. We need to be able to support generating a YAML from any meta dict, no only after parsing the respective yaml.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
"with"key to the "meta" dictionary passed without loading the external components?Let's asume you open and HDF5 file, you load the meta and then you pass it to
generate_yaml. At no point there was aparse_yamlcall, so all external modules will not be loaded. This code will fail as it needs to instantiate the elements in the meta to generate the yaml.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?
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.
Same here, explicit check
Same here
This note should only appear if the dataset was dirty (the meta said so)
The check is updated to be precise now. Also, open to go the non-idiomatic route as well.
Updated now.
We still rely on a
ValueError. It should be something likeif component is registered:
Instantiate and dump
else:
ValueErroris only raised if the component is not registered.try...exceptis the "idiomatic" way to do it. I understand if that doesn't work and it needs to be superfluous.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:
model_constructdoes not raise an exception so there will be no error during the model construction.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.
model_constructis now replaced withmodel_validatewhich will fail for most due to the nature of the metadata and the models. Necessary comments will be added on generation.I still don't understand why it will fail for "most". As long as you choose the
dump_fieldsand pass it to the constructor, this should recreate the same object.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.
So this tests that the actual function works. Can we test for correctness?
The latest commit should check for basic correctness.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.