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

only use roman.meta for get_crds_parameters #372

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Aug 5, 2024

Closes #367
Closes #366

This PR modifies DataModel.get_crds_parameters to only include (and only access) items under roman.meta. get_crds_parameters is called twice during the setup of commands like the following strun MyStep mydata.asdf. Once to get the pars file for the MyStep and again to prefetch reference files for mydata.asdf. Each of these calls will:

  • open the Datamodel contains in mydata.asdf (each call separately opens the file, the opened datamodel is not shared between calls)
  • call Datamodel.get_crds_parameters
    With the current implementation of get_crds_parameters, every attribute of the datamodel is accessed and then the attributes are filtered to include select non-array values (see the code for the specific filtering). For a "lazy" file this will result in loading and converting all contents of the file twice before the file is again re-opened within MyStep.process.

With this PR only the nodes under roman.meta are accessed and included for get_crds_parameters. This PR does not query crds for the required parameter selectors (parkeys) as:

  • there doesn't appear to be an API for reliably getting these parkeys
  • even with an API some coordination with stpipe would likely be called for as it has a crds dependency (unlike roman_datamodels)

Querying crds would allow fewer metadata entries to be loaded (which is a very minor improvement that might be offset by the crds API) and would protect against the unlikely case that a non-roman.meta attribute is used as a selector.

This PR also fixes the "include_arrays" filtering for DataModel.to_flat_dict.

Regression tests: https://github.com/spacetelescope/RegressionTests/actions/runs/10268370867
show unrelated failures due to background changes.

Checklist

Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.77%. Comparing base (087a60d) to head (d29955c).
Report is 52 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   97.56%   97.77%   +0.21%     
==========================================
  Files          30       36       +6     
  Lines        2788     3369     +581     
==========================================
+ Hits         2720     3294     +574     
- Misses         68       75       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@braingram braingram marked this pull request as ready for review August 6, 2024 16:46
@braingram braingram requested a review from a team as a code owner August 6, 2024 16:46
@braingram

This comment was marked as outdated.

@schlafly
Copy link
Collaborator

Sorry that it's been a while since you filed this.

With this PR only the nodes under roman.meta are accessed and included for get_crds_parameters. This PR does not query crds for the required parameter selectors (parkeys) as:

there doesn't appear to be an API for reliably getting these parkeys
even with an API some coordination with stpipe would likely be called for as it has a crds dependency (unlike roman_datamodels)

Just making sure I follow, you're saying that you could ask CRDS to say what it needs, and then send it back exactly that, rather than just sending the entire meta contents minus the arrays. I hear you and agree that we shouldn't need to do that and your PR is good as is.

@braingram braingram force-pushed the crds_parameters branch 2 times, most recently from fa04672 to 6f5e540 Compare September 26, 2024 20:27
@braingram braingram enabled auto-merge (squash) October 3, 2024 23:26
@braingram braingram merged commit 7485edc into spacetelescope:main Oct 3, 2024
15 of 16 checks passed
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