JP-3721: Simplify ModelContainer #190

emolter · 2024-09-26T13:49:19Z

Helps to resolve JP-3721

This PR enables the JWST ModelContainer to no longer inherit from DataModel and to no longer require its own save method, as part of an effort to make ModelContainer's usage narrower and easier to understand. This required two changes to the logic of step.py:

Add handling for Sequence when setting skip attribute
In save_model(), remove the assumption that a Sequence-type object will always have a save method

Tasks

update or add relevant tests
update relevant docstrings and / or docs/ page
Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
- write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
- run regression tests with this branch installed ("git+https://github.com/<fork>/stpipe@<branch>")
  - jwst regression test
  - romancal regression test

news fragment change types...

changes/<PR#>.feature.rst: new feature
changes/<PR#>.bugfix.rst: fixes an issue
changes/<PR#>.doc.rst: documentation change
changes/<PR#>.removal.rst: deprecation or removal of public API
changes/<PR#>.misc.rst: infrastructure or miscellaneous change

…JP-3721

codecov · 2024-09-26T13:52:34Z

Codecov Report

Attention: Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 76.97%. Comparing base (5efae61) to head (7ed2255).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/stpipe/step.py	50.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
+ Coverage   73.29%   76.97%   +3.68%     
==========================================
  Files          25       25              
  Lines        1917     1920       +3     
==========================================
+ Hits         1405     1478      +73     
+ Misses        512      442      -70

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

emolter · 2024-10-07T19:40:39Z

starting regression tests on jwst main with this PR branch pinned as the stpipe version. This should show whether the stpipe changes are back-compatible with the previous ModelContainer

emolter · 2024-10-08T13:50:22Z

Only one failed regtest, but starting a full new run just in case. Edit: all tests passed.

emolter · 2024-10-08T15:58:09Z

I have now added several unit tests of the step save() method. Hopefully these, along with the corresponding changes in jwst spacetelescope/jwst#8831, clarify the intent behind this PR.

The passing regression tests in the link above, which use the jwst main branch with stpipe pinned to this PR branch show that these changes are compatible with that pipeline even without making any changes to ModelContainer. Therefore I think we could safely merge this without waiting for the corresponding changes in the jwst repository. I don't think the two failing unit tests in jwst test_downstream are caused by this PR, because the same ones are also present on yesterday's CI on main.

Marking this ready for review now.

Can someone please start a romancal regtest run with this branch at some point? It would be good to ensure we aren't breaking anything there, either.

braingram

Here are some minor comments (that I found I had not previously submitted).

I haven't had a chance to look at this PR in detail but I'm surprised by the number of required changes to simplify ModelContainer. Is the expectation that stpipe should be able to treat it like a list of models?

braingram · 2024-10-03T16:56:20Z

src/stpipe/step.py

-                                        )
-                            elif isinstance(args[0], AbstractDataModel):
+
+                    elif isinstance(args[0], Sequence) and self.class_alias is not None:


Suggested change

elif isinstance(args[0], Sequence) and self.class_alias is not None:

elif isinstance(args[0], Sequence) and not isinstance(args[0], str) and self.class_alias is not None:

if args[0] = "some_filename.fits" the current if can pass (issubclass(str, Sequence) == True). I think the suggestion here will fix that.
I'd say we ignore potential issues with 0-length args (as those appear to exist with the current code).

braingram · 2024-10-03T16:57:57Z

src/stpipe/step.py

+
+                    elif isinstance(args[0], Sequence) and self.class_alias is not None:
+                        # handle ModelContainer or list of models
+                        if isinstance(args[0][0], AbstractDataModel):


Suggested change

if isinstance(args[0][0], AbstractDataModel):

if args[0] and isinstance(args[0][0], AbstractDataModel):

To avoid an exception for args[0] = [].

braingram · 2024-10-08T20:04:09Z

src/stpipe/step.py

                        except AttributeError as e:
                            self.log.info(
-                                "Could not record skip into DataModel"
-                                " header: %s",
+                                "Could not record skip into DataModel" " header: %s",


Suggested change

"Could not record skip into DataModel" " header: %s",

"Could not record skip into DataModel header: %s",

emolter · 2024-10-08T20:38:54Z

I haven't had a chance to look at this PR in detail but I'm surprised by the number of required changes to simplify ModelContainer. Is the expectation that stpipe should be able to treat it like a list of models?

Yes, in the present version of the code a list of datamodels and the new ModelContainer take the same logic path. Some more notes on what changed:

The refactor removes repeated or near-repeated code between step.run() and step.save_model() that were both designed to handle lists of models. I don't know if ModelContainer was the original reason this was all implemented, but it doesn't seem necessary to me and handling everything inside save_model made more sense in my head.
It was necessary to also allow a Sequence that had its own save() attribute because SourceModelContainer has one. I can see the argument for removing this too, but the problem is that SourceModelContainer encodes a MultiExposureModel and therefore is in some ways akin to a singular datamodel. There is an open issue about rethinking SourceModelContainer and I'd love to hear your thoughts, but the more I think about it, the more I think it's mostly fine as-is.
I decided to make it impossible to save lists-of-lists. The motivation for that was the jwst badpix_selfcal step, for which allowing Sequence-type input in the way I'm doing it meant that step.save_model() was saving the science exposure and all background exposures to the same filename. In a previous version, I had modified save_model for just the badpix_selfcal_step, but that made it so the stpipe changes were not compatible with jwst/main. This decision could be changed though.

braingram · 2024-10-08T21:20:44Z

Yes, in the present version of the code a list of datamodels and the new ModelContainer take the same logic path. Some more notes on what changed:

* The refactor removes repeated or near-repeated code between `step.run()` and `step.save_model()` that were both designed to handle lists of models.  I don't know if `ModelContainer` was the original reason this was all implemented, but it doesn't seem necessary to me and handling everything inside `save_model` made more sense in my head.

Thanks. Is is straightforward to undo that de-duplication? It sounds like it's not required for this PR and would hopefully make it easier to review.

* It was necessary to also allow a Sequence that had its own `save()` attribute because `SourceModelContainer` has one.  I can see the argument for removing this too, but the problem is that `SourceModelContainer` encodes a `MultiExposureModel` and therefore is in some ways akin to a singular datamodel.  There is an open [issue](https://github.com/spacetelescope/jwst/issues/8742) about rethinking `SourceModelContainer` and I'd love to hear your thoughts, but the more I think about it, the more I think it's mostly fine as-is.

Can this be handled in jwst (either in the Step subclass or by fixing SourceModelContainer to have an API consistent with ModelContainer)? That way the code here that's shared with roman can be kept generic.

* I decided to make it impossible to save lists-of-lists.  The motivation for that was the jwst badpix_selfcal step, for which allowing Sequence-type input in the way I'm doing it meant that `step.save_model()` was saving the science exposure and all background exposures to the same filename.  In a previous version, I had modified `save_model` for just the `badpix_selfcal_step`, but that made it so the stpipe changes were not compatible with jwst/main.  This decision could be changed though.

I'll have to look at this a bit more to understand it. Are you saying the code on main isn't behaving as expected so this PR is also fixing a different bug?

emolter · 2024-10-08T22:40:20Z

Are you saying the code on main isn't behaving as expected so this PR is also fixing a different bug?

No, but it's behaving in an undefined way.

Can this be handled in jwst (either in the Step subclass or by fixing SourceModelContainer to have an API consistent with ModelContainer)? That way the code here that's shared with roman can be kept generic.

Not sure what isn't generic about the present solution, as it does not check for any jwst-specific data types, just a save method. It's also required to maintain support for the old version of ModelContainer.

Is is straightforward to undo that de-duplication? It sounds like it's not required for this PR and would hopefully make it easier to review.

I can try.
edit: regression tests that attempt this here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1775/ using PR branch for JWST
and for the JWST main branch here https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1776/

braingram · 2024-10-09T13:10:15Z

No, but it's behaving in an undefined way.

What's undefined? I'm expecting the answer might be the lack of documentation.

Not sure what isn't generic about the present solution, as it does not check for any jwst-specific data types, just a save method. It's also required to maintain support for the old version of ModelContainer.

I sort of expected that a simplification of ModelContainer in jwst wouldn't involve changes to stpipe (or at least wouldn't add code here since ModelContainer isn't used in roman). The comment # JWST SourceModelContainer takes this path indicates that there is a logical path being added that will never be used by roman. Even though this isn't checking specifically for a jwst type it (and the other paths that handle only ModelContainer) look designed to only handle the jwst types. Would it be easier to code and read to check for those types? This would be easier if some portion of this step code was implemented in the jwst Step subclass (which might require some refactoring here to provide methods that the subclass can override).

I'd be curious to hear your thoughts on either:

implementing this jwst specific logic in the jwst Step subclass
retaining the ModelContainer API that stpipe expects. This would put a limit on what simplifcation is possible within jwst. Are all of the changes here a result of dropping save from ModelContainer?

emolter · 2024-10-09T13:16:54Z

have a look at https://github.com/spacetelescope/stpipe/pull/194/files
Does this look more like what you expected?

edit: I added the change to this branch, as it seems to be the best thing to do. Restarting regtests with jwst/main and this branch here. All passing!

I think I was using the refactor as a crutch to understand the code better. I think you're right that this was possible with many fewer changes to step.py.

The regression tests linked above show that this more minimal change is fully back-compatible with the current JWST pipeline on main, and the one failure on the PR branch looks easily fixed

for more information, see https://pre-commit.ci

braingram · 2024-10-10T13:22:19Z

Thanks for updating this PR. I think my previous comment and question got missed. My main question is are all of the changes here a result of dropping save from ModelContainer?

emolter · 2024-10-10T13:26:36Z

are all of the changes here a result of dropping save from ModelContainer?

That's part of it, but mainly it's dropping the inheritance of AbstractDataModel from ModelContainer. It's true that ModelContainer used to be saved by its own save method, but that only worked within stpipe because it was being caught by isinstance(model, AbstractDataModel).

The changes to the skip logic have nothing to do with save and are entirely because of dropping the inheritance, for example

braingram · 2024-10-10T13:32:37Z

Thanks. If save is kept does the simplified ModelContainer pass an isinstance check against AbstractDataModel? I think so and hopefully that would mean no changes are needed here in stpipe. It sounds like SourceModelContainer still needs a save so I'm in favor of keeping save on ModelContainer (especially if it means no changes are needed here).

emolter · 2024-10-10T13:47:56Z

If save is kept does the simplified ModelContainer pass an isinstance check against AbstractDataModel

Not sure what you're asking, of course it doesn't pass such a check unless it's inheriting from AbstractDataModel, which it is not. Those checks can (and, basically, have) been changed to a check whether the input has its own save method. I can see an argument for only checking that, and not whether it's a datamodel at all.

I am not against retaining a save method for ModelContainer. However, I don't think that would mean zero changes to stpipe, and to be frank I still do not understand the hesitancy to make the proposed changes.

braingram · 2024-10-10T14:11:07Z

Not sure what you're asking, of course it doesn't pass such a check unless it's inheriting from AbstractDataModel, which it is not. Those checks can (and, basically, have) been changed to a check whether the input has its own save method. I can see an argument for only checking that, and not whether it's a datamodel at all.

The subclasshook in AbstractDataModel means explicit inheritance isn't required. Here's an example:

import stpipe

class NotAModel:
    def save(self):
        pass

    @property
    def crds_observatory(self):
        pass

    def get_crds_parameters(self):
        pass

assert isinstance(NotAModel(), stpipe.datamodel.AbstractDataModel)

The assert succeeds even though NotAModel doesn't explicitly inherit from AbstractDataModel. This is how stpipe can check for a "DataModel" even though neither JwstDataModel or RomanDataModel inherit from AbstractDataModel.

I am not against retaining a save method for ModelContainer. However, I don't think that would mean zero changes to stpipe, and to be frank I still do not understand the hesitancy to make the proposed changes.

My hesitation is that stpipe is mostly unmaintained and a critical part of jwst and roman. It's pretty lacking in tests (it might be a bug in the coverage but this PR shows only 50% of the diff covered) and especially for roman the tests in the downstream packages don't well cover all the expected features from stpipe. Given that the motivation for this change is entirely due to jwst it seems overly risky to change stpipe if not necessary.

emolter · 2024-10-10T14:30:35Z

The subclasshook in AbstractDataModel means explicit inheritance isn't required.

Well, that would have been very good for me to know at the start of all of this. Learning hurts I guess...

With that knowledge it does seem likely that retaining save would allow stpipe to go largely unchanged. I will try that out.

My hesitation is that stpipe is mostly unmaintained and a critical part of jwst and roman. It's pretty lacking in tests (it might be a bug in the coverage but this PR shows only 50% of the diff covered) and especially for roman the tests in the downstream packages don't well cover all the expected features from stpipe

The way I'm hearing this is, "stpipe should be maintained and tests should be expanded to cover any changes." Historically things have changed as a result of ModelContainer specifically. But maintaining the repository requires a human person, of course, so it's a management question and probably beyond either of our control.

If I can make things work without changes to step.run(), I will change this PR to just add the unit tests. Do you have any comments about those?

emolter · 2024-10-10T18:45:50Z

After discussion with Brett and Perry, I started a regression test run for JWST where stpipe is pinned to this PR branch, and the crds_observatory() and get_crds_parameters() methods have both been removed from ModelContainer. Edit: apparently crds_observatory is queried, starting another one with only get_crds_parameters() removed.

I also talked with Melanie and Tyler, and they want me to do my best to avoid requiring any stpipe changes with the jwst changes because they don't want the jwst development cycle to rely on changes and testing in stpipe that nobody is responsible for.

Therefore, the plan is:

Assuming the above regtests pass, add documentation to the crds methods in ModelContainer saying they have no effect, clarifying what actually happens, and maybe making them return None or something so their (lack of) effect is extra clear. Edit: the regtests did pass, errors in file naming for outlier detection crf files are unrelated.
Open an issue to decide later whether we want to remove those two methods from ModelContainer and update stpipe such that it can accept a ModelContainer that is not an AbstractDataModel, or add support into stpipe for actually calling them, or both, or neither.
Put the save method back into ModelContainer. Melanie thinks it doesn't hurt anything, although she is doubtful many users are using it. And it's necessary to avoid stpipe changes.
Fix any issues related to the new ModelContainer by modifying step code inside jwst, and not touching stpipe if at all possible.
Make a PR that includes the unit tests only, and removes all code changes.
Withdraw this PR

emolter added 4 commits September 11, 2024 10:54

handle Sequence input for skip and save_results

f4c2814

Merge branch 'main' of https://github.com/spacetelescope/stpipe into …

20f0feb

…JP-3721

make Sequence calls work with SourceModelContainer

7afa00c

fix stpipe unit tests

4b63ec2

emolter mentioned this pull request Sep 26, 2024

JP-3721: Simplify ModelContainer spacetelescope/jwst#8831

Open

10 tasks

emolter force-pushed the JP-3721 branch 2 times, most recently from ac93d7c to e44fce4 Compare September 26, 2024 22:42

emolter force-pushed the JP-3721 branch from 4de4876 to 4b63ec2 Compare October 7, 2024 19:11

added changelog entry

8298c38

emolter force-pushed the JP-3721 branch from 1e7a7aa to 8298c38 Compare October 7, 2024 19:19

fix overwriting same file multiple times when list of list passed in

9eeddb1

emolter force-pushed the JP-3721 branch from 9e096df to 9eeddb1 Compare October 8, 2024 13:36

added simple tests of step save methods

30888d4

emolter force-pushed the JP-3721 branch from cd96894 to 30888d4 Compare October 8, 2024 15:10

added unit test for ModelContainer with custom save method

83bbea0

emolter force-pushed the JP-3721 branch from 5731ac9 to 83bbea0 Compare October 8, 2024 15:48

emolter marked this pull request as ready for review October 8, 2024 15:58

emolter requested a review from a team as a code owner October 8, 2024 15:58

emolter requested review from braingram, tapastro and melanieclarke and removed request for a team October 8, 2024 15:58

added simple tests of skip

622b58a

emolter force-pushed the JP-3721 branch from 279a481 to 622b58a Compare October 8, 2024 17:40

fix silly problem with unit test

843c18c

emolter force-pushed the JP-3721 branch from 8f5b91c to 843c18c Compare October 8, 2024 17:46

braingram reviewed Oct 8, 2024

View reviewed changes

less invasive changes to save logic in step

15936e9

emolter force-pushed the JP-3721 branch from 7e1a79c to 15936e9 Compare October 9, 2024 21:14

[pre-commit.ci] auto fixes from pre-commit.com hooks

7ed2255

for more information, see https://pre-commit.ci

emolter requested a review from braingram October 10, 2024 13:18

emolter mentioned this pull request Oct 10, 2024

Unit tests for step.save and step.skip #197

Open

7 tasks

emolter closed this Oct 11, 2024

emolter mentioned this pull request Oct 11, 2024

Step and Pipeline do not respect get_crds_parameters override for JWST ModelContainer #196

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JP-3721: Simplify ModelContainer #190

JP-3721: Simplify ModelContainer #190

emolter commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

emolter commented Oct 7, 2024 •

edited

Loading

emolter commented Oct 8, 2024 •

edited

Loading

emolter commented Oct 8, 2024

braingram left a comment

braingram Oct 3, 2024

braingram Oct 3, 2024

braingram Oct 8, 2024

emolter commented Oct 8, 2024

braingram commented Oct 8, 2024

emolter commented Oct 8, 2024 •

edited

Loading

braingram commented Oct 9, 2024

emolter commented Oct 9, 2024 •

edited

Loading

braingram commented Oct 10, 2024

emolter commented Oct 10, 2024 •

edited

Loading

braingram commented Oct 10, 2024

emolter commented Oct 10, 2024

braingram commented Oct 10, 2024

emolter commented Oct 10, 2024

emolter commented Oct 10, 2024 •

edited

Loading

	elif isinstance(args[0], Sequence) and self.class_alias is not None:
	elif isinstance(args[0], Sequence) and not isinstance(args[0], str) and self.class_alias is not None:

	if isinstance(args[0][0], AbstractDataModel):
	if args[0] and isinstance(args[0][0], AbstractDataModel):

	"Could not record skip into DataModel" " header: %s",
	"Could not record skip into DataModel header: %s",

JP-3721: Simplify ModelContainer #190

JP-3721: Simplify ModelContainer #190

Conversation

emolter commented Sep 26, 2024 • edited Loading

Tasks

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

emolter commented Oct 7, 2024 • edited Loading

emolter commented Oct 8, 2024 • edited Loading

emolter commented Oct 8, 2024

braingram left a comment

Choose a reason for hiding this comment

braingram Oct 3, 2024

Choose a reason for hiding this comment

braingram Oct 3, 2024

Choose a reason for hiding this comment

braingram Oct 8, 2024

Choose a reason for hiding this comment

emolter commented Oct 8, 2024

braingram commented Oct 8, 2024

emolter commented Oct 8, 2024 • edited Loading

braingram commented Oct 9, 2024

emolter commented Oct 9, 2024 • edited Loading

braingram commented Oct 10, 2024

emolter commented Oct 10, 2024 • edited Loading

braingram commented Oct 10, 2024

emolter commented Oct 10, 2024

braingram commented Oct 10, 2024

emolter commented Oct 10, 2024

emolter commented Oct 10, 2024 • edited Loading

emolter commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

emolter commented Oct 7, 2024 •

edited

Loading

emolter commented Oct 8, 2024 •

edited

Loading

emolter commented Oct 8, 2024 •

edited

Loading

emolter commented Oct 9, 2024 •

edited

Loading

emolter commented Oct 10, 2024 •

edited

Loading

emolter commented Oct 10, 2024 •

edited

Loading