Skip to content
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

Merged
merged 42 commits into from
Nov 6, 2024
Merged

Convert models to macro templates #94

merged 42 commits into from
Nov 6, 2024

Conversation

dbirman
Copy link
Member

@dbirman dbirman commented Oct 25, 2024

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 the models 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.

@dbirman dbirman marked this pull request as ready for review October 30, 2024 05:03
@dbirman dbirman requested review from jtyoung84, Sun-flow and dyf and removed request for Sun-flow October 30, 2024 05:03
@dbirman
Copy link
Member Author

dbirman commented Oct 30, 2024

Closes #73

@dbirman
Copy link
Member Author

dbirman commented Oct 30, 2024

Closes #63

@dbirman
Copy link
Member Author

dbirman commented Oct 31, 2024

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.

@dbirman dbirman requested a review from jtyoung84 November 4, 2024 21:00
@jtyoung84
Copy link
Contributor

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.

Copy link
Contributor

@jtyoung84 jtyoung84 left a 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'

@jtyoung84
Copy link
Contributor

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

@dbirman
Copy link
Member Author

dbirman commented Nov 5, 2024

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...

@jtyoung84 jtyoung84 self-requested a review November 6, 2024 17:53
Copy link
Contributor

@jtyoung84 jtyoung84 left a 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.

@dbirman
Copy link
Member Author

dbirman commented Nov 6, 2024

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...

@dbirman dbirman merged commit d198752 into main Nov 6, 2024
4 checks passed
@dbirman dbirman deleted the refactor-macro-generator branch November 6, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants