JP-3631: remove direct setting of self.skip within calibration steps

[emolter]

Resolves JP-3631

Closes #8498

This PR removes all instances where a step sets its own .skip attribute, including the one in the record_step_status() helper function. That pattern causes potential issues; see, for example, this PR and therefore should be avoided. In a few instances where the calibration step was checking a step's .skip attribute after running the step, the step result's metadata is now being checked instead.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

• added entry in CHANGES.rst within the relevant release section
• updated or added relevant tests
• updated relevant documentation
• added relevant milestone
• added relevant label(s)
• ran regression tests, post a link to the Jenkins job below.
  How to run regression tests on a PR
• All comments are resolved
• Make sure the JIRA ticket is resolved properly

[emolter]

Regression tests started here

[codecov]

Codecov Report

Attention: Patch coverage is 66.00000% with 17 lines in your changes missing coverage. Please review.

Project coverage is 59.60%. Comparing base (064208c) to head (fd9b2f4).

Files with missing lines	Patch %	Lines
jwst/tweakreg/tweakreg_step.py	12.50%	7 Missing ⚠️
...st/master_background/master_background_mos_step.py	16.66%	5 Missing ⚠️
jwst/master_background/master_background_step.py	60.00%	2 Missing ⚠️
jwst/cube_build/cube_build_step.py	66.66%	1 Missing ⚠️
jwst/pipeline/calwebb_spec3.py	50.00%	1 Missing ⚠️
jwst/pixel_replace/pixel_replace_step.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8600      +/-   ##
==========================================
+ Coverage   59.56%   59.60%   +0.03%     
==========================================
  Files         391      391              
  Lines       39285    39286       +1     
==========================================
+ Hits        23402    23418      +16     
+ Misses      15883    15868      -15     

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

[emolter]

new regtest round started here

[emolter]

In the regtests above, there was one failure (test_masterbkg_corrpars) related to master_background_mos_step that appears to have been caused by the slit.source_name attributes in the test data containing the string "BACKGROUND" when the function nirspec_utils.is_background_msa_slit() was expecting "BKG". It looks like those tests haven't been changed in a while, but this PR seems to have changed the naming convention.

The question is, why was this test passing before, and failing only now as of this PR? I think the answer is due to unexpected/bad behavior when setting self.skip - I need to check this, but if the first call to step.run failed for any reason, it would have set the step status to SKIPPED and then the second call would have also skipped, therefore making both of their outputs identical (and identical to the inputs).

In terms of how to fix that, should I allow nirspec_utils.is_background_msa_slit() to check for "BACKGROUND" or "BKG", like I'm doing here in the most recent commit, or should we change the test dataset?

[melanieclarke]

In terms of how to fix that, should I allow nirspec_utils.is_background_msa_slit() to check for "BACKGROUND" or "BKG", like I'm doing here in the most recent commit, or should we change the test dataset?

I think it wouldn't hurt to allow either, in case there are other older datasets around.

[braingram]

Did you notice if any steps are now saving results when skipped (when they weren't previously)? I ask because stpipe checks Step.skip when saving resulting:
https://github.com/spacetelescope/stpipe/blob/915a8326432254bd5d03ee67586bc94c79d74327/src/stpipe/step.py#L536
I would expect that, for example, tweakreg would now save results if provided an input association with 1 group when configured to skip absolute alignment (to trigger the skip on line 208).

[emolter]

Putting some relevant points from a meeting Brett and I had here:

This line in stpipe is checking the step.skip parameter to decide whether files should be saved when save_results is True. With the changes in this PR, that means that if save_results==True but a step status is set to SKIPPED, output files will be created when they would not have before. We agreed that this should be handled in stpipe somehow, perhaps by making step.finalize_result() check the metadata of the result and returning a value that indicates whether the step had been skipped.

We discussed whether record_step_status and query_step_status should be put into stpipe, along with variables similar to the one suggested in this comment for the SKIPPED, COMPLETE, and NOTSET constants. In a perfect world it would be good to achieve similarity between Roman and JWST in the way their pipeline steps are set up and the way stpipe handles them, e.g. this open issue which is a problem for Roman but not Webb. What do other people think about moving these to stpipe?

If we do decide to do something in stpipe, it remains unclear whether this should hold up merging this PR. If the creation of files when save_results is True even when steps are skipped is not a serious problem, then I'd say we can go ahead and move toward merging it. If it is a serious problem, then we should coordinate this PR with an stpipe release that includes at minimum the aforementioned change to finalize_result. What do you think @nden @hbushouse?

[melanieclarke]

LGTM!

[emolter]

regression tests again

[emolter]

even more regtests. I will merge this if these don't turn up any failures

[braingram]

This review may have some out-dated comments (I was adding them at the same time this was updated).

[emolter]

This is ready to merge. Waiting because this should go into Build 11.1, and other bugfixes still need to go into 11.0