Fix remote HTTP test stalls and tune HTTP HDF5 caching - #784
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughRemote HDF5 access now pauses garbage collection for loop-backed and remote handles, uses bounded HTTP block caching, improves failure cleanup and abort semantics, expands HTTP regression coverage, and runs network tests across the shared OS matrix. ChangesRemote HDF5 lifecycle hardening
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_utils/test_io_utils.py (1)
409-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover HTTPS in this regression test.
The implementation and constants now support both
httpandhttps, but this test exercises onlyhttp://. Parameterize the scheme so HTTPS regressions cannot pass unnoticed.Proposed test update
- def test_h5_reader_uses_block_cache_for_http_upath(self, monkeypatch): + `@pytest.mark.parametrize`("scheme", ("http", "https")) + def test_h5_reader_uses_block_cache_for_http_upath(self, monkeypatch, scheme): ... - path = UPath("http://example.com/example.h5") + path = UPath(f"{scheme}://example.com/example.h5")As per PR objectives, HTTP and HTTPS HDF5 caching are both in scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_utils/test_io_utils.py` around lines 409 - 440, Parameterize test_h5_reader_uses_block_cache_for_http_upath over both “http” and “https” schemes, and construct the UPath URL from that parameter. Keep the existing monkeypatches, configuration, and block-cache assertions unchanged so the regression test covers both supported protocols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_utils/test_io_utils.py`:
- Around line 409-440: Parameterize
test_h5_reader_uses_block_cache_for_http_upath over both “http” and “https”
schemes, and construct the UPath URL from that parameter. Keep the existing
monkeypatches, configuration, and block-cache assertions unchanged so the
regression test covers both supported protocols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bd3f303-be17-44cd-a670-ca695eb36be5
📒 Files selected for processing (4)
dascore/constants.pydascore/utils/hdf5.pytests/test_io/conftest.pytests/test_utils/test_io_utils.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ea86f396c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #784 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 177 177
Lines 20153 20253 +100
==========================================
+ Hits 20153 20253 +100
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:
|
|
Note on scope: this fixes the reproducible stall class (accept-loop parking — before: 2/3 pinned runs of the module hit the 30 s pytest-timeout; after: 5/5 stable) and the transfer amplification, but one intermittent stall remains in |
|
Update: the residual stall is root-caused and fixed in ba6ba5c — it was never a network problem. It's a platform-agnostic ABBA deadlock: h5py holds its global Evidence: faulthandler burst dumps during a live stall show the loop thread frozen at an allocation point with Fix: remote h5py handles pause automatic collection for their bounded lifetime (resumed on close) — the mitigation h5py's file-object docs recommend for exactly this pattern. Pre-pause |
|
Ran three independent adversarial reviews on the GC-pause mechanism (lock-ordering, CPython GC semantics, handle lifecycle/platforms) — commit 6a0a8ef addresses everything they surfaced: Real gaps fixed:
Verified sound by the reviews (now documented in code): refcount-driven h5py deallocs can never land on the loop thread because references flow strictly one way through the sync bridge; the implementation matches h5py's file-object docs' recommended mitigation; wasm and free-threaded CPython carry over. Windows: the review confirmed the fixture stack is Windows-correct and the skip's stated cause is the now-fixed platform-agnostic deadlock, so the win32 skip is removed and |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.xdg-test/pint/01448202a267fcbe1a419a34e97f9f53346f5212.json:
- Line 1: Replace the machine-specific absolute source paths in the committed
Pint cache metadata with portable identifiers, or regenerate all entries in CI.
Apply this to .xdg-test/pint/01448202a267fcbe1a419a34e97f9f53346f5212.json:1-1,
.xdg-test/pint/a4391b0ab5f2b7b7b67c1b2c28cd86d2cb5b3dec.json:1-1,
.xdg-test/pint/c2a57dde47058a3a44a02d7d48817affe593f48e.json:1-1, and
.xdg-test/pint/db8943bf1adb3e18852fbc22864ab561c4a6be8c.json:1-1, preserving the
respective constants_en.txt and default_en.txt metadata.
In `@dascore/utils/io.py`:
- Around line 247-258: Update close_all to isolate cleanup failures per cached
handle: wrap each handle’s abort or close operation in exception handling so one
failure does not stop iteration through self._cache. Continue attempting every
remaining handle, while preserving the existing abort selection and normal close
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 20e16382-14c6-491b-8f48-3c212c63f518
⛔ Files ignored due to path filters (6)
.xdg-test/pint/01448202a267fcbe1a419a34e97f9f53346f5212.pickleis excluded by!**/*.pickle.xdg-test/pint/0ffe33abec788d9f5d1f14291461e7bed27e2a4f.pickleis excluded by!**/*.pickle.xdg-test/pint/a4391b0ab5f2b7b7b67c1b2c28cd86d2cb5b3dec.pickleis excluded by!**/*.pickle.xdg-test/pint/c2a57dde47058a3a44a02d7d48817affe593f48e.pickleis excluded by!**/*.pickle.xdg-test/pint/cfc24d8be1b723f7021ad520e66c8ae0e35e7152.pickleis excluded by!**/*.pickle.xdg-test/pint/db8943bf1adb3e18852fbc22864ab561c4a6be8c.pickleis excluded by!**/*.pickle
📒 Files selected for processing (15)
.github/workflows/runtests.yml.xdg-test/dascore/data/0.0.0/terra15_das_1_trimmed.hdf5.xdg-test/pint/01448202a267fcbe1a419a34e97f9f53346f5212.json.xdg-test/pint/0ffe33abec788d9f5d1f14291461e7bed27e2a4f.json.xdg-test/pint/a4391b0ab5f2b7b7b67c1b2c28cd86d2cb5b3dec.json.xdg-test/pint/c2a57dde47058a3a44a02d7d48817affe593f48e.json.xdg-test/pint/cfc24d8be1b723f7021ad520e66c8ae0e35e7152.json.xdg-test/pint/db8943bf1adb3e18852fbc22864ab561c4a6be8c.jsondascore/io/core.pydascore/utils/hdf5.pydascore/utils/io.pydascore/utils/remote_io.pytests/test_io/test_io_core.pytests/test_io/test_remote_http.pytests/test_utils/test_io_utils.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dascore/utils/hdf5.py (1)
130-137: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse explicit
is not Nonewhen picking the wrapped layer.
getattr(resource, "raw", None) or getattr(resource, "buffer", None)relies on truthiness. A file-like wrapper that defines__bool__/__len__(falsy when empty) would be skipped, silently failing detection and reopening the deadlock window this function exists to close.♻️ Proposed change
- wrapped = getattr(resource, "raw", None) or getattr(resource, "buffer", None) + wrapped = getattr(resource, "raw", None) + if wrapped is None: + wrapped = getattr(resource, "buffer", None) if wrapped is None or wrapped is resource: return False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dascore/utils/hdf5.py` around lines 130 - 137, Update the wrapped-layer selection in the resource traversal loop to prefer resource.raw whenever it is not None, otherwise use resource.buffer, without relying on truthiness. Preserve the existing None and self-reference termination checks and async_impl detection.dascore/utils/io.py (1)
264-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the intentional broad cleanup catch.
Ruff reports BLE001 here. The broad catch is justified because cleanup must continue for every handle, but add a narrowly scoped
# noqa: BLE001with rationale (or use a shared cleanup exception) so this does not become a CI lint failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dascore/utils/io.py` around lines 264 - 265, Update the broad exception handler in the cleanup loop around first_exc to add a narrowly scoped # noqa: BLE001 with a concise rationale explaining that cleanup must continue across all handles; preserve the existing first exception capture behavior and avoid broader lint suppressions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dascore/utils/hdf5.py`:
- Around line 151-155: Update the exception cleanup block surrounding the
visible fileobj.close() call so resume_gc() executes in a finally clause even
when close raises BaseException. Mirror the nested try/finally cleanup structure
used by _ManagedH5pyFile.close(), while preserving the existing re-raise
behavior.
---
Nitpick comments:
In `@dascore/utils/hdf5.py`:
- Around line 130-137: Update the wrapped-layer selection in the resource
traversal loop to prefer resource.raw whenever it is not None, otherwise use
resource.buffer, without relying on truthiness. Preserve the existing None and
self-reference termination checks and async_impl detection.
In `@dascore/utils/io.py`:
- Around line 264-265: Update the broad exception handler in the cleanup loop
around first_exc to add a narrowly scoped # noqa: BLE001 with a concise
rationale explaining that cleanup must continue across all handles; preserve the
existing first exception capture behavior and avoid broader lint suppressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e8d1ed25-c220-4aa9-9f7b-46880f9f6bd3
📒 Files selected for processing (6)
dascore/io/core.pydascore/utils/hdf5.pydascore/utils/io.pydascore/utils/remote_io.pytests/test_utils/test_gc_pause.pytests/test_utils/test_io_utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- dascore/utils/remote_io.py
- tests/test_utils/test_io_utils.py
Squashed rebase of fix-remote-hdf5-http-cache onto dev.
- Pause collection only for loop-backed (async fsspec) resources; local and memory UPaths took the remote branch and paused process-wide gc for every HDF5 read. - Fold the two fileobj-driver opens into one helper; keep a caller-supplied file object usable after a failed open, which get_format relies on. - Flatten the tuned-open-kwargs branch, the no-range predicate, and the type-caster error path.
…errors - Run the rate-limited safety collection before the pause is taken, so an interrupt inside gc.collect cannot strand a pause nobody will resume. - Keep a failed abort from replacing the exception that triggered it. - Treat a UPath whose backend is not installed as not loop backed rather than raising while deciding whether to pause.
- Keep the resume balanced when closing a fileobj raises a BaseException. - Cover https alongside http in the tuned-open-kwargs test. - Correct the handle-ownership docstring.
…afety - __del__ only releases the gc pause; closing there would also close an h5py file or stream the caller still owns. - The type caster aborts a handle it created when the method raises, so a failed remote HDF5 write discards its temp file instead of uploading it. close_all and the caster now share one release_handle helper. - pause_gc keeps its depth consistent when interrupted, and the whole paused region of _open_h5_fileobj sits inside its rebalancing try. - _ManagedH5pyFile gets class-level defaults so a half-built wrapper is still closeable; the fork hook resets the safety-valve deadline. - Narrow the changelog abort note to remote HDF5 writes; drop the .buffer hop from the loop-backed check.
- Move the pause guard to the root conftest and make it repair the state before failing, so one leak blames its own test instead of erroring out every test after it. This also covers the pause tests in test_io_utils. - Patch remote_io's gc reference rather than the stdlib module. - Restore gc conditionally in the user-disabled test. - Add a threaded nest/unnest test; drop a dead daemon_threads assignment.
14b6d57 to
e361e5c
Compare
d-chambers
added
ready_for_review
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_utils/test_gc_pause.py (1)
144-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore
remote_io._gc_collect_afterwithmonkeypatch.Lines 151 and 165 assign the module global directly. The value is never restored, so the safety-collect rate limit stays disabled for every later test in the session. That makes
pause_gc()run a fullgc.collect()on each remote open in unrelated tests, which slows the suite and changes timing-sensitive behavior. Line 164 already receivesmonkeypatch; use it for this attribute too.♻️ Proposed fix
- def test_stranded_cyclic_handle_is_recovered(self): + def test_stranded_cyclic_handle_is_recovered(self, monkeypatch): """A handle leaked inside a cycle is healed by the next remote open.""" holder = {} handle = _open_paused(_FakeRemoteFile()) holder["handle"], holder["self"] = handle, holder # unreachable cycle del handle, holder assert not gc.isenabled() - remote_io._gc_collect_after = 0.0 # the valve is rate limited + monkeypatch.setattr(remote_io, "_gc_collect_after", 0.0) # rate limited pause_gc() resume_gc() assert gc.isenabled() @@ monkeypatch.setattr(remote_io, "gc", fake_gc) - remote_io._gc_collect_after = 0.0 # the valve is rate limited + monkeypatch.setattr(remote_io, "_gc_collect_after", 0.0) # rate limited🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_utils/test_gc_pause.py` around lines 144 - 168, Update both tests, test_stranded_cyclic_handle_is_recovered and test_interrupted_safety_collect_takes_no_pause, to set remote_io._gc_collect_after through the existing monkeypatch fixture instead of assigning the module global directly. Add the fixture to the first test and use monkeypatch.setattr in both tests so the original rate-limit value is restored after each test.tests/conftest.py (1)
170-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the failure message match the detected condition.
The fixture fails on two different conditions: a non-zero pause depth, and a changed global GC enable state. When a test leaves
gcdisabled without holding a pause, the message reportsdepth=0and blames the remote-read pause, which is misleading. Report the actual mismatch.♻️ Proposed fix
depth = remote_io._gc_pause_depth if not depth and gc.isenabled() == was_enabled: return + enabled_now = gc.isenabled() remote_io._gc_pause_depth = 0 if was_enabled: gc.enable() else: gc.disable() - pytest.fail(f"test left the remote-read gc pause held (depth={depth})") + pytest.fail( + "test left garbage collection state modified " + f"(pause depth={depth}, gc enabled={enabled_now}, expected={was_enabled})" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 170 - 180, Update the fixture cleanup logic around remote_io._gc_pause_depth and was_enabled so the pytest.fail message identifies whether the failure is a non-zero pause depth or a changed global GC state. Preserve the existing GC state restoration, and report depth only when the remote-read pause remains held.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/changelog.qmd`:
- Line 80: Update the changelog sentence in the HDF5 remote-backend description
to state that automatic garbage collection resumes only after the last such
remote handle closes, while preserving the surrounding behavior and scope.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 170-180: Update the fixture cleanup logic around
remote_io._gc_pause_depth and was_enabled so the pytest.fail message identifies
whether the failure is a non-zero pause depth or a changed global GC state.
Preserve the existing GC state restoration, and report depth only when the
remote-read pause remains held.
In `@tests/test_utils/test_gc_pause.py`:
- Around line 144-168: Update both tests,
test_stranded_cyclic_handle_is_recovered and
test_interrupted_safety_collect_takes_no_pause, to set
remote_io._gc_collect_after through the existing monkeypatch fixture instead of
assigning the module global directly. Add the fixture to the first test and use
monkeypatch.setattr in both tests so the original rate-limit value is restored
after each test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: abd43056-0ac4-4cfc-b325-c17614857850
📒 Files selected for processing (13)
.github/workflows/runtests.ymldascore/constants.pydascore/io/core.pydascore/utils/hdf5.pydascore/utils/io.pydascore/utils/remote_io.pydocs/changelog.qmddocs/tutorial/remote_patches.qmdtests/conftest.pytests/test_io/conftest.pytests/test_io/test_io_core.pytests/test_utils/test_gc_pause.pytests/test_utils/test_io_utils.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_io/test_io_core.py
- .github/workflows/runtests.yml
- dascore/utils/remote_io.py
- test_remote_h5_handle_pauses_gc built a real HTTP filesystem to decide loop-backedness; wasm and free-threaded CPython have no aiohttp, so it silently took the no-pause branch. Stand in a fake async filesystem. - Mark the deadlock property test as concurrency; it starts a thread, which wasm cannot do.
|
✅ Documentation built: |
- Say collection resumes when the last remote handle closes, not the handle. - Restore the safety-valve deadline through monkeypatch. - Report which part of the collection state a test changed.
h5py documents this failure mode and names disabling collection as one of its two mitigations; say so in the tutorial, the changelog, and pause_gc.
# Conflicts: # docs/changelog.qmd
Bot review dispositionAll CodeRabbit and Codex findings on this PR are addressed; every review thread is now resolved. Summary for reviewers, since the branch was rebased and several threads are marked outdated:
Beyond the bots, the branch also went through three internal adversarial reviews (concurrency/lock-ordering, IO semantics/ownership, tests/CI) and a Codex CLI review. Those surfaced several issues the bots did not, the most significant being: |
1 participant
Description
Tracks down the intermittent remote-HTTP deadlock/stall noted in the
test_remote_http.pymodule docstring (the Windows skip rationale). Pinning the suite to 2 CPUs made it reproducible, which separated it into three independent causes.1. Single-threaded fixture servers (test-only)
Two localhost fixture servers (
http_regression_das_path,http_range_das_path) were single-threadedHTTPServers. One slow or abandoned connection parks the accept loop, and every new connection then sits in TCP SYN retry backoff — observed as reproducible 15 s stalls (the SYN retry schedule), 30 s pytest-timeout failures, or an indefinite hang when pytest-timeout isn't installed. All three fixtures now share one threaded-server context manager. Before: 2/3 pinned module runs hit the 30 s timeout; after: 5/5 stable.2. A real deadlock between h5py's lock and garbage collection (product bug)
This is a known failure mode which h5py documents, under Python file-like objects:
fsspec's event loop is exactly that service thread. DASCore reads remote HDF5 through h5py's
fileobjdriver, so each fetch is serviced on the loop thread while the reader holds the global lock; a cyclic collection landing there needs the same lock, and neither side moves.h5py names two mitigations: avoid reference cycles which keep h5py objects alive, or temporarily disable garbage collection. We take the second. The first is not available to a library — the cycle can be anywhere in the process. (h5py's third suggestion, avoiding file-like objects altogether, is what DASCore's existing
ensure_local_filefallback does when streaming is not viable; it costs a full download, which is what the streaming path exists to avoid.)This is platform-agnostic. Windows simply lost the timing coin flip more often, which is why it was previously filed as Windows flakiness. HDF5 handles backed by an async fsspec filesystem (HTTP, S3, ...) now pause automatic collection for the handle's lifetime (
dascore.utils.remote_io.pause_gc) and resume it on close; reference counting still frees non-cyclic garbage. Local paths and synchronous backends such asmemory://are untouched.The pause is balanced across the cases that can strand it: an interrupt anywhere inside the paused region, a
BaseExceptionraised during teardown, a handle inherited through a fork (handles record the pid that paused for them, so a child cannot resume a pause it never took), an interrupt inside aFiberIOread or write, and buffered wrappers that hide the fsspec filesystem from the loop-backed check. A rate-limitedgc.collect()on the next remote open recovers a handle leaked inside a reference cycle, which__del__cannot reach while collection is off.Because this is fixed, the Windows skip is removed and the
network_testsmatrix reuses the shared OS list — Linux, macOS, and Windows now all exercise the localhost-HTTP path. That job iscontinue-on-error, so it reports without gating unrelated changes.Note that the pause is process-global for the lifetime of an open remote handle: cyclic garbage from every thread accumulates until the last one closes, and a handle leaked inside a cycle keeps it paused until the next remote open, if the program makes one. That is a deliberate trade against a hard deadlock. It is documented for users in the remote patches tutorial and for developers on
pause_gc, both of which cite the h5py page above.3. HTTP HDF5 reads overfetched (performance)
Remote HDF5 opens over HTTP amplify transfers: h5py's metadata probe alternates between the file header and footer, and fsspec's default single-window
BytesCacherefetches a multi-MB block on every jump — debug logs showed 18+ MB fetched to probe a 12.7 MB file, and range-less servers re-stream the entire file per miss.H5Reader._get_open_kwargsalready tuned S3 for exactly this reason; HTTP now gets the tuned block size too, withcache_type="blockcache"(a block LRU that keeps both file ends resident, capped at 8 blocks).Trade-off worth knowing:
BlockCacheissues one range request per block, where the default cache coalesces a large contiguous read into one request. That favours the metadata probe this PR is about and costs requests on bulk data reads over HTTP, which is the path DASCore already steers to the local cache.Separately, servers that report no size stream rather than range, so
is_no_range_http_errornow also routes "cannot seek streaming HTTP file" to the same local-file fallback. Such a server therefore materializes a file locally to probe it, which is the behaviour range-less servers already had.Also here
dc.writeof HDF5 to a remote path no longer commits a partial upload when the write fails. A remote HDF5 write buffers into a temp file and uploads it on close, so a mid-write error used to upload whatever had been written; bothIOResourceManager.__exit__and theFiberIOtype caster now abort instead, and share onerelease_handlehelper.close_allisolates per-handle failures so one bad close cannot skip the rest, since remote handles resume the GC pause inclose, and a cleanup failure no longer replaces the error that caused it.Review notes
Taken over from the original branch: rebased onto current
devand reviewed end to end, then put through three internal adversarial reviews plus a Codex review. Fixes those surfaced, beyond the original scope:memory://UPathHDF5 reads took the remote branch and disabled process-wide GC on every read; the pause is now gated onfs.async_impl.get_format(it offers the same object to every FiberIO in turn).__del__closed handles DASCore does not own; it now only releases the pause._type_caster's failure path committed a partial remote HDF5 upload; it aborts now.pause_gccould leave its depth inconsistent when interrupted, which would have silently disabled the fix for the rest of the process.Known and accepted: the GC pause is process-global (see above);
hasattr(handle, "abort")matches only the remote HDF5 writer, so other remote writers still stream directly;_type_castercloses a caller-supplied stream when a read fails, matching what it already did on success.Changelog
remote_hdf5_block_size).Checklist
I have:
docs/contributing/general_guidelines.qmd).I have (if applicable):