Skip to content

Close files of garbage collected file managers once their lock is free - #11692

Merged
headtr1ck merged 2 commits into
pydata:mainfrom
headtr1ck:fix-flaky-netcdf4-distributed
Oct 8, 2026
Merged

Close files of garbage collected file managers once their lock is free#11692
headtr1ck merged 2 commits into
pydata:mainfrom
headtr1ck:fix-flaky-netcdf4-distributed

Conversation

@headtr1ck

@headtr1ck headtr1ck commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Description

Fixes the flaky test_open_mfdataset_netcdf4_parallel_distributed, whose dask workers occasionally segfaulted inside HDF5.

  • CachingFileManager.__del__ left its file open if another thread held the lock, so distributed workers ended up with several handles open on the same file
  • With a variable-length string variable, HDF5 can crash when reopening a file after one of several handles was closed (upstream bug, see Segfault on interleaved reads on same dataset from different file handles h5py/h5py#2920)
  • Such files are now closed in a separate thread once the lock is free
  • Locally the test crashed every 5-13 runs before, and passed 95 runs in a row with the fix

Checklist

AI Disclosure

  • This PR contains AI-generated content.
    • I have tested any AI-generated content in my PR.
    • I take responsibility for any AI-generated content in my PR.
      Tools: Claude Code

[This is Claude Code on behalf of Michael Niklas]

🤖 Generated with Claude Code

headtr1ck and others added 2 commits October 8, 2026 12:28
@headtr1ck @claude
Close files of garbage collected managers once their lock is free
544612a 
CachingFileManager.__del__ only closed its file if it could take the lock
without blocking. Otherwise the file stayed open for the rest of the
process. On dask distributed workers this regularly left a second handle to
a file open, and with several netCDF4 handles open on the same path, HDF5
can segfault while opening the file again once one of them is closed
(variable-length strings, see h5py/h5py#2920). This made
test_open_mfdataset_netcdf4_parallel_distributed flaky.

Close such files in a separate thread once the lock is free instead.

Co-authored-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@headtr1ck @claude
Add PR number to whats-new
a5b8c79 
Co-authored-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 8, 2026
Open
Open
Open
dcherian
dcherian approved these changes Oct 8, 2026
View reviewed changes

@dcherian dcherian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

insane solution; great use of Claude if it indeed fixes it :P

@headtr1ck headtr1ck added the plan to merge Final call for comments label Oct 8, 2026
@headtr1ck
headtr1ck merged commit 8bc8e8a into pydata:main Oct 8, 2026
59 checks passed
@headtr1ck
headtr1ck deleted the fix-flaky-netcdf4-distributed branch October 8, 2026 18:16
@kmuehlbauer kmuehlbauer mentioned this pull request Oct 11, 2026
Merged
4 tasks done
aladinor added a commit to aladinor/xradar that referenced this pull request Oct 12, 2026
@aladinor
fix(io): keep CfRadial2 lazy with a single cache layer
14423f8 
Builds on the lazy CfRadial2 reader (tree._close plus
datatree_from_dict_with_io_cleanup):

- the inner open_datatree runs with cache=False, so read data is cached
  once, by xarray's outer open, instead of twice
- the inner tree is closed when normalization fails, including warnings
  raised as errors
- open_groups_as_dict keeps the close hook when site_coords=False drops
  the station variables
- single-group open_dataset is lazy too (was ds.load()) and closes its
  file on close()

Several handles on one file can crash HDF5 with variable-length strings;
fixed upstream in pydata/xarray#11692 (unreleased).

Refs openradar#481
@aladinor aladinor mentioned this pull request Oct 12, 2026
Closed
3 tasks
aladinor added a commit to aladinor/xradar that referenced this pull request Oct 12, 2026
@aladinor
test(io): skip the CfRadial2 laziness test until xarray#11692 is rele…
acadc9f 
…ased

With the released xarray, several open handles on one netCDF4 file can
crash HDF5 (h5py/h5py#2920); the Python 3.12 unit-test job hung on this
test. Re-enable it once an xarray release contains pydata/xarray#11692,
tracked in openradar#486.
aladinor added a commit to aladinor/xradar that referenced this pull request Oct 12, 2026
@aladinor
ci: test against xarray main until pydata/xarray#11692 is released
6d05c94
kmuehlbauer added a commit to openradar/xradar that referenced this pull request Oct 13, 2026
@aladinor @kmuehlbauer
FIX: legacy open_*_datatree regressions after #335 (#484)
f252cec 
* fix(io): restore legacy open_*_datatree options via xr.open_datatree

#335 made the deprecated open_*_datatree functions and xd.open_datatree
call the backend's open_datatree directly, which skipped xarray's dataset
wrapping and dropped most backend_kwargs. Both now go through
xr.open_datatree (shared _open_legacy_datatree helper), so:

- chunks, cache and other xarray options work again
- backend_kwargs reach the backend; xarray options passed there are
  lifted to the top level; legacy `Optional` and `site_as_coords` map to
  `optional` and `site_coords` (passing both site spellings raises)
- legacy optional=False no longer raises "multiple values"
- iris, rainbow, furuno and datamet sweeps keep their range index

Also:
- metadata subgroups no longer copy latitude/longitude/altitude
- reindex_angle is resolved once per tree; its FutureWarning, the NEXRAD
  drop warnings and the CfRadial2 warnings point at the caller
- cfradial1/cfradial2 backend option for the netCDF reader is
  netcdf_engine (engine collided with xarray's argument); the legacy
  engine= keyword maps to it
- CfRadial2 keeps its inner file open for the tree's lifetime and
  DataTree.close() closes it (closing and reopening crashed netCDF4)
- open_uf_datatree has its signature and docstring back; docstrings no
  longer list parameters an engine rejects; IRIS drops an unused `group`

Closes #481

* test(io): compare legacy and engine datatree paths

Legacy open_*_datatree output must equal xd.open_datatree for the same
options; cover chunks, backend_kwargs (incl. decoders and `Optional`),
optional=False, optional_groups, site_as_coords, the range index,
xd vs xr equality, sweep identity, the reindex_angle warning, the UF
signature, docstrings, netcdf_engine and CfRadial2 close.

Refs #481

* docs: link PR #484 in the changelog

* test(io): cover legacy backend_kwargs engine mapping

The legacy open_cfradial1_datatree and open_cfradial2_datatree map an inner netCDF `engine` given in backend_kwargs to netcdf_engine; check it reaches the reader.

* fix(io): read CfRadial2 in one pass and close the file

Going through xr.open_datatree made xarray reopen the CfRadial2 file for
lazy reads after the backend had closed it; with several handles on the
same file netCDF4/HDF5 segfaulted intermittently (locally and once in
CI). The backend now loads the normalized groups inside the `with` block
and closes the file before returning, so a tree holds no open handle.

Refs #481

* test(io): close CfRadial2 trees in tests

* attempt to fix cfradial2 closing issues, specify FutureWarning which imd should not raise

* black & ruff

* try Filelock in cfradial2 fixture to prevent segfault, add filelock to unittests.yml, use xarray from conda again

* remove numpy v1 test runs, pin Python>=3.12, add filelock to nightly tests

* lock fetching of cfradial1_file with FileLock

* use context manager for cfradial2 open_datatree test

* ci: test against xarray main until pydata/xarray#11692 is released

---------

Co-authored-by: Kai Mühlbauer <kmuehlbauer@wradlib.org>
Co-authored-by: Kai Mühlbauer <kai.muehlbauer@uni-bonn.de>
This was referenced Oct 13, 2026
Closed
Closed
@syedhamidali syedhamidali mentioned this pull request Oct 13, 2026
Merged
@aladinor aladinor mentioned this pull request Oct 13, 2026
Merged
1 task done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcherian dcherian dcherian approved these changes

Assignees

No one assigned

Labels

io plan to merge Final call for comments topic-backends

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@headtr1ck @dcherian