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

wfss_contam use datamodel in with context #7425

Merged

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Jan 6, 2023

When the DataModel is cleaned up (del is called) during garbage collection it will close any opened hdulists. This will make any subsequent attempts to load data from the hdulist fail (with ValueError: I/O operation on closed file).

DataModel prior to the below PR contained a self reference which created a reference cycle which made python fail to collect the object during generation 0 passes with the garbage collector. This meant that the hdulist was not immediately closed when the model fell out of scope.

Code in wfss_contam relied on this persistance as the model created in the following line is no longer referenced on subsequent lines.

dimage = datamodels.open(dir_image_name).data

python will attempt to clean up the opened data model and fail, keeping the hdulist (stored at dir_image_name) open. This allowed later lines that reference dimage to load the array data within the hdulist.

Fixing the reference cycle in stdatamodels in this PR: spacetelescope/stdatamodels#109 makes the datamodel more easily garbage collected which causes wfss_contam to fail due to the hdulist closing when the model is collected.

To keep the model in scope, the above line is replaced with a with context to make it explicit that the datamodel is held open which it's contents are read.

This is work towards fixing JP-2798.

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 Author

@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Base: 78.61% // Head: 78.61% // No change to project coverage 👍

Coverage data is based on head (d89abc0) compared to base (d89abc0).
Patch has no changes to coverable lines.

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7425   +/-   ##
=======================================
  Coverage   78.61%   78.61%           
=======================================
  Files         455      455           
  Lines       39143    39143           
=======================================
  Hits        30772    30772           
  Misses       8371     8371           
Flag Coverage Δ
nightly 78.61% <0.00%> (ø)
unit 51.34% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

Code looks good, as do all the CI and regtest results. But I would like it to have a change log entry (in CHANGES.rst). It's not a change in functionality or results, but enough of a change in the code that I'd like to have a record of it (as a bug fix).

CHANGES.rst Outdated Show resolved Hide resolved
@braingram braingram force-pushed the bug/wfss_contam_datamodel_use branch from 65e3532 to fe21efc Compare January 6, 2023 20:23
When the DataModel is cleaned up (__del__ is called)
during garbage collection it will close any opened
hdulists. This will make any subsequent attempts to load
data from the hdulist fail (with ValueError: I/O operation on closed file).

DataModel prior to the below PR contained a self reference
which created a reference cycle which made python fail to collect
the object during generation 0 passes with the garbage collector.
This meant that the hdulist was not immediately closed when
the model fell out of scope.

Code in wfss_contam relied on this persistance as the model created
in the following line is no longer referenced on subsequent lines.

dimage = datamodels.open(dir_image_name).data

python will attempt to clean up the opened data model and fail,
keeping the hdulist (stored at dir_image_name) open. This allowed
later lines that reference dimage to load the array data within the
hdulist.

Fixing the reference cycle in stdatamodels in this PR:
spacetelescope/stdatamodels#109
makes the datamodel more easily garbage collected which causes
wfss_contam to fail due to the hdulist closing when the model
is collected.

To keep the model in scope, the above line is replaced with
a with context to make it explicit that the datamodel is held
open which it's contents are read.

update changelog
@braingram braingram force-pushed the bug/wfss_contam_datamodel_use branch from fe21efc to e792de1 Compare January 9, 2023 14:22
@hbushouse hbushouse merged commit 597b712 into spacetelescope:master Jan 10, 2023
@braingram braingram deleted the bug/wfss_contam_datamodel_use branch January 10, 2023 13:59
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

2 participants