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

Warn during Step.run when the step hasn't been configured from CRDS #198

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Oct 11, 2024

This PR adds a warning that will appear in the console and step log when a step is called without configuring the step from CRDS. For example:

NoCRDSParametersWarning: ResampleStep.run was called without first getting step parameters from CRDS. To create a Step instance using CRDS parameters use Step.from_config_section(Step.build_config(input)[0]) or use Step.call which will create and use the instance.

This warning is controlled by a new Step attribute _warn_on_missing_crds_steppars which will allow each calibration pipeline to control this behavior either for all steps or for only steps that have parameter files. This currently defaults to False (but is additionally set to False in the test Step classes used in the unit tests here because they inherit from jwst steps, by re-setting the default in the test classes this will allow jwst to set this flag for the base JwstStep without breaking the tests here).

For Step.run uses where the warning is problematic the user has 2 options, set _warn_on_missing_crds_steppars=False on the instance or filter the warning (using either pytest or the warnings standard library).

jwst regtests (with context 1996): https://github.com/spacetelescope/RegressionTests/actions/runs/11298896541
show only the 5 expected tests due to different results from jenkins and no NoCRDSParameterWarning
romancal regtests: https://github.com/spacetelescope/RegressionTests/actions/runs/11297976103
only unrelated failures and no NoCRDSParameterWarning

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

@braingram braingram changed the title Warn when run is called for a Step that hasn't been configured from CRDS Warn during Step.run when the step hasn't been configured from CRDS Oct 11, 2024
Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.86%. Comparing base (c55d0d2) to head (891234a).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #198       +/-   ##
===========================================
+ Coverage   75.26%   94.86%   +19.59%     
===========================================
  Files          34       37        +3     
  Lines        2689     3153      +464     
===========================================
+ Hits         2024     2991      +967     
+ Misses        665      162      -503     

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

@braingram braingram force-pushed the warn_run branch 2 times, most recently from 61f62ac to dbb177c Compare October 14, 2024 12:55
@braingram braingram marked this pull request as ready for review October 14, 2024 13:38
@braingram braingram requested a review from a team as a code owner October 14, 2024 13:38
@braingram
Copy link
Collaborator Author

@melanieclarke what do you think about this as an alternative to #192 ?

@melanieclarke
Copy link
Collaborator

I think this approach solves only one of several problems with the current design of run: it lets the user know that they need to do something special to get the recommended parameter values, but it does not actually get them for them by default. What I was trying to do in #192 was make sure that the easiest thing to do is the most frequently correct thing to do. Since it’s a special case to not want to get default parameters from CRDS, I think that needs to be the opt-in behavior, not the other way around.

In addition, this approach preserves some of the most awkward features of call and run usage: that user-provided parameters can only be set consistently and with full validation if they are provided before the step is instantiated. With #192, it is possible to directly provide an override at run time, as well as fully merging in all other sources of possible parameter defaults, at the point when all inputs are available. If we do not provide a way to update parameters after instantiation that respects priority order for overrides, then we can’t deprecate call, and that leaves us still in the position of recommending a hard-to-understand class method in place of the instance method.

The only advantage I see to this PR is that it is a simpler change. I think if #192 or similar approaches are not acceptable, then this is better than nothing.

@braingram
Copy link
Collaborator Author

Thanks!

I think this approach solves only one of several problems with the current design of run: it lets the user know that they need to do something special to get the recommended parameter values, but it does not actually get them for them by default. What I was trying to do in #192 was make sure that the easiest thing to do is the most frequently correct thing to do. Since it’s a special case to not want to get default parameters from CRDS, I think that needs to be the opt-in behavior, not the other way around.

I agree that when executing a JwstStep fetching CRDS parameters makes sense as the default. That's currently what call does (leaving aside all the other issues with call).

In addition, this approach preserves some of the most awkward features of call and run usage: that user-provided parameters can only be set consistently and with full validation if they are provided before the step is instantiated. With #192, it is possible to directly provide an override at run time, as well as fully merging in all other sources of possible parameter defaults, at the point when all inputs are available. If we do not provide a way to update parameters after instantiation that respects priority order for overrides, then we can’t deprecate call, and that leaves us still in the position of recommending a hard-to-understand class method in place of the instance method.

Thanks. Yes this PR does not try to introduce any breaking changes to call or run.

The only advantage I see to this PR is that it is a simpler change. I think if #192 or similar approaches are not acceptable, then this is better than nothing.

I'll follow up on #192. The approach of this PR is to reinforce the documented API until we have a more complete plan for what we want in a new API.

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