search

spacetelescope / stpipe

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

Remove unused code to simplify filename generation #154

Closed braingram closed 2 months ago

braingram commented 3 months ago

This PR removes some unused code in stpipe. Most of this is related to filename generation which will hopefully be somewhat easier to understand after this PR:

jwst regtests: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1519/ shows 1 unrelated error (test_duplicate_names which randomly fails) romancal regtests: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/825/ passed with no errors

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 69.83%. Comparing base (46137f0) to head (e074807). Report is 1 commits behind head on main.

Files Patch % Lines
src/stpipe/step.py 0.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #154 +/- ## ========================================== + Coverage 69.46% 69.83% +0.37% ========================================== Files 24 24 Lines 1634 1618 -16 ========================================== - Hits 1135 1130 -5 + Misses 499 488 -11 ```

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

braingram commented 3 months ago

Closing and re-opening to re-trigger CI (it failed due to github actions being "degraded").

hbushouse commented 3 months ago

I can see the benefit in cleaning up a library for unused/unnecessary code, but another part of me thinks it must've had a reason to be there in the first place. Perhaps some of this could still be useful at some point in the future, but just isn't being used right now? Would be nice if we still had access to the thoughts of the original designer and author.

braingram commented 3 months ago

Thanks for taking a look.

tldr; this aims to make output file name generation easier to understand

I'll try to fill in details and ping authors (if they're still available).

Pipeline.set_input_filename

This method was introduced before stpipe existed for the "inital commit" of jwst 8 years ago by @jhunkeler : https://github.com/spacetelescope/jwst/blame/c353186397091d1821ca8723da49b182346d5626/jwst/stpipe/pipeline.py#L251 It is related to Step._input_filename which is set by Step.set_primary_input Under some circumstances _input_filename can impact the default_output_file which (again under some circumstances) can be used to construct the output path in _make_output_path. This is a small portion of the convoluted set of conditions and variables that impact the output filename. Part of me hopes that someone has a better understand of all of this than I do and can help me understand all the conditions that determine how files. I believe removing some of this complication can help to make the file naming easier to understand (and hopefully less buggy). Since Pipeline.set_input_filename is unused in stpipe, jwst and romancal removing it is a small step towards this goal.

Step.name_format

This was introduced by @tapastro in https://github.com/spacetelescope/stpipe/pull/29 to allow for https://github.com/spacetelescope/jwst/pull/6376 It has since been unused by jwst or romancal. https://github.com/spacetelescope/jwst/pull/8539 used make_output_path to accomplish the same goal and I believe can replace any future need for name_format.

Step.resolve_file_name

This was also added in jwst as part of the "initial commit" 8 years ago by @jhunkeler and now appears unused. It includes a single line of code which if needed could be re-introduced (and perhaps updated to use newer syntax): https://github.com/spacetelescope/stpipe/blob/54ec1d100dd2cd94eddf6848da85e32d3de80aae/src/stpipe/step.py#L639

the format argument to Step.save_model the name_format component_format and separator arguments to Step._make_output_path

These trace back to https://github.com/spacetelescope/jwst/pull/1316 made 6 years ago by @stscieisenhamer The goal in removing these is similar to Pipeline.set_input_filename. As these are unknown options that complicate file naming removing them will hopefully make this process easier to understand.

remove (https://github.com/spacetelescope/jwst/pull/8542) Step.reference_uri_to_cache_path

This change isn't related to file naming. Step.reference_uri_to_cache_path is a class method (that doesn't use the class) and contains only a call to: https://github.com/spacetelescope/stpipe/blob/54ec1d100dd2cd94eddf6848da85e32d3de80aae/src/stpipe/step.py#L921 Any further need for this feature can use the crds_client so I don't think the Step benefits from having this as a class method.

hbushouse commented 2 months ago

@braingram Do any of the changes here result in API changes, such that we need to keep an updated version of stpipe in sync with other packages (e.g. jwst)?

zacharyburnett commented 2 months ago

I can't find any usages of ..., name_format= in the JWST code, so I don't think it requires bumping the pin there, but this does change the API so I think it does warrant a new version here

braingram commented 2 months ago

Everything removed here is unused in jwst and roman.

This package is 0.5.2 currently so 0.6 seems reasonable to me for the next version (and updating the pins in jwst and roman once this is released to use the new version so >=0.6).

I mostly assumed that since this package is not 1.0 that we don't claim any form of backwards compatibility and that as long as we don't break jwst and roman we're good to go. Let me know if this is not the case.

hbushouse commented 2 months ago

Everything removed here is unused in jwst and roman.

This package is 0.5.2 currently so 0.6 seems reasonable to me for the next version (and updating the pins in jwst and roman once this is released to use the new version so >=0.6).

I mostly assumed that since this package is not 1.0 that we don't claim any form of backwards compatibility and that as long as we don't break jwst and roman we're good to go. Let me know if this is not the case.

Sounds good. Although given the fact that this has been in major public use (by the jwst pipeline) for several years now, I'm wondering why we've never come out with a version 1.0 yet.

braingram commented 2 months ago

I think a 1.0 is a great idea but there are some more things I'd like to take a stab at cleaning up before we go there. I'll open an issue for 1.0.0 planning but one major gap is the lack of documentation.

hbushouse commented 2 months ago

I think a 1.0 is a great idea but there are some more things I'd like to take a stab at cleaning up before we go there. I'll open an issue for 1.0.0 planning but one major gap is the lack of documentation.

Good point. So for the impending release we'll stick with 0.6.0, but start making plans for a future 1.0.0.

braingram commented 2 months ago

Good point. So for the impending release we'll stick with 0.6.0, but start making plans for a future 1.0.0.

I think that makes the most sense so we don't rush 1.0.0. So the next release will be 0.6.0 and we can lay out a plan for 1.0.0 in: https://github.com/spacetelescope/stpipe/issues/161