ENH: synchronize remote IO, add free-threaded CI and concurrency docs - #781
Conversation
Final part of the series superseding #763. Remote file cache: - One download lock per cached resource, so unrelated files still download at the same time while callers wanting the same file take turns. The locks are never removed, which lets a cache clear hold all of them without racing new ones into existence. - clear_remote_file_cache holds every download lock, and refuses to run from inside a materialization (thread-local depth guard). - A failed download publishes nothing, so the next caller retries it. - The lru_cache on _materialize_remote_file goes away in favor of the locks; the policy checks move into _download_to_cache. IOResourceManager gets an instance lock so each required type is opened exactly once. Its source property is no longer memoized into the handle cache, where close_all could have closed the source itself. Adds a standalone free-threaded CI job (3.14t, PYTHON_GIL=0, asserting the GIL really is off) which runs the suite on the core dependencies, and documents the concurrency contract the series establishes in the parallelization recipe.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughChangesThe PR adds explicit locking for remote-file caching and I/O concurrency
Possibly related PRs
Suggested labels: 🚥 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 |
coderabbitai
Bot
added
CI
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #781 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 164 164
Lines 17659 17703 +44
=======================================
+ Hits 17657 17701 +44
Misses 2 2
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/test_utils/test_io_utils.py (2)
1264-1276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
finallyonly restores_REMOTE_CACHE_LOCK.
_reinit_remote_cache_locks()also rebinds_REMOTE_KEY_LOCKSand_REMOTE_CACHE_LOCAL, which stay replaced after this test. Harmless today, but it leaks module state into later tests and would mask a depth counter left set by an earlier failure. Restore all three.♻️ Proposed fix
def test_fork_handler_replaces_held_locks(self): """Locks held at fork time are replaced so the child cannot deadlock.""" old_lock = remote_io._REMOTE_CACHE_LOCK + old_key_locks = remote_io._REMOTE_KEY_LOCKS + old_local = remote_io._REMOTE_CACHE_LOCAL try: @@ finally: remote_io._REMOTE_CACHE_LOCK = old_lock + remote_io._REMOTE_KEY_LOCKS = old_key_locks + remote_io._REMOTE_CACHE_LOCAL = old_local🤖 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 1264 - 1276, Update test_fork_handler_replaces_held_locks so its finally block snapshots and restores _REMOTE_CACHE_LOCK, _REMOTE_KEY_LOCKS, and _REMOTE_CACHE_LOCAL, preserving all module state even when assertions fail. Keep the existing lock replacement assertions unchanged.
1210-1223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBarrier party count implicitly depends on
run_in_threads' defaultcount=4.
threading.Barrier(len(resources))only matches becauserun_in_threadsdefaults to 4 threads (tests/conftest.py:114-139). Changing that default turns this into a 30s barrier timeout inside a download. Pass the count explicitly to bind the two together.♻️ Proposed fix
- results = run_in_threads(lambda index: ensure_local_file(resources[index])) + results = run_in_threads( + lambda index: ensure_local_file(resources[index]), count=len(resources) + )🤖 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 1210 - 1223, Update test_distinct_resources_are_not_serialized so run_in_threads receives an explicit worker count matching len(resources), and use the same count for threading.Barrier. Do not rely on run_in_threads’s default count.dascore/utils/remote_io.py (1)
145-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
get_remote_cache_path()already recreates the directory.Per the definition at
dascore/utils/remote_io.py:67-71,get_remote_cache_path()doesmkdir(parents=True, exist_ok=True), so line 159 creates the directory beforermtreeremoves it and line 160 re-creates it. Capturing the path once makes the intent (and the fact that the second call is only there for the mkdir side effect) clearer.♻️ Suggested tidy-up
- shutil.rmtree(get_remote_cache_path(), ignore_errors=True) - get_remote_cache_path().mkdir(parents=True, exist_ok=True) + cache_path = get_remote_cache_path() + shutil.rmtree(cache_path, ignore_errors=True) + cache_path.mkdir(parents=True, exist_ok=True) _REMOTE_RESOURCE_CACHE.clear()🤖 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/remote_io.py` around lines 145 - 161, Update clear_remote_file_cache to capture get_remote_cache_path() once before removing the cache, then pass that captured path to shutil.rmtree and call get_remote_cache_path() only afterward for its existing directory-recreation side effect. Preserve the current locking and cache-clearing behavior.dascore/utils/io.py (1)
278-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSentinel ordering is correct — worth a brief note.
_lockis assigned last in__init__, so its presence implies_cacheexists; that's what makes it a valid sentinel. Adding a short comment on line 225 would keep a future reordering of__init__from silently reintroducing theAttributeErrorin__del__.🤖 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 278 - 280, Add a brief comment near the `_lock` assignment in `__init__` explaining that it is deliberately assigned last and serves as the sentinel confirming `_cache` exists before `__del__` calls `close_all()`. Preserve the existing sentinel check and cleanup behavior.
🤖 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/recipes/parallelization.qmd`:
- Line 38: Update the Spool API references in this recipe to use the fully
qualified dascore.core.spool.BaseSpool form consistently, including map. Add
dascore.utils.remote_io.clear_remote_file_cache to the API-index generation
configuration so the qmd reference resolves.
---
Nitpick comments:
In `@dascore/utils/io.py`:
- Around line 278-280: Add a brief comment near the `_lock` assignment in
`__init__` explaining that it is deliberately assigned last and serves as the
sentinel confirming `_cache` exists before `__del__` calls `close_all()`.
Preserve the existing sentinel check and cleanup behavior.
In `@dascore/utils/remote_io.py`:
- Around line 145-161: Update clear_remote_file_cache to capture
get_remote_cache_path() once before removing the cache, then pass that captured
path to shutil.rmtree and call get_remote_cache_path() only afterward for its
existing directory-recreation side effect. Preserve the current locking and
cache-clearing behavior.
In `@tests/test_utils/test_io_utils.py`:
- Around line 1264-1276: Update test_fork_handler_replaces_held_locks so its
finally block snapshots and restores _REMOTE_CACHE_LOCK, _REMOTE_KEY_LOCKS, and
_REMOTE_CACHE_LOCAL, preserving all module state even when assertions fail. Keep
the existing lock replacement assertions unchanged.
- Around line 1210-1223: Update test_distinct_resources_are_not_serialized so
run_in_threads receives an explicit worker count matching len(resources), and
use the same count for threading.Barrier. Do not rely on run_in_threads’s
default count.
🪄 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: 9b209568-b730-4eb5-8b10-d7784b2cca20
📒 Files selected for processing (5)
.github/workflows/test_free_threaded.ymldascore/utils/io.pydascore/utils/remote_io.pydocs/recipes/parallelization.qmdtests/test_utils/test_io_utils.py
Dropping the lru_cache made every resolution of an already-cached remote file re-hash the id, rebuild the path, take two locks and stat the file: 12.2us -> 35.1us per call, none of which the download lock needs to protect. Keep a dict of published paths, checked before that work and emptied by clear_remote_file_cache, which puts the warm path back at 12.8us. Only successful downloads are recorded, so a failed one is still retried by the next caller.
The materializer and the cached-path probe each built cache_root / sha256(remote_id) / name themselves, so they had to agree by inspection or one would download a file the other could not find. Both now call one helper. Deletes _get_remote_cache_dir, which had no callers. Its "pragma: no cover" is why that went unnoticed: coverage cannot report dead code it has been told to ignore. Also drops the pragma from _annotate_handle_path, which the non-network suite does reach. The remaining pragmas in hdf5.py, chunk_plan.py and io/core.py were checked the same way and are still needed.
|
✅ Documentation built: |
The file mixed dascore.BaseSpool.map with the dascore.core.spool.BaseSpool form used by its other links and elsewhere in the docs. Both resolve, but only one form should appear.
Two adversarial reviews of the previous design both reproduced the same defect: the memo was published outside the download lock, so the fence clear_remote_file_cache put up did not cover the step that mattered. A clear landing in that window left a memo entry pointing at a deleted file, permanently, for the rest of the session. One review also produced a hard deadlock: a download hook re-entering materialization for another resource took the management lock while holding a download lock, which inverts the order clear_remote_file_cache acquires them in. Rather than add a generation counter to defend a guarantee the docs already say is unsupported, this drops the machinery that was buying it: the management lock, the ExitStack over every download lock, the thread-local depth guard, and the hand-rolled memo. What remains is one lock per resource, created with setdefault, plus the lru_cache the base branch used. That is enough for the property this PR is actually for: two threads never download the same file at once, unrelated downloads still run together, and a failed download is retried because lru_cache does not memoize exceptions. Clearing is now documented as unsynchronized, matching both the base branch's behavior and what the concurrency docs already required of callers. Measured: an already-cached remote resolution is 12.0us against the base branch's 12.1us, so the memo the previous commit added is not needed.
1 participant
Description
The last part of the series superseding #763 (parts 5 and 6 of the plan, combined). It synchronizes the remaining IO state, adds a free-threaded CI job, and writes down the contract the whole series establishes.
Follows #772 (per-entry index map), #773 (two-tier config) and #779 (registries, units, catalogs). Once this merges, #763 can be closed as superseded.
Remote file cache
One lock per resource, created with
dict.setdefault(atomic), so two threads never download the same file at once while unrelated downloads still run together. Memoization of resolved paths stays with thelru_cachethe base branch already used, which also means a failed download is not memoized and the next caller retries it.clear_remote_file_cacheclears that cache and the resource registry.clear_remote_file_cacheis documented as unsynchronized: run it while nothing else is reading remote files. That matches what the base branch did and what the concurrency docs already require of callers.An earlier revision of this PR did more, and was wrong. It added a management lock, an
ExitStackacquiring every download lock during a clear, a thread-local materialization depth guard, and a hand-rolled memo. Two adversarial reviews independently reproduced two failures in it:clear_remote_file_cacheacquires them in. That is a hard deadlock, reproduced with both threads stuck.Fixing the first without inverting the lock order needs a generation counter, to defend a guarantee the docs already say is unsupported. Removing the machinery was the better trade. Both failures are gone in the current revision, checked directly.
IO resources
IOResourceManagergets an instanceRLock, so each required type is opened exactly once no matter how many threads ask. The reentrancy is load-bearing:clear_cacheholds the lock and callsclose_all, which re-acquires it. The contract stays "one manager per IO operation": a handle it returns is not itself safe to use from two threads at once, and that is now stated on the class.This also fixes a live bug on
dev, unrelated to threading.sourcewas memoized withcached_method, which stores intoself._cache— the same dictclose_alliterates over calling.close(). Touching.sourceon a manager wrapping an open handle therefore madeclose_all()close the caller's own source. Dropping the memo fixes that; the walk it replaced is twoisinstancechecks, and measures faster than the memo it removed (0.20 µs → 0.06 µs).CI
A standalone
TestFreeThreadedworkflow runs the suite on CPython3.14twithPYTHON_GIL=0, after assertingsys._is_gil_enabled()is actually False so a dependency cannot quietly turn it into an ordinary job. It installs only the core dependencies (not every optional one is built for free-threading yet; their tests skip), reuses the existing test-data cache steps, and takes read-only permissions. The ordinary test matrix is untouched.Docs
docs/recipes/parallelization.qmdgains a "Thread Safety" section covering only what the series actually establishes: which objects threads may share (immutable patches, concurrent spool reads, caller-owned dataframes, read-only coordinate arrays, the synchronized registries and remote cache), which they may not (single-writer state mutation, one open handle per operation, third-party plugin state), and how the two configuration tiers behave across threads. The stale "the GIL means threads won't help" note is updated.Validation
Against
devatd4a21fc6:PYTHON_GIL=0: 7971 passed, 241 skipped, 2 xfailed. That environment runs pandas 3, so it also exercises the copy-on-write path added in ENH: synchronize registries, units, and catalogs for free-threading #779.pre-commit run --all(including actionlint on the new workflow): passed.IOResourceManager.sourceIOResourceManager.get_resource(Path)ensure_local_file(already local)ensure_local_file(remote, already cached)The remote warm path is worth spelling out: an intermediate revision replaced the
lru_cachewith locking alone, which made it 35.1 µs, because every resolution of an already-cached file then re-hashed the id, rebuilt the path, took two locks and stat'ed the file — none of which the download lock needs to protect. Keepinglru_cacheavoids all of it.get_resourcepays one uncontended lock acquisition per required type; it runs once per file open, against an actual open costing tens of microseconds.sourcegot faster because the removed memo cost more than the twoisinstancechecks it was avoiding.New tests are deterministic (barriers and events, no sleeps) and use the in-memory filesystem, so they need no network: concurrent callers download once, distinct resources are proven not to serialize (a shared barrier inside the download would deadlock if they did), a failed download is retried, clearing from inside a materialization raises, and racing
get_resourcecallers share one handle.Notes
set_configtier rather thanconfig_context: a newly started thread begins with a fresh context, so a scoped override does not reach worker threads. This is the same trap noted when ENH: two-tier runtime config (permanent set_config + scoped config_context) #773 landed.codex execis still over quota (until 2026-07-29 16:45), so the counterpart CLI review could not be run for this one either; self-reviewed instead.Changelog
IOResourceManagersynchronizes opening and closing its handles, so concurrent callers share one handle per type; a handle it hands back is still not safe to use from several threads at once.Checklist
I have (if applicable):