Expose the remote HDF5 knobs and announce the gc pause - #879
Merged
Conversation
The HTTP block cap was hard-coded next to a configurable block size, and pausing collection process-wide left no trace a user could connect to a remote read. - Add remote_hdf5_max_blocks; _get_open_kwargs reads it. - Add warn_on_gc_pause, warned once per process when the pause is taken. - Document both, plus the memory they imply, in the remote-patches tutorial and the configuration page.
Contributor
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesRemote HDF5 settings now control block-cache capacity and garbage-collection pause warnings. HTTP HDF5 handles use the configured block limit. Format probing suppresses these warnings. Documentation and tests cover configuration, suppression, fork behavior, and warning errors. Remote HDF5 controls
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
d-chambers
added
ready_for_review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@dascore/utils/remote_io.py`:
- Line 45: Update _reset_gc_pause_state() to reset the module-level
_gc_pause_warned flag to its initial false state after a fork. Add a fork-state
test verifying that a child process has warning state cleared and can emit its
once-per-process warning.
- Around line 60-63: Synchronize the check-and-set of _gc_pause_warned in the
relevant warning function using _gc_pause_lock so only one concurrent caller
claims the warning; keep warnings.warn outside the lock. Add a concurrency test
verifying the warning is emitted at most once per process.
🪄 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: e7778971-f353-417b-9a74-05f4ce845824
📒 Files selected for processing (6)
dascore/config.pydascore/utils/hdf5.pydascore/utils/remote_io.pydocs/tutorial/configuration.qmddocs/tutorial/remote_patches.qmdtests/test_utils/test_gc_pause.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: 3b1f8dcd27
ℹ️ 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 #879 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 177 177
Lines 20615 20638 +23
=========================================
+ Hits 20615 20638 +23
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:
|
- Warn after the pause is accounted for. A filter turning the warning into an error previously raised before the depth moved, and the caller's resume then released a pause it never took, freeing a live handle's. - Claim the warned flag under the lock so two openers cannot both warn. - Rearm the flag after a fork; a pool worker should announce its own pause. - Read both HDF5 cache knobs from one config snapshot.
|
✅ Documentation built: |
- Do not announce the gc pause while probing formats. The warning claimed an HDF5 read before h5py had decided the resource was one, and under warnings-as-errors it landed in _get_format's robustness handler and read as 'wrong format' -- silently skipping the reader which did match. - Validate remote_hdf5_block_size as positive; zero made fsspec stream the whole file, the opposite of what the docs now advise. - Fix the scanning recipe, which told users to spool a remote directory; that raises InvalidSpoolError. - Fork for real in the fork test rather than calling the reset hook, and bound it since the repo sets no global timeout. Drop the unfailable silencing test. Trim repeated prose.
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.
Description
Follow-up to #784, which is now merged, so this sits directly on
dev.#784 introduced two remote-IO behaviours that a user cannot see or influence:
remote_hdf5_block_size, which is configurable. Their product is what an open remote HDF5 handle retains — ~40 MiB by default — so the one number that sets memory use was the one you could not change.gc.isenabled()returning False, has nothing connecting either to a remote read.This adds the missing surface, using the existing configuration machinery rather than a new idiom.
remote_hdf5_max_blocksBlocks one open HTTP HDF5 handle may keep cached; default 8, unchanged from #784.
H5Reader._get_open_kwargsnow reads it instead of hard-coding the value.warn_on_gc_pauseWarned once per process, the first time a pause is taken, mirroring the existing
warn_on_remote_cache. A spool over thousands of remote files says it once. Setwarn_on_gc_pause=Falseto silence it.The warning explains what is paused, why, and that reference counting is unaffected — the three things someone diagnosing memory growth needs.
Documentation
The remote-patches tutorial gains a Tuning Remote HDF5 Transfers section. Rather than just listing the knobs, it gives direction for the two workloads, which pull opposite ways:
BlockCacheissues one request per block (verified infsspec/caching.py: intermediate blocks are fetched individually, with a source comment noting they cannot be coalesced without breaking the LRU). So bigger blocks cut request count, and the LRU earns little on a streaming read — trade blocks for size. It also points out that if you are reading whole files, letting DASCore materialize them locally beats any streaming pattern.max_blocksfirst, because evictions cost nothing for scans and sequential reads.The GC section now mentions the warning and how to turn it off. The configuration page's pointer to remote-IO settings lists the three new names.
Notes
The pause itself stays automatic. It is a correctness requirement rather than a preference — reading remote HDF5 without it deadlocks — so making it opt-in would make the default path the broken one. What is configurable here is the visibility of the pause and the memory of the cache, which are the parts a user might legitimately want to change.
There is no test that the fields are settable; that exercises pydantic and the config machinery rather than DASCore. The behaviour is covered: the warning fires once, can be silenced, and silencing it does not disable the pause.
remote_hdf5_max_blocksis covered throughtest_remote_h5_open_kwargs_are_tuned.Changelog
remote_hdf5_max_blocksconfig option caps the blocks an open remote HDF5 handle caches over HTTP. Together withremote_hdf5_block_sizeit sets the memory one handle retains (~40 MB by default).warn_on_gc_pauseconfig option; DASCore now warns once per process when a remote HDF5 read pauses automatic garbage collection. Set it toFalseto silence.Checklist
I have:
docs/contributing/general_guidelines.qmd).I have (if applicable):
Summary by CodeRabbit
New Features
Documentation