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

JP-2609: Fix ignored step-pars during cmdline pipeline run #57

Merged

Conversation

tapastro
Copy link
Collaborator

@tapastro tapastro commented Jun 6, 2022

Currently, running a pipeline while providing a pars-{step} file overwrites the user-supplied pars with the default pars. It appears as though a simple swap in a merge may be all that's required to get this functioning - the overwrite priority is now flipped for parameters which are specified in both the default spec and the user pars file.

Addresses JP-2609

@tapastro tapastro added the bug Something isn't working label Jun 6, 2022
@tapastro tapastro self-assigned this Jun 6, 2022
@codecov
Copy link

codecov bot commented Jun 6, 2022

Codecov Report

Merging #57 (62f3f67) into master (584d547) will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master     #57   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          25      25           
  Lines        3027    3060   +33     
======================================
- Misses       3027    3060   +33     
Impacted Files Coverage Δ
src/stpipe/cmdline.py 0.00% <0.00%> (ø)
src/stpipe/pipeline.py 0.00% <0.00%> (ø)
src/stpipe/step.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 584d547...62f3f67. Read the comment docs.

@tapastro
Copy link
Collaborator Author

tapastro commented Jun 6, 2022

Note: the simple trick failed.

WIP

@tapastro
Copy link
Collaborator Author

tapastro commented Jun 7, 2022

@stscieisenhamer I committed the first implementation of the refactored config building; the version with the original pipeline.merge_config() ordering (commit "this version has no step-pars values") still gets the step-pars overwritten by the combined spec-defaults+explicit cmdline par setting, while the most recent commit with the merge_config order swapped clobbers all spec+cmdline settings with the values contained in the step-pars file.

I believe the issue is still a depth problem - the flow in cmdline.py being modified is only been accessed while step_class and config are for the pipeline - as such, the step-pars haven't been loaded into a config yet. This only happens for a step inside the pipeline during the pipeline init call, during which each step calls from_config_section and either

a) overwrites both the spec and the steps.tweakreg.kernel_fwhm with the step-pars values

or

b) overwrites the step-pars with spec + cmdline definitions.

Because of the depth in loading step-pars configs while building a pipeline config, either the cmdline args need to make it all the way into pipline init or we need to make a pipeline version of one of these methods to push the cmdline args into the config merges another way.

@tapastro tapastro requested a review from hbushouse June 8, 2022 17:56
@tapastro
Copy link
Collaborator Author

tapastro commented Jun 8, 2022

This update pushes any parameter specifications:

either through command line arguments like --steps.tweakreg.minobj=10, or through a python session Pipeline.call(input, steps={"tweakreg":{"config_file": "my_pars-tweakregstep.asdf", "kernel_fwhm": 1.23456}})

downstream to the Pipeline init, due to the problem that any step-specific pars files provided in a pipeline initialization aren't loaded as a configobj until the pipeline is instantiated and begins working through its step_defs to instantiate the steps inside.

The current method has broken every test it could possibly break in jwst/stpipe, and is still a WIP.

@tapastro tapastro marked this pull request as draft June 8, 2022 18:28
@tapastro tapastro marked this pull request as ready for review June 30, 2022 15:44
Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me. As long as it works properly.

@hbushouse
Copy link
Collaborator

At some point we should probably also include an entry for this fix in the jwst change log, just to have a record of the bug in jwst pipeline usage being fixed.

Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and confirmed it works with romancal

@zacharyburnett zacharyburnett merged commit 5f86aab into spacetelescope:master Jul 5, 2022
@zacharyburnett zacharyburnett added this to the 0.4.0 milestone Jul 5, 2022
@mcara
Copy link
Member

mcara commented Dec 3, 2022

This PR broke support for list arguments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants