spacetelescope / stpipe

https://stpipe.readthedocs.io
Other
3 stars 25 forks source link

WIP: JP-3563: Raise warning instead of error for unexpected values in config #153

Closed melanieclarke closed 2 months ago

melanieclarke commented 5 months ago

Resolves JP-3563

Closes https://github.com/spacetelescope/jwst/issues/8335

For future-proofing pipeline runs with new steps, unexpected values in pipeline or step configuration now raise warnings instead of ValidationErrors. Unexpected steps or parameter values are ignored. This allows an older pipeline version to be used with a newer parameter reference file.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 71.14%. Comparing base (531f752) to head (2d0e8e0). Report is 76 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #153 +/- ## ========================================== + Coverage 69.46% 71.14% +1.68% ========================================== Files 24 24 Lines 1634 1653 +19 ========================================== + Hits 1135 1176 +41 + Misses 499 477 -22 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

braingram commented 5 months ago

I'm a little surprised that test_extra_parameter isn't failing in the jwst downstream job. https://github.com/spacetelescope/jwst/blob/f8665574b9e8ad314c003f3661ca50e515719cad/jwst/stpipe/tests/test_step.py#L563-L565 A quick glance makes me think that test is checking that the step should raise an exception for an unexpected value. Is that still passing (and raising an exception) because an unknown parameter on the command line is expected to produce an error whereas a parameter in a file should now produce a warning?

melanieclarke commented 5 months ago

A quick glance makes me think that test is checking that the step should raise an exception for an unexpected value. Is that still passing (and raising an exception) because an unknown parameter on the command line is expected to produce an error whereas a parameter in a file should now produce a warning?

Yes, the current change is specifically for parsing configuration files - directly passed parameters on the command line or in step arguments will still raise a ValidationError. I think this is as requested, to support parameter reference files in CRDS that are a little ahead of the pipeline code.

melanieclarke commented 5 months ago

@nden - I don't seem to have permissions on stpipe to request specific reviewers, but I think this one is ready for review, as discussed.

braingram commented 5 months ago

The code changes look good to me. I left a comment on JP-3563 about my concerns for turning this error into a warning.

@melanieclarke would you run the jwst and romancal regtests with this PR?

melanieclarke commented 5 months ago

@braingram - I can run the jwst regtests, but I don't have access to the romancal ones.

jwst tests started here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1495/

braingram commented 5 months ago

Thanks! I started a romancal run here: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/807/

EDIT: passed with no errors

melanieclarke commented 5 months ago

There were a few failures in the jwst regtests, but they're unrelated to this PR.

melanieclarke commented 5 months ago

Setting this back to draft to explore warning for extra parameters only when pulling from CRDS.

melanieclarke commented 5 months ago

Added some draft changes that would warn for extra values in configuration files retrieved from CRDS only.

Note that this adds a validation step earlier in the process than it currently happens, so will add default values to the configuration where they did not exist before. We'd need to check that this doesn't have downstream effects (i.e. default values overwriting real ones).

Also, I have not added any optional controls for this process yet: it always strips extra values from CRDS when retrieved via get_config_from_reference. It's not clear to me at what point in the process that might need to be turned off.

In its current form, there's one downstream unit test in jwst that will need to be updated.

I'm leaving this PR at draft status: it needs more discussion.

melanieclarke commented 2 months ago

Closing: the associated JP ticket is withdrawn.