ENH: synchronize registries, units, and catalogs for free-threading - #779
Conversation
Replaces the heavyweight synchronization in #763 with one lock per piece of shared mutable state: - _FiberIOManager gets one instance RLock held across a whole load_plugins() call (including the loader), so no caller can see a multi-version format half registered. Cached lookups become invalidatable snapshots (frozenset/tuple) and copy/pickle drops and recreates the lock. - Namespace registration and lazy attachment each get one module/class level RLock; plugin loading and third-party namespace construction stay off the registry lock, and a double check lets a second thread reuse the attached instance. - units.py gets one _UNIT_LOCK around every helper that touches the mutable pint registry; the registry is built exactly once. - PatchCatalog serializes its revision counter and the caches keyed on it; file reads, patch resolution, and directory syncs stay outside the lock. - Shared data becomes caller owned: read-only cached coord arrays, a FrozenDict for coord_shapes, and a copied frame from Spool.get_contents(). Adds _locked and _reinit_after_fork helpers to utils.misc.
|
@codex review |
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds immutable coordinate and catalog-facing snapshots, caller-owned spool dataframes, and synchronization for FiberIO managers, catalogs, Pint registries, and namespaces, including lock recreation after forking and concurrency regression tests. ChangesState ownership and concurrency
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
IO
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #779 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 164 164
Lines 17488 17589 +101
==========================================
+ Hits 17488 17589 +101
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: 5
🧹 Nitpick comments (1)
tests/test_io/test_io_core.py (1)
522-536: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour byte-identical
_runthread harnesses; extract one shared helper (with timeouts).The same barrier-based runner was copy-pasted into four test modules (one variant lacks the
count=4default), so a fix like adding barrier/join timeouts has to be applied four times.
tests/test_io/test_io_core.py#L522-L536: replace with the shared helper (e.g. arun_in_threadsutil intests/conftest or a test utility module) and drop the local copy; addcountdefault for parity.tests/test_io/test_index/test_catalog.py#L317-L331: delete the local_runand call the shared helper.tests/test_units.py#L477-L491: delete the local_runand call the shared helper.tests/test_utils/test_namespace.py#L267-L281: delete the local_runand call the shared helper.🤖 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_io/test_io_core.py` around lines 522 - 536, Extract the duplicated barrier-based _run harness into one shared run_in_threads helper with a count=4 default, barrier timeouts, and thread join timeouts. In tests/test_io/test_io_core.py:522-536, replace the local _run and update callers; in tests/test_io/test_index/test_catalog.py:317-331, tests/test_units.py:477-491, and tests/test_utils/test_namespace.py:267-281, delete each local _run and use the shared helper instead.
🤖 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/core/spool.py`:
- Around line 51-60: Update _copy_public_dataframe so the pandas 2.x branch
treats copy-on-write as enabled only when pd.options.mode.copy_on_write is
exactly True; use a deep copy for "warn" and other non-True values. Add a
regression test covering the "warn" setting where supported and verifying caller
mutations do not alter the cached dataframe.
In `@dascore/io/core.py`:
- Around line 478-508: Update FiberIOManager.load_plugins to avoid holding _lock
while calling _load_entry_point: under the lock, claim/mark requested formats as
in-flight, then release it before importing loaders so callbacks can register
safely. Re-acquire _lock only for register_fiberio, completion status, and
_loaded_formats/_failed_formats updates, preserving concurrent callers’ ability
to distinguish in-flight formats and wait or return appropriately.
In `@dascore/io/index/catalog.py`:
- Around line 597-612: Move the self._syncer.ensure_updated() call out of the
self._revision.lock in the backend property flow, keeping only backend
initialization under that lock. After ensure_updated() completes, reacquire
self._revision.lock before calling self._invalidate(), preserving the existing
backend-sharing and initialization behavior while avoiding a long directory scan
under the revision lock.
In `@dascore/utils/namespace.py`:
- Line 50: Update the return expression in the namespace-building function to
pass strict=True to both the outer zip and the nested zip over package_name and
package_url, preserving the existing FrozenDict structure.
In `@tests/test_utils/test_namespace.py`:
- Around line 302-305: Update the register function to acquire the registry’s
existing lock while copying
_MethodNameSpace._registry["dascore.concurrent_test"] into a snapshot, then
build and return the set from that snapshot after releasing the lock. Preserve
the namespace creation and returned registry contents.
---
Nitpick comments:
In `@tests/test_io/test_io_core.py`:
- Around line 522-536: Extract the duplicated barrier-based _run harness into
one shared run_in_threads helper with a count=4 default, barrier timeouts, and
thread join timeouts. In tests/test_io/test_io_core.py:522-536, replace the
local _run and update callers; in
tests/test_io/test_index/test_catalog.py:317-331, tests/test_units.py:477-491,
and tests/test_utils/test_namespace.py:267-281, delete each local _run and use
the shared helper instead.
🪄 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: 198889ca-efa1-4b0e-b6a5-3d77817f6004
📒 Files selected for processing (18)
dascore/core/coordmanager.pydascore/core/coords.pydascore/core/patch.pydascore/core/spool.pydascore/io/core.pydascore/io/index/catalog.pydascore/units.pydascore/utils/misc.pydascore/utils/namespace.pytests/test_core/test_coord_segmented.pytests/test_core/test_coordmanager.pytests/test_core/test_coords.pytests/test_core/test_spool.pytests/test_io/test_index/test_catalog.pytests/test_io/test_io_core.pytests/test_units.pytests/test_utils/test_misc.pytests/test_utils/test_namespace.py
Refactors: - Collapse the duplicated revision-stamp caches in PatchCatalog into a small _RevisionCache (value plus the revision it was built at). - PatchCatalog.__iter__ is now the single iteration implementation: it snapshots the relation under one lock acquisition, resolves patches outside the lock, and owns the #583 skip warning. Spool.__iter__ delegates to it instead of repeating the loop with per-patch locking. - _FiberIOManager registries are plain dicts, so a missing-key read can no longer register an empty entry. - load_plugins expresses its bookkeeping as one set expression, and regains a memoized no-op for the already-loaded case (its repeat call is on the get_format path). Review (#779): - get_contents: only the literal True enables pandas copy-on-write, so the "warn" setting takes the deep copy again. - _load_plugin_registry zips strictly. - The concurrent registry test snapshots under the registry lock. - The four copies of the thread harness become one run_in_threads fixture, with barrier and join timeouts so a deadlock fails instead of hanging. - Document why the one-time directory scan stays under the revision lock (it serializes the build, and readers have nothing to read until it finishes) while a later update() does not. Adds tests for the fork handlers and both dataframe-copy branches, which were the only uncovered lines in the patch.
coderabbitai
Bot
added
CI
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_io/test_io_core.py (1)
547-552: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the partial-registry regression deterministic.
The auxiliary releaser unblocks the loader immediately after it enters; the barrier does not ensure the other calls have reached
load_pluginswhile loading is stalled. This can pass even if a competing thread bypasses the lock after_loaded_formatsor_all_loadedis stamped early. Instrument and wait for competing load attempts before releasing the loader.🤖 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_io/test_io_core.py` around lines 547 - 552, Update the partial-registry regression around manager.yield_fiberio and run_in_threads to instrument load attempts, wait until the competing threads have entered load_plugins while the primary load remains blocked, then release the auxiliary loader. Ensure the test deterministically exercises concurrent loading before collecting and asserting the results.dascore/io/index/catalog.py (1)
646-691: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
_df_cache/_live_cachesurvive pickling and can serve stale rows against a rebuilt backend.
__getstate__resets_idsforrebuilt_membershipcatalogs specifically because In-memory backends are rebuilt on the other side with FRESH patch ids, so a stored id membership would bind to the wrong rows. The same hazard applies to_df_cache/_live_cache, but they are not cleared here.Since
to_df()returns on a cache hit (Lines 885-887) without ever touchingself.backend, and_CatalogRevision.valueis expected to survive pickling unchanged (only the lock is excluded/recreated per the diff summary), an unpickled catalog whose cache was warmed before pickling will return the pre-pickleDataFrame/live tuple as-is — potentially before the backend is even rebuilt, and with_patch_idvalues that don't match the freshly-rebuilt backend's row ids.union()(Lines 552-554) is a concrete internal consumer that truststo_df()["_patch_id"]to matchcatalog.backend's current rows, so this can silently return wrong/missing records. This is reachable via the documentedSpool.mapper-task pickling path onceget_contents()/iteration has realized the relation before pickling.🐛 Proposed fix
def __getstate__(self) -> dict: state = dict(self.__dict__) state["_backend"] = None + # Cached rows are keyed to *this* backend's row ids. A rebuilt + # in-memory backend on the receiving side gets fresh ids, so a + # stale cache must not cross the pickle boundary. + state["_df_cache"] = _RevisionCache() + state["_live_cache"] = _RevisionCache()🤖 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/io/index/catalog.py` around lines 646 - 691, Update __getstate__ to invalidate _df_cache and _live_cache whenever pickling can rebuild or replace the backend, especially for rebuilt_membership catalogs; ensure the unpickled catalog cannot serve cached rows or live tuples with stale patch IDs before backend reconstruction. Preserve caches only when the backend and its row identity remain valid, such as the syncer case.
🧹 Nitpick comments (1)
tests/conftest.py (1)
113-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove remaining module-local copies of this fixture.
The supplied context still shows duplicate
run_in_threadsfixtures intests/test_utils/test_namespace.py,tests/test_io/test_io_core.py,tests/test_io/test_index/test_catalog.py, andtests/test_units.py. Those definitions shadow this shared fixture, so future fixes will not apply consistently.🤖 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 113 - 141, Remove the module-local run_in_threads fixture definitions from tests/test_utils/test_namespace.py, tests/test_io/test_io_core.py, tests/test_io/test_index/test_catalog.py, and tests/test_units.py, leaving tests/conftest.py::run_in_threads as the shared fixture used by those tests.
🤖 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 `@tests/conftest.py`:
- Around line 127-129: Update the worker function in _run() to catch exceptions
from func(index) and store each exception per worker; after all threads join,
re-raise the captured exception in the test thread before returning results,
while preserving successful result collection.
---
Outside diff comments:
In `@dascore/io/index/catalog.py`:
- Around line 646-691: Update __getstate__ to invalidate _df_cache and
_live_cache whenever pickling can rebuild or replace the backend, especially for
rebuilt_membership catalogs; ensure the unpickled catalog cannot serve cached
rows or live tuples with stale patch IDs before backend reconstruction. Preserve
caches only when the backend and its row identity remain valid, such as the
syncer case.
In `@tests/test_io/test_io_core.py`:
- Around line 547-552: Update the partial-registry regression around
manager.yield_fiberio and run_in_threads to instrument load attempts, wait until
the competing threads have entered load_plugins while the primary load remains
blocked, then release the auxiliary loader. Ensure the test deterministically
exercises concurrent loading before collecting and asserting the results.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 113-141: Remove the module-local run_in_threads fixture
definitions from tests/test_utils/test_namespace.py,
tests/test_io/test_io_core.py, tests/test_io/test_index/test_catalog.py, and
tests/test_units.py, leaving tests/conftest.py::run_in_threads as the shared
fixture used by those tests.
🪄 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: 426a0cc6-7d6f-4bf0-9b9e-adf9ed7a8231
📒 Files selected for processing (10)
dascore/core/spool.pydascore/io/core.pydascore/io/index/catalog.pydascore/utils/namespace.pytests/conftest.pytests/test_core/test_spool.pytests/test_io/test_index/test_catalog.pytests/test_io/test_io_core.pytests/test_units.pytests/test_utils/test_namespace.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_utils/test_namespace.py
- tests/test_io/test_index/test_catalog.py
- dascore/utils/namespace.py
- dascore/io/core.py
Reading pd.options.mode.copy_on_write emits a deprecation warning on every access under pandas 3, where copy-on-write can no longer be disabled. Settle that at import from the version and only consult the option on pandas 2, so get_contents() stays quiet on pandas 3 and the helper keeps a single, always-executed body (the previous version branch was the one line codecov reported as uncovered). The copy-mode test is now parametrized over every setting pandas 2 accepts and skips where the option no longer applies.
|
✅ Documentation built: |
1 participant
Description
Part 3+4 of the series superseding #763. This makes the remaining shared mutable state safe under free-threaded CPython, using one lock per piece of state rather than #763's claim/wait event graph and keyed-lock tables.
Base: current
dev(#776). Follows #772 (per-entry index map) and #773 (two-tier config).Registries
_FiberIOManagergets one instanceRLock, held for a wholeload_plugins()call including the loader invocation. A format is only marked loaded once all of its entry points are registered, so a caller can never see a multi-version format half registered (previously a second thread could observe onlyV1of a two-version format and use the older reader)._get_prioritized_list,_get_fiber_io_by_input_type) move fromfunctools.cache/cached_methodinto an invalidatable_lookup_cachecleared byregister_fiberio, and publish immutable snapshots (tuple/frozenset). This also fixes a latent staleness bug: aFiberIOregistered after the first prioritized-list build was previously never seen.known_formatsreturns afrozenset;_yield_format_version/_yield_extensionssnapshot under the lock rather than reading shared state between yields.__getstate__and recreates it in__setstate__._load_plugin_registryreturns aFrozenDict.Documented cost: plugin loading is now serialized, and it runs with the manager lock held. A thread importing a module that defines a
FiberIOtherefore waits for any in-progress load to finish. Serializing a once-per-format step was preferred over the in-flight tracking (events, wait-for cycle detection,__getstate__pruning) that #763 needed to avoid it.Units
dascore/units.pygets one_UNIT_LOCKaround every helper that touches the mutable pint registry, and the registry is built exactly once behind that lock (replacing@cacheonget_registry). Each of those helpers is itself cached, so the lock is only taken on a cache miss.Catalog
PatchCatalogserializes its revision counter and the caches keyed on it (backendbootstrap,to_df,__len__,get_patch,add/remove,close). Kept outside the lock: patch resolution/reads, and the long directory scan inupdate()(only its cache invalidation is locked). The two revision-stamped caches share one small_RevisionCachehelper, and_cold_live_values()documents that callers hold the lock instead of re-acquiring it. The revision object drops/recreates its lock on pickle.PatchCatalog.__iter__becomes the single iteration implementation: it snapshots the relation under one lock acquisition, resolves patches outside the lock, and owns the #583 skip warning.Spool.__iter__delegates to it rather than repeating the loop with a lock per patch (this is what makes the memory-spool benchmark faster than the base).Caller-owned shared data
Deferred out of #773 because it is not config:
CoordRange.valuessingle-sample branch,CoordSegmented._segment_offsets) are now read-only like their siblings.CoordManager.coord_shapesreturns aFrozenDict(Patch.coord_shapestypedMapping).Spool.get_contents()returns a caller-owned frame. Previously it handed back the catalog's cached relation, so mutating the returned dataframe corrupted the spool.Fork safety
utils.miscgains_locked(lock_name)and_reinit_after_fork(func). The module-level locks (units, namespaces) and theFiberIO.managerlock are reinstalled in forked children, matching the config lock added in #773.Validation
Run against
devat1183dbcc:PYTHON_GIL=0: 7966 passed, 238 skipped, 2 xfailed.PYTHON_GIL=1on the free-threading build: 4594 passed.pytest dascore --doctest-modules): 144 passed.pre-commit run --all: passed.Benchmark gates, best of 200 runs, this branch vs the exact
devbase:test_get_formatTestMemorySpoolBenchmarks::test_spool_from_patches_accessThe memory-spool benchmark ends up faster than the base:
Spool.__iter__used tocall
catalog.get_patch(i)once per patch, and now the catalog snapshots therelation once, so the added locking costs one acquisition per iteration instead of
one per patch.
get_formatis dominated by disk IO, so the gate benchmark cannot resolve smallchanges. Micro-benchmarks of the manager hot paths (no IO), best of 9 x 2000 calls:
load_plugins()(already loaded)load_plugins("DASDAE")yield_fiberio(extension="h5")yield_fiberio("DASDAE", "1")The one regression left is the extension path, +0.5 µs from two lock acquisitions
and one defensive tuple copy of the extension list.
Notes
codex execis over quota until 2026-07-29 16:45, so the counterpart CLI review could not be run; this was self-reviewed instead. CodeRabbit's review has been addressed (three fixes, two declined with reasoning on the threads).defaultdictregistries with plain dicts (a missing-key read used to register an empty entry), and simplifiedload_plugins' bookkeeping.docs/recipes/parallelization.qmd) and the free-threaded CI job come in the last PR of the series, which will also close ENH: freeze the shared entry-point loader mapping #763.Changelog
Checklist
I have (if applicable):
Summary by CodeRabbit
get_contents()now returns a caller-owned dataframe copy, so mutations no longer affect subsequent results.