Remove __del__ from DataModel.

[WilliamJamieson]

We have now encountered several issues concerning roman_datamodels closing there associated ASDF file unexpectedly and/or unreliably. These issues tend to be extremely difficult to debug as the bugs cannot be consistently replicated. In the majority of cases these issues are caused by the fact that there is a DataModel.__del__ method which triggers the closure of the associated ASDF file if there is one. This method is "called" whenever a given object's reference count drops to zero, and in this case it is improperly being used as a "destructor".

The issue with comes with the fact that the ASDF file opened and held by the DataModel itself references the DataModel which creates a reference cycle. Thus in this case DataModel.__del__ is only called on a given instance by the garbage collector not when its apparent reference count drops to zero. Consequently, we cannot guarantee when/if DataModel.__del__ is successfully called on a given DataModel instance. This ambiguity can lead to issues where sometimes operations which reference data in the ASDF file work and sometimes they do not, the root of which is when the Python garbage collector actually gets run during a particular program execution instance.

Note that the reason why DataModel.__del__ exists the way that it does is so that usage of DataModels should not leave behind open files. While this is important, the inherent inconsistencies with the approach of using DataModel.__del__ create's more issues than it solves. Instead, either

1. DataModel.__del__ should be removed.
2. Datamodel.__del__ should raise an error if the ASDF file is still open.

Currently, this PR removes DataModel.__del__.

Note that to avoid issues with DataModels and file handles we really should make the roman_datamodels.open method a proper context manager instead of just a function, so that it explicitly cleans up the DataModel whenever it exits AND resolve any open file warnings/errors we encounter during testing.

Checklist

• Added entry in CHANGES.rst under the corresponding subsection
• updated relevant tests
• updated relevant documentation
• Passed romancal regression testing on Jenkins / PLWishMaster. Link: https://plwishmaster.stsci.edu:8081/job/RT/job/romancal/XXX/

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c9d6323) 97.04% compared to head (baf2523) 96.95%.

❗ Current head baf2523 differs from pull request most recent head 6dc36ca. Consider uploading reports for the commit 6dc36ca to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
- Coverage   97.04%   96.95%   -0.09%     
==========================================
  Files          28       28              
  Lines        2504     2463      -41     
==========================================
- Hits         2430     2388      -42     
- Misses         74       75       +1     

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

[schlafly]

It's useful for me to think about a specific use case. The particular error I hit was on this line:
https://github.com/spacetelescope/jwst/blob/master/jwst/resample/resample.py#L285-L360
where the model container had overloaded getitem so that this was effectively the same as rdm.open(filename).meta.data.items. With your change, we'd be responsible for closing that, so it would be more like:

f = image_models[0]
f.meta....
f.close()

I think that would argue for a rework of ModelContainer for this case, since at least to me it's surprising to have to close things like that, where the open is pretty implicit?

It's not obvious to me how much would have to be reworked. But broadly I do think we'd benefit to being more explicit about opening and closing things. e.g., I don't expect image_models[0] to be expensive, and if we were forced to make the open & closing explicit, it would be more clear that that costs something.

[schlafly]

Following a conversation with William, the expectation is that if we merge this, we'll get some ResourceWarning: unclosed files, until we address cases where we're leaving files open. There will be a fair number of those but we should be able to hunt them down at our leisure. There is some risk that with this change we exhaust the number of open file descriptors until we start regularly closing these contexts.