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

Remove unused code to simplify filename generation #154

Merged
merged 6 commits into from
Jun 24, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Jun 6, 2024

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:

  • Pipeline.set_input_filename
  • Step.name_format: Requires remove name_format usage in wfs_combine jwst#8539 to first remove the 1 usage in jwst.
  • Step.resolve_file_name
  • the format argument to Step.save_model
  • the name_format component_format and separator arguments to Step._make_output_path
  • remove (now unused) Step.reference_uri_to_cache_path

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

Copy link

codecov bot commented Jun 6, 2024

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 70 commits behind head on main.

Files with missing lines Patch % Lines
src/stpipe/step.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            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     

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

@braingram braingram changed the title remove Step.name_format WIP: remove unused name formatting code Jun 6, 2024
@braingram braingram changed the title WIP: remove unused name formatting code WIP: remove unused code Jun 7, 2024
@braingram
Copy link
Collaborator Author

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

@braingram braingram closed this Jun 12, 2024
@braingram braingram reopened this Jun 12, 2024
@braingram braingram marked this pull request as ready for review June 12, 2024 13:27
@braingram braingram requested a review from a team as a code owner June 12, 2024 13:27
@hbushouse
Copy link
Collaborator

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 braingram changed the title WIP: remove unused code Remove unused code Jun 14, 2024
@braingram
Copy link
Collaborator Author

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 #29 to allow for spacetelescope/jwst#6376
It has since been unused by jwst or romancal.
spacetelescope/jwst#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):

return join(dirname(self.config_file or ""), file_name)

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

These trace back to spacetelescope/jwst#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 (spacetelescope/jwst#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:

return crds_client.reference_uri_to_cache_path(reference_uri, observatory)

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.

@braingram braingram changed the title Remove unused code Remove unused code to simplify filename generation Jun 14, 2024
Copy link
Collaborator

@zacharyburnett zacharyburnett left a comment

Choose a reason for hiding this comment

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

LGTM, I dunno what name_format was used for but if it's not needed that's one less variable-

@braingram braingram merged commit 0c4b339 into spacetelescope:main Jun 24, 2024
15 of 17 checks passed
@braingram braingram deleted the name_format branch June 24, 2024 13:01
@hbushouse
Copy link
Collaborator

@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
Copy link
Collaborator

zacharyburnett commented Jun 24, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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: #161

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

Successfully merging this pull request may close these issues.

3 participants