-
Notifications
You must be signed in to change notification settings - Fork 2
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
Convert models to macro templates #94
Conversation
- stripped out old code - added test coverage where needed
Sorry if anybody ever needs this but why on earth did they do this
Closes #73 |
Closes #63 |
FYI to get this to run I had to manually change the acronym of the "cranial nerve" region in the brain atlas. I'm not sure what to do about this. The acronyms are case sensitive apparently (ugh) which breaks some of the templating code. I need to go back and do something custom to handle this but not sure what/how to implement it. |
Seems good to me. I want to import this branch into a few repos and run the unit tests in those to see if everything passes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pid_names module seems to have been removed in this PR. It raises a few errors in aind-data-schema
ModuleNotFoundError: No module named 'aind_data_schema_models.pid_names'
The sub-lists in the Organizations are missing. Stuff like DETECTOR_MANUFACTURERS, SUBJECT_SOURCES, etc.: https://github.com/AllenNeuralDynamics/aind-data-schema-models/blob/main/src/aind_data_schema_models/organizations.py |
Whoops thanks for catching that, I'll add a test too so we don't lose them again... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be good to go. There was one small issue where a warning message was displaying
Union[_Callithrix_Jacchus, _Homo_Sapiens,...]
instead of
Union[CALLITHRIX_JACCHUS, HOMO_SAPIENS,...]
but I think that should be okay.
I think I changed the class names by accident, but the dynamically generated classes probably shouldn't have been named in all caps anyways... |
This PR overhauls the aind-data-schema-models repository to use
jinja2
templates.The entry point is the
run_all.sh
script, which pulls the templates from the_generators/templates
folder. The name of each template file is used to get the corresponding data from themodels
folder. The templates then parse each row, generating the corresponding classes, and merge them into the ALL/ONE_OF patterns that we use. It's not super easy to write the templates so I almost certainly made a typo somewhere, especially in the docstrings appreciate a close eye on them.Some additional utility functions were added to support this and tests to coverage the new code.
Overall I'm happy with this approach, it's super simple and the templates are pretty understandable except for some weird edge cases with how empty columns are treated by pandas. This should be backward compatible with our existing repos, but we need to check carefully before merging this.
Also the
mouse_anatomy
file is massive, although it's only really a few MB so not a big deal to keep it. I vote that we merge this in some form and then revisit how to handle large vocabularies with this issue #80 in the future.