Fix segfault from opening netCDF files once per variable - #3460
Draft
djhoese wants to merge 4 commits into
Draft
Conversation
NetCDF4FileHandler opened the file with xarray for every variable and closed it again immediately, returning lazy arrays that reopen it at compute time. Every one of those opens builds its own CachingFileManager, and CachingFileManager._make_key() includes a per-manager id, so no two of them ever share an entry in xarray's global FILE_CACHE. Reading more variables than file_cache_maxsize (128 by default) therefore makes that LRU cache evict and close netCDF4 handles while sibling arrays of the same file are still reading through them, which segfaults in libhdf5. Keep one xarray.Dataset open per group instead, for the lifetime of the file handler, and hand out copies so callers can still modify metadata. Add a close() method to release the datasets before the file handler is deleted, which the METimage base tests need in order to reopen their test file for writing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKR5WimPBvwWpVgcJ52TNv
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3460 +/- ##
========================================
Coverage 96.35% 96.36%
========================================
Files 465 465
Lines 59327 59539 +212
========================================
+ Hits 57163 57372 +209
- Misses 2164 2167 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
With cache_handle=False the variable objects stored in file_content belong to the handle closed at the end of __init__. Reading them raised 'NetCDF: Not a valid ID' (netCDF4) or 'I/O operation on closed file' (h5netcdf); for the FCI reader the netCDF4 error was silently swallowed and produced wrongly padded arrays. Read through xarray instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013jjGBN7DkiaVHEnA1qG5dR
Lets a file handler choose how xarray datasets for the groups of a file are created: one open_dataset per group (default, existing behaviour), one open_datatree per file, one shared backend store per file with a child store per group, or a store wrapping the already open cache_handle=True file handle. The last three keep every group of a file in a single xarray file-cache slot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013jjGBN7DkiaVHEnA1qG5dR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was getting a seg fault when processing a large METimage pass. It's getting late here so I had Claude put together a commit with a fix in case anyone wanted to take a look. It isn't noted here but it seems tied to vlen strings (which there is only one in the file) that puts HDF5 into a bad state. I plan on filing an xarray issue tomorrow. Below is Claude's commenting.
I HAVE NOT REVIEWED THIS YET
Claude:
NetCDF4FileHandler opened the file with xarray for every variable and closed it again immediately, returning lazy arrays that reopen it at compute time. Every one of those opens builds its own CachingFileManager, and CachingFileManager._make_key() includes a per-manager id, so no two of them ever share an entry in xarray's global FILE_CACHE. Reading more variables than file_cache_maxsize (128 by default) therefore makes that LRU cache evict and close netCDF4 handles while sibling arrays of the same file are still reading through them, which segfaults in libhdf5.
Keep one xarray.Dataset open per group instead, for the lifetime of the file handler, and hand out copies so callers can still modify metadata. Add a close() method to release the datasets before the file handler is deleted, which the METimage base tests need in order to reopen their test file for writing.
Claude-Session: https://claude.ai/code/session_01TKR5WimPBvwWpVgcJ52TNv
AUTHORS.mdif not there already