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

pre_hooks are broken #203

Open
jdavies-st opened this issue Oct 25, 2024 · 1 comment
Open

pre_hooks are broken #203

jdavies-st opened this issue Oct 25, 2024 · 1 comment

Comments

@jdavies-st
Copy link
Contributor

Step pre_hooks are broken currently, and we don't currently unit test them. post_hooks work fine. They were unit tested.

The recent removal of jwst as a test dependency masks the actual failure. So if we go back a few commits:

$ git reset 4667ada --hard

And then rewrite one of the unit tests:

$ git diff
diff --git a/tests/test_hooks.py b/tests/test_hooks.py
index 4d7d40a..6ff37d1 100644
--- a/tests/test_hooks.py
+++ b/tests/test_hooks.py
@@ -66,7 +66,7 @@ def test_hook_as_step_class(caplog):
 
     steps = {
         "cancelnoise": {
-            "post_hooks": [
+            "pre_hooks": [
                 HookStep,
             ]
         }

We get the following failure when running the unit tests:

$ pytest tests/test_hooks.py
...
FAILED tests/test_hooks.py::test_hook_as_step_class - TypeError: CancelNoiseStep.process() takes 2 positional arguments but 6 were given

This happens with real data and real steps as well if you try to use pre_hooks. Everything works fine with post_hooks. Not sure why the asymmetry.

h/t @TheSkyentist

@braingram
Copy link
Collaborator

braingram commented Oct 25, 2024

Thanks for opening the issue.

Do you know if these ever worked? Looking at the code it seems like the expectation is that a pre-hook will return a tuple that replaces args:

stpipe/src/stpipe/step.py

Lines 463 to 468 in 799be65

hook_args = args
for pre_hook in self._pre_hooks:
hook_results = pre_hook.run(*hook_args)
if hook_results is not None:
hook_args = hook_results
args = hook_args

Which would cause issues using a Step as a pre-hook. If I try to change some of the tests in test_hooks (which no longer require jwst to run following #199) this leads to expected failures when Step instances used as pre-hooks don't return multiple values.

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

No branches or pull requests

2 participants