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

Proof of Concept: Dynamically Generated DataModels #221

Conversation

WilliamJamieson
Copy link
Contributor

After the roman tag-up this week (22 June 2023), I was left wondering how difficult would it be to dynamically generate the datamodels from RAD now that the datamodel_name meta data has been added to the schemas.

As it turns out, this is not that difficult if one builds on the proposed changes in PRs #213 and #214 (This PR is based off a a rebase of #214 onto #213). This PR move the datamodels from being statically defined in code to dynamic code generation as discussed.

Checklist

@WilliamJamieson WilliamJamieson changed the title Proof of Concept for Dynamically Generated DataModels Proof of Concept: Dynamically Generated DataModels Jun 23, 2023
@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Patch coverage: 95.00% and project coverage change: +0.14 🎉

Comparison is base (7b5f02f) 96.74% compared to head (af4e8d9) 96.88%.

❗ Current head af4e8d9 differs from pull request most recent head 5ca92e1. Consider uploading reports for the commit 5ca92e1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #221      +/-   ##
==========================================
+ Coverage   96.74%   96.88%   +0.14%     
==========================================
  Files          29       31       +2     
  Lines        2398     2379      -19     
==========================================
- Hits         2320     2305      -15     
+ Misses         78       74       -4     
Impacted Files Coverage Δ
src/roman_datamodels/datamodels/_core.py 89.89% <ø> (-0.11%) ⬇️
src/roman_datamodels/datamodels/_mixins.py 90.00% <90.00%> (ø)
src/roman_datamodels/datamodels/__init__.py 100.00% <100.00%> (ø)
src/roman_datamodels/datamodels/_datamodels.py 100.00% <100.00%> (+8.97%) ⬆️
src/roman_datamodels/datamodels/_factory.py 100.00% <100.00%> (ø)
src/roman_datamodels/stnode/__init__.py 100.00% <100.00%> (ø)
tests/test_models.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

A DataModel object class
"""
if hasattr(_mixins, mixin := f"{datamodel_name}Mixin"):
class_type = (DataModel, getattr(_mixins, mixin))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bug here, things like the
get_primary_array_name cannot be properly overridden as desired.

This is because the "order" of inheritance in Python is right to left meaning it applies the right most class structure first then proceeds to the left. Thus for the get_primary_array_name example the Mixin will load its version and then it will be overloaded by the DataModel object. This is the reverse of what we want.

This should not effect anything added by the Mixin (as one would normally do), but since this is the way I am dynamically injecting alterations to dynamically created objects, we need to pay attention to this ordering.

@WilliamJamieson WilliamJamieson force-pushed the feature/dynamic_datamodels branch 6 times, most recently from 88518bc to 492c1a5 Compare June 27, 2023 18:16
@WilliamJamieson WilliamJamieson force-pushed the feature/dynamic_datamodels branch 6 times, most recently from cadbc20 to 9c8ba9a Compare July 10, 2023 18:03
Copy link
Collaborator

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and pretty straightforward, aside from the needed changes to a couple docstrings.


def get_primary_array_name(self):
"""
Returns the name "primary" array for this model, which
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be changed to reflect that it is the override, rather than intended to be overridden.


def get_primary_array_name(self):
"""
Returns the name "primary" array for this model, which
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise.

@WilliamJamieson WilliamJamieson force-pushed the feature/dynamic_datamodels branch 3 times, most recently from 124d00f to af4e8d9 Compare July 19, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants