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

Update infrastructure files to include emicorr #8137

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

hbushouse
Copy link
Collaborator

@hbushouse hbushouse commented Dec 15, 2023

This PR adds entries for the new emicorr step to a couple of infrastructure files, in order to get unit tests to pass everywhere.

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@braingram
Copy link
Collaborator

To check that I understand these changes. Prior to this PR emicorr would not run via the cli (strun emicorr ...) as the step alias was not added to get_steps which is fixed in this PR. Adding the step to get_steps also requires adding it to jwst.step.__all__ due to this test

assert {t[0].split(".")[-1] for t in tuples} == set(jwst.step.__all__ + jwst.pipeline.__all__)
which compares __all__ and the output of get_steps. Is this correct?

@hbushouse
Copy link
Collaborator Author

To check that I understand these changes. Prior to this PR emicorr would not run via the cli (strun emicorr ...) as the step alias was not added to get_steps which is fixed in this PR. Adding the step to get_steps also requires adding it to jwst.step.__all__ due to this test

assert {t[0].split(".")[-1] for t in tuples} == set(jwst.step.__all__ + jwst.pipeline.__all__)

which compares __all__ and the output of get_steps. Is this correct?

You are correct, sir. Without this change, executing "strun emicorr ..." results in "ValueError: 'emicorr' is not a path to a config file or a Python Step class". With this change, it runs just fine.

Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

LGTM!

Is it worth looking into adding a test to catch this type of omission for future step additions? Perhaps iterating through Step and Pipeline subclasses checking that all are registered?

@hbushouse
Copy link
Collaborator Author

LGTM!

Is it worth looking into adding a test to catch this type of omission for future step additions? Perhaps iterating through Step and Pipeline subclasses checking that all are registered?

Good question. We seem to have run into this issue twice in the past 1-2 weeks when brand new steps got merged. The CI tests did not reveal any failures (not sure why). It's not until we run a full regtest that suddenly we see 1 or 2 unit tests failing, because of the missing suffix or step name in tests like this. I would've thought that the unit tests would get run by at least some of the CI's.

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (02a3f2e) 75.24% compared to head (dd60d4f) 75.24%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8137   +/-   ##
=======================================
  Coverage   75.24%   75.24%           
=======================================
  Files         470      470           
  Lines       38420    38421    +1     
=======================================
+ Hits        28908    28909    +1     
  Misses       9512     9512           
Flag Coverage Δ *Carryforward flag
nightly 77.37% <ø> (ø) Carriedforward from 02a3f2e

*This pull request uses carry forward flags. Click here to find out more.

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

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 when build CI passes

@zacharyburnett
Copy link
Collaborator

I think this should be good to merge now, the ARM64 builds are just taking a while but I assume they will pass

@hbushouse hbushouse merged commit 2650df7 into spacetelescope:master Dec 15, 2023
28 checks passed
@hbushouse hbushouse deleted the add_emicorr branch December 15, 2023 15:02
braingram added a commit to braingram/jwst that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants