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

Investigate need for lazy_loading asdf files #1341

Closed
braingram opened this issue Jul 31, 2024 · 4 comments · Fixed by #1342
Closed

Investigate need for lazy_loading asdf files #1341

braingram opened this issue Jul 31, 2024 · 4 comments · Fixed by #1342

Comments

@braingram
Copy link
Collaborator

Now that files can be loaded with a "lazy_tree" (spacetelescope/roman_datamodels#358) some investigation of the "lazy_load" usage in romancal is worth undertaking:
https://github.com/search?q=repo%3Aspacetelescope%2Fromancal%20lazy_load&type=code

The use of "lazy_load=False" prevents much of the benefits of "lazy_tree" since with "lazy_load=False" all ASDF block data is loaded on the call to asdf.open.

@schlafly
Copy link
Collaborator

Here's the PR where we introduced lazy_load = False: #774 (comment)

Some back and forth between me and you there! It looks like the core issue is that we need to think harder about when we're responsible for closing files if we want to do lazy_load = False, which certainly sounds doable.

@braingram
Copy link
Collaborator Author

Thanks! The point about when files are closed is a great addition to this issue.

I started a test PR with the lazy_load=False lines removed #1342 to see what breaks. Hopefully that will tell us something about the scope of the required changes. I suspect that this is related to input handling for steps (#1313).

@braingram
Copy link
Collaborator Author

braingram commented Jul 31, 2024

The above test PR shows no unit test failure but several regression test failures:

FAILED romancal/regtest/test_dark_current.py::test_dark_current_subtraction_step - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_dark_current.py::test_dark_current_outfile_step - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_dark_current.py::test_dark_current_outfile_suffix - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_dark_current.py::test_dark_current_output - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_wfi_flat_field.py::test_flat_field_image_step - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_wfi_flat_field.py::test_flat_field_crds_match_image_step - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_wfi_pipeline.py::test_level2_image_processing_pipeline - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_linearity.py::test_linearity_step - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_linearity.py::test_linearity_outfile_step - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_wfi_saturation.py::test_saturation_image_step - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_wfi_saturation.py::test_saturation_grism_step - OSError: Attempt to load block from closed file
FAILED romancal/regtest/test_wfi_photom.py::test_absolute_photometric_calibration - OSError: Attempt to load block from closed file

@braingram
Copy link
Collaborator Author

To provide one example of an issue in the code that disabling lazy loading is currently hiding is in the flat field step.

The step modifies the input model in place but also explicitly closes the input file:


So when provided a filename as an input it will open the model in the file, perform the flat field, then close the file and return the model (which refers to a closed file). This leads to the errors above in the flat field regression tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants