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

JP-3490: Allow run to check CRDS or user parameters for default overrides #192

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

melanieclarke
Copy link
Collaborator

@melanieclarke melanieclarke commented Oct 4, 2024

Resolves JP-3490
Resolves RCAL-nnnn

Closes #

WIP: proposal to allow run to check CRDS for defaults not already set

The idea is: keep run and call as is, but allow run to check CRDS for parameter overrides when called.

To allow users to set parameters as attributes and not override them with defaults, track initialization status for all parameters.

  • Override setattr to set initialized = True whenever a parameter is set.
  • In init, set initialized = False for all parameters set from software defaults in the spec
    Inside run, check for CRDS defaults and use them.
  • Use the input data to retrieve any param files from CRDS, and build a config in the same way call does.
  • Set the parameters to the CRDS config defaults, for any that have not already been initialized.

This would be mostly invisible to users: both call and run would work in the same way they currently do, except that recommended defaults are retrieved from CRDS when run is called.

Additionally, we could allow run to accept kwargs in the same way call does, so users could directly set parameters for steps when calling run. Since run and call can use the same method to retrieve and merge CRDS and user parameters, this is straightforward to support.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • run regression tests with this branch installed ("git+https://github.com/<fork>/stpipe@<branch>")
news fragment change types...
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.misc.rst: infrastructure or miscellaneous change

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.76%. Comparing base (5efae61) to head (580aeb6).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #192      +/-   ##
==========================================
+ Coverage   73.29%   74.76%   +1.47%     
==========================================
  Files          25       25              
  Lines        1917     1942      +25     
==========================================
+ Hits         1405     1452      +47     
+ Misses        512      490      -22     

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

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.

1 participant