Add AbstractModelContainer

[braingram]

This PR adds AbstractModelContainer which behaves similarly to AbstractDataModel.

The interface/protocol for AbstractModelContainer was constructed to:

• comply with romancal.datamodel.container.ModelContainer
• comply with jwst.datamodels.container.ModelContainer
• Include API required by stpipe:
  • have a crds_observatory attribute
  • be iterable (have an __iter__ attribute)
  • have a save method
• and to include a few more common requirements that also allow this abstract class to be distinct from AbstractDataModel
  • have a read_asn method
  • have a from_asn method

To test the new AbstractModelContainer the jwst and romancal tests were updated. This conflicts with the changes in #116 It expands upon the changes in that PR to include running the integration tests when jwst and romancal/roman_datamodels are installed in the CI and extends those tests to test both AbstractDataModel and AbstractModelContainer.

This PR is likely an alternative to spacetelescope/stdatamodels#102

[codecov]

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
src/stpipe/pipeline.py	34.67% <25.00%> (ø)
src/stpipe/step.py	48.17% <25.00%> (+0.09%)	⬆️
src/stpipe/container.py	75.86% <75.86%> (ø)

📢 Thoughts on this report? Let us know!.

[braingram]

Roman regression tests: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/436/
All tests passed (workflow failed on artifact upload)

JWST regression tests: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1013/
All tests passed

[braingram]

@schlafly I don't seem to be able to request you as a reviewer for this PR. Given that it relates to roman (and jwst) is there someone that can review this with an eye for the romancal interaction?

[WilliamJamieson]

This PR adds AbstractModelContainer which behaves similarly to AbstractDataModel.

I do not like this approach as it digs us further into reliance on old difficult to follow Python practices. Instead this (and for that matter AbstractDataModel) should be defined using typing.runtime_checkable Python typeing.Protocol generic objects. Essentially, Protocols (which the changes suggested here are not actually a protocol in the python sense) are a way for library authors to consistently define a template for what an object which contains expected features for the library to hook into.

This is precisely what the intent for AbstractDataModel is attempting to provide. Indeed,

stpipe/src/stpipe/datamodel.py

Lines 18 to 31 in de7afd9

	@classmethod
	def __subclasshook__(cls, C):
	"""
	Pseudo subclass check based on these attributes and methods
	"""
	if cls is AbstractDataModel:
	mro = C.__mro__
	if (
	any([hasattr(CC, "crds_observatory") for CC in mro])
	and any([hasattr(CC, "get_crds_parameters") for CC in mro])
	and any([hasattr(CC, "save") for CC in mro])
	):
	return True
	return False

makes it so that any object containing crds_observatory, get_crds_parameters, and save attributes will pass an isinstance(object, AbstractDataModel) check without any need for inheritance. The lack of inheritance is necessary because there is no good reason for "DataModel-libraries" which are "stpipe-compatible" to require stpipe as a dependency. In fact, this is counter-productive in general as those libraries should be functional outside of pipeline execution code, e.g. opening the results of a pipeline run. For example both:

• stdatamodels.DataModel for JWST
• and roman_datamodels.DataModel for ROMAN

do NOT have any inheritance from AbstractDataModel despite being used in our pipelines as AbstractDataModel objects when passed to stpipe. Indeed, to my knowledge there is no actual code which inherits from AbstractDataModel instead all usages of AbstractDataModel are really treatments of it as if it was a Python Protocol, which it is not.

Since AbstractDataModel is not in fact a true Python Protocol, things like modern IDE tooling don't correctly flag/handle things surrounding its effective use quite correctly. It also is quite repetitive in its construction as it attempts to act as a Protocol and an abstract base class, despite there being no intention of using it as a base class for anything.

Thus I suggest that the proposed notion of an AbstractModelContainer be dropped in favor of simply defining a ModelContainer Protocol instead.

[schlafly]

Sorry, I can't provide useful feedback about this kind of thing. Since Roman & JWST regression tests pass it looks fine to me and I would approve were I able. I will defer to you and William on protocols vs. abstract interfaces vs. ...

[braingram]

I'm closing this in favor of changes to the ModelContainer api may help to address memory issues in JWST.