Fix pre- and post-hooks

[jdavies-st]

Currently pre- and post-hooks only work as strings pointing to importable Step subclasses. This made sense back when the foreseen use case of the pipeline was with strun at the command line, where all args are strings.

This PR fixes hooks to work with our current use cases. It adds the possibility of using a defined function or defined Step subclass as hook, or an instance with parameters set of a Step subclass. And it tests all the cases.

So now a hook can be a list of a number of useful things, not just a list of strings.

Hooks before:

post_hooks = ["mypackage.MyFirstStep", "mypackage.MySecondStep"]  # string pointing to Step subclass
post_hooks = ["mypackage.my_function"]  # string pointing to a function that is Step.process() equivalent
post_hooks = ["fitsinfo {0}"]  # string pointing to system call

With this PR, we can also now use the following as hooks:

from mypackage import MyFirstStep, my_function


post_hooks = [MyFirstStep]  # imported Step subclass
post_hooks = [my_function]  # imported function that is Step.process() equivalent
post_hooks = [MyFirstStep(param1=True, param2=3.5)]  # imported Step instance
post_hooks = ["mypackage.MyFirstStep(param1=True, param2=3.5)"]  # string pointing to Step subclass instance

Also added an extra CI job that runs the unit tests with some downstream dependencies. There's always been tests in this package that depend on stdatamodels and roman_datamodels, but these have been skipped since the recent-ish github actions workflow updates, and subsequently the roman_datamodels test had a broken import. I've chosen to test the hooks with jwst as an optional test dependency, as that's the lowest overhead way to do it without having to create a bunch of redundant test infrastructure.

[codecov]

Codecov Report

Attention: Patch coverage is 98.24561% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 68.76%. Comparing base (e747142) to head (c2810ea).
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/stpipe/hooks.py	97.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #133      +/-   ##
==========================================
+ Coverage   59.33%   68.76%   +9.43%     
==========================================
  Files          24       24              
  Lines        1618     1652      +34     
==========================================
+ Hits          960     1136     +176     
+ Misses        658      516     -142     

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

[jdavies-st]

@hbushouse this is the promised PR to make pre- and post-hooks work within python instead of just with the strun CLI. There's still some tests in jwst that use .cfg files for hooks. Once this is merged and released, we can update those properly.

The only use case I have not thought about is if one wanted to do something like:

$ strun calwebb_detector1 jw001234_blah_blah_uncal.fits --steps.jump.post_hooks="mypackage.MyAwesomeStep(threshold=3.5)",

i.e. passing an instance of a step via the CLI as a hook. One can pass an instance of a step as a hook within python, so perhaps this is not worth pursuing. EDIT: implemented in commit 604842f

[braingram]

JWST regtests run at: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1148/
This run showed the 3 random unrelated errors (test_against_standard, test_nirspec_missing_msa_fail, test_nirspec_missing_msa_nofail).

roman regtests ran with no errors: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/551/

[drlaw1558]

I just tried out this PR and ran into a couple of issues; unknown if this is user error or a problem with the code itself. Defining a custom step 'XplyStep' this works ok if the code for the step is in a different file in the same directory as my notebook, but defining the step within the notebook itself still fails.

2024-02-26 10:13:37,678 - stpipe.Spec2Pipeline.assign_wcs.post_hook0 - INFO - Spawning 'xply.XplyStep'
2024-02-26 10:13:37,692 - stpipe.Spec2Pipeline.assign_wcs.post_hook0 - INFO - Done with errorcode 127
2024-02-26 10:13:37,693 - stpipe.Spec2Pipeline.assign_wcs.post_hook0 - INFO - STDOUT: b''
2024-02-26 10:13:37,694 - stpipe.Spec2Pipeline.assign_wcs.post_hook0 - INFO - STDERR: b'/bin/sh: xply.XplyStep: command not found\n'
2024-02-26 10:13:37,695 - stpipe.Spec2Pipeline - ERROR - Assign_wcs processing was skipped.
2024-02-26 10:13:37,695 - stpipe.Spec2Pipeline - ERROR - Aborting remaining processing for this exposure.
2024-02-26 10:13:37,695 - stpipe.Spec2Pipeline - ERROR - No output product will be created.

Looks like something isn't linking up quite right.

[jdavies-st]

Thanks for giving it a spin @drlaw1558. Could you show the cells of your code?

If you define a step in one cell:

class XplyStep(Step):

    class_alias = "xply"

    def process(self, input_data):
        return input_data

Then the way to use it later as a hook, is directly:

steps = dict(
    assign_wcs = dict(
        post_hooks = [
            XplyStep,
        ],
    ),
)
result = Spec2Pipeline.call(input_data, steps=steps)

No need to pass it as xply.XplyStep, as that's only if its in a module xply that is importable. That would be the case if you had an xply.py module sitting in the same dir as your notebook, or if there was some package out there called xply that could be installed and imported.

[drlaw1558]

Ah, that was it. I'd removed the xply., but I also needed to remove the quotes. I.e.,

dict = {"assign_wcs": {"post_hooks": ["XplyStep"]}}
but needed
dict = {"assign_wcs": {"post_hooks": [XplyStep]}}

Looks good to me then, and the logging functionality is now working too.

[jdavies-st]

Yes, the quotes are what was originally needed if you have a module called xply.py sitting next to your notebook. And in fact that already works without this PR. This PR allows one to import a class or function directly, or define it, and just use it. Much better. And also to use an instance of that class, which allows you do specify non-default parameters of the Step class you're using as a hook.

[jdavies-st]

Any chance of this getting a review? From a user perspective, it would be really nice to have this improved hook functionality in the next release of jwst.

[nden]

Should this be deprecated instead of removed? It looks like it's been available for a long time.

[jdavies-st]

It's not removed, I've renamed the hook_from_string() function to hook_from_string_or_class() to better describe what it actually does. Happy to change the name back if you think this will affect the public API. Since this is all internal workings, I would rather make all this private, including the whole hooks.py module, but happy to maintain backwards compat.

If not maintaining backwards compat, maybe "hook_from_object()` would be a better function name.

stpipe suffers from a lot of its internal workings, such as Step.run(), being public functions.

As far as I can tell, a search of Github doesn't turn up any uses of stpipe.hooks outside of stpipe.

[jdavies-st]

I've gone ahead and changed the function name back to its original name, and just clarified in the docstring what it does. We can revisit moving public functions and methods to be private in a future refactor.

[nden]

Thanks! And I agree it's unlikely anyone uses it.

[nden]

@jdavies-st I think this looks good - other than the one comment I have.

[jdavies-st]

Once this is merged and stpipe 0.6.0 is released, I'll go ahead and update the docs in jwst to reflect the new possibilities of hook usage.

[nden]

Regression tests pass again.

[nden]

Thanks! And I agree it's unlikely anyone uses it.

[nden]

@jdavies-st Please ping me on the JWST PR.