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-3690: Switch from ModelContainer to ModelLibrary for image3 pipeline #8683

Merged
merged 64 commits into from
Sep 5, 2024

Conversation

emolter
Copy link
Collaborator

@emolter emolter commented Jul 30, 2024

Resolves JP-3690
Resolves JP-3619
Resolves JP-3620
Resolves JP-3621, see JP-3707 for additional work on resample memory usage beyond the scope of this ticket.
Resolves JP-3498
Work is under the epic JP-3602.

Fixes #8649
Fixes #8478
Fixes #8479
Fixes #8480
Fixes #8164

This PR is part of a larger effort to improve memory usage throughout the pipeline. Here, the AbstractModelLibrary class in stpipe is subclassed for JWST, and the pipeline steps that form the calwebb_image3 pipeline are updated to use ModelLibrary instead of ModelContainer. This facilitates ensuring that the pipeline step runs the same way whether models are loaded into memory or saved to disk.

When ModelLibrary is used with on_disk=True, memory usage is lower, both for individual pipeline steps and for calwebb_spec3 as a whole. An analysis of memory usage for each step can be found in the linked tickets JP-3619, JP-3620, and JP-3621. An analysis of the memory usage as a whole, when run on a large dataset, can be found in JP-3690.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

  • 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
  • All comments are resolved
  • Make sure the JIRA ticket is resolved properly

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 89.09396% with 65 lines in your changes missing coverage. Please review.

Project coverage is 60.77%. Comparing base (04217a2) to head (364cd60).

Files with missing lines Patch % Lines
jwst/tweakreg/tweakreg_step.py 69.23% 20 Missing ⚠️
jwst/pipeline/calwebb_image3.py 71.05% 11 Missing ⚠️
jwst/assign_mtwcs/moving_target_wcs.py 82.35% 6 Missing ⚠️
jwst/outlier_detection/spec.py 64.28% 5 Missing ⚠️
jwst/resample/resample.py 96.40% 5 Missing ⚠️
jwst/assign_mtwcs/assign_mtwcs_step.py 55.55% 4 Missing ⚠️
jwst/datamodels/library.py 94.36% 4 Missing ⚠️
jwst/outlier_detection/utils.py 96.10% 3 Missing ⚠️
jwst/pipeline/calwebb_coron3.py 33.33% 2 Missing ⚠️
jwst/pipeline/calwebb_spec3.py 0.00% 2 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8683      +/-   ##
==========================================
+ Coverage   60.63%   60.77%   +0.13%     
==========================================
  Files         372      373       +1     
  Lines       38375    38596     +221     
==========================================
+ Hits        23270    23458     +188     
- Misses      15105    15138      +33     

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

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.

If we can get a clean regtest run that would be great but I think now we're hindered more by the unrelated miri failures.

Thanks for addressing my comments. I approve!

@emolter
Copy link
Collaborator Author

emolter commented Aug 23, 2024

Another round of regtests after all the nircam image and miri image failures are fixed is here. Fingers crossed that the only failures are the same ones that show up on the nightly runs

I believe that all the regression tests are unrelated

Copy link
Member

@mcara mcara 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 did not look into rst or tests in much detail. But the logic looks fine.

@mcara
Copy link
Member

mcara commented Aug 25, 2024

Why there is still usage for the ModelContainer? Is ModelLibrary not completely replacing ModelContainer? Is it only for backward compatibility to support ModelContainer inputs?

@emolter
Copy link
Collaborator Author

emolter commented Aug 26, 2024

Why there is still usage for the ModelContainer? Is ModelLibrary not completely replacing ModelContainer? Is it only for backward compatibility to support ModelContainer inputs?

We decided to only replace container with library in the calwebb_image3 pipeline for now, where the memory performance gains are most important. There are other complications with replacing this for the spectroscopic modes too, e.g., the SourceModelContainer class that uses ModelContainer as a parent class needs to be replaced also

Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

This is a ton of work -- thanks for tackling it! Just a few questions, especially for spectral integration, and some minor suggestions.

I skimmed through previous review comments, but didn't read them all carefully, so my apologies if anything has already been addressed.

CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
docs/jwst/tweakreg/README.rst Outdated Show resolved Hide resolved
jwst/datamodels/library.py Outdated Show resolved Hide resolved
jwst/lib/exposure_types.py Outdated Show resolved Hide resolved
jwst/resample/resample.py Show resolved Hide resolved
jwst/resample/resample_spec_step.py Outdated Show resolved Hide resolved
jwst/resample/resample_spec_step.py Outdated Show resolved Hide resolved
jwst/resample/resample.py Show resolved Hide resolved
jwst/resample/resample_step.py Outdated Show resolved Hide resolved
@emolter
Copy link
Collaborator Author

emolter commented Aug 27, 2024

Another round of regression tests have been started here after fixes per Melanie's review.

Edit: lots of failing tests, but they match last night's nightly run

@emolter emolter requested a review from melanieclarke August 28, 2024 13:01
Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

All my comments are addressed, and regtest failures match the nightly run, so LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment