ENH: freeze the shared entry-point loader mapping - #763
Conversation
d-chambers
added
the
ready_for_review
ChatGPT Codex Connector
|
@codex review |
|
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:
📝 WalkthroughWalkthroughThe pull request adds context-local configuration, immutable cached values, lock-based coordination across registries, IO managers, remote caches, and catalogs, SQLite-backed index-map persistence, concurrency tests, and free-threaded Python CI coverage. ChangesConcurrency and storage updates
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
![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: 351c84c996
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
dascore/utils/remote_io.py (1)
177-187: 🚀 Performance & Scalability | 🔵 TrivialGlobal cache lock is held while blocking on a per-key download (same-key contention).
In
_materialize_remote_file,state.lock.acquire()runs inside thewith _REMOTE_CACHE_LOCKblock. When two threads request the same key, the second thread blocks onstate.lockwhile still holding the global_REMOTE_CACHE_LOCK; because the holder only releasesstate.lockafter its (potentially long) download completes, other threads cannot even begin materializing different keys during that window. Lock ordering and correctness are sound, but this serializes unrelated remote downloads under same-key contention, which partially undercuts the free-threading goal. Consider a condition-variable / "leader downloads, followers await" pattern that releases the management lock before waiting.🤖 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 177 - 187, Update _materialize_remote_file so it never blocks on state.lock while holding _REMOTE_CACHE_LOCK. Keep management-lock bookkeeping and per-key state setup inside the global lock, then release it before waiting for an existing same-key download; use a condition-variable or equivalent leader/follower coordination so followers await completion while unrelated keys can proceed concurrently, preserving the current cache and cleanup correctness..github/workflows/runtests.yml (2)
125-232: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNew
free_threadjob has no explicitpermissions:block.Static analysis flags this as running with default (potentially broad) token permissions. Add a job-level
permissions:block scoped to what's actually needed (this job doesn't appear to write anything besides Codecov upload).🔒 Proposed fix
free_thread: name: free_thread timeout-minutes: 60 runs-on: ubuntu-latest + permissions: + contents: read🤖 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 @.github/workflows/runtests.yml around lines 125 - 232, Add a job-level permissions block to free_thread with the minimum required scope, keeping repository contents read-only and granting only the permission needed by the Codecov upload. Place it alongside the job configuration and avoid inheriting broad default token permissions.Source: Linters/SAST tools
139-142: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
actions/checkoutdoesn't disable credential persistence.Static analysis flags this checkout step for not setting
persist-credentials: false, which leaves the GitHub token in the local git config for later steps (including third-party actions) to potentially exfiltrate.🔒 Proposed fix
- uses: actions/checkout@v4 with: fetch-tags: 'true' fetch-depth: '0' + persist-credentials: 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 @.github/workflows/runtests.yml around lines 139 - 142, Update the actions/checkout step to explicitly set persist-credentials to false while preserving the existing fetch-tags and fetch-depth settings.Source: Linters/SAST tools
dascore/io/index/catalog.py (1)
585-599: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
self._syncer.ensure_updated()runs on every.backendaccess while holding the shared revision lock.Since
_view()shares_revisionacross all views of a root catalog, and every mutation/read path (to_df,add,update,remove,order_by,_ordered_ids) funnels through thisbackendproperty, any directory-sync cost incurred byensure_updated()now serializes all concurrent catalog operations on every access, not just the first. Consider gating the sync check so it only runs when actually needed (e.g., a dedicated "last-checked" revision/timestamp) or decoupling it from the bootstrap lock so cheap reads aren't blocked behind a full directory rescan.🤖 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 585 - 599, The backend property currently calls self._syncer.ensure_updated() on every access while holding the shared self._revision lock, serializing all catalog operations behind directory rescans. Update the backend initialization/synchronization flow around self._backend and ensure_updated() to gate or decouple sync checks so unchanged catalogs avoid repeated rescans and cheap reads do not remain blocked by synchronization, while preserving invalidation when an update is detected.
🤖 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/io/index/catalog.py`:
- Around line 941-972: Protect all shared-state read paths with
self._revision.lock, including _cold_live_values(), __len__(), get_patch(), and
__iter__(). Ensure cache value/revision pairs and resolver.live_entries()
iteration are read while holding the lock, relying on the RLock for safe nesting
with existing locked mutators and to_df/backend calls.
In `@dascore/utils/namespace.py`:
- Line 79: Update the nested zip calls in the namespace-building return
expression to pass strict=True, preserving the existing FrozenDict structure and
documenting that the DataFrame-derived columns must have matching lengths.
---
Nitpick comments:
In @.github/workflows/runtests.yml:
- Around line 125-232: Add a job-level permissions block to free_thread with the
minimum required scope, keeping repository contents read-only and granting only
the permission needed by the Codecov upload. Place it alongside the job
configuration and avoid inheriting broad default token permissions.
- Around line 139-142: Update the actions/checkout step to explicitly set
persist-credentials to false while preserving the existing fetch-tags and
fetch-depth settings.
In `@dascore/io/index/catalog.py`:
- Around line 585-599: The backend property currently calls
self._syncer.ensure_updated() on every access while holding the shared
self._revision lock, serializing all catalog operations behind directory
rescans. Update the backend initialization/synchronization flow around
self._backend and ensure_updated() to gate or decouple sync checks so unchanged
catalogs avoid repeated rescans and cheap reads do not remain blocked by
synchronization, while preserving invalidation when an update is detected.
In `@dascore/utils/remote_io.py`:
- Around line 177-187: Update _materialize_remote_file so it never blocks on
state.lock while holding _REMOTE_CACHE_LOCK. Keep management-lock bookkeeping
and per-key state setup inside the global lock, then release it before waiting
for an existing same-key download; use a condition-variable or equivalent
leader/follower coordination so followers await completion while unrelated keys
can proceed concurrently, preserving the current cache and cleanup correctness.
🪄 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
Run ID: 22339d0d-fee1-4773-b029-3a2cba918c64
📒 Files selected for processing (26)
.github/workflows/runtests.ymldascore/config.pydascore/core/coordmanager.pydascore/core/coords.pydascore/core/patch.pydascore/core/spool.pydascore/io/core.pydascore/io/index/catalog.pydascore/io/index/indexer.pydascore/units.pydascore/utils/io.pydascore/utils/namespace.pydascore/utils/plugins.pydascore/utils/remote_io.pydocs/recipes/parallelization.qmdtests/conftest.pytests/test_core/test_coord_segmented.pytests/test_core/test_coords.pytests/test_core/test_spool.pytests/test_io/test_index/test_index_edge_cases.pytests/test_io/test_indexer.pytests/test_io/test_io_core.pytests/test_utils/test_config.pytests/test_utils/test_io_utils.pytests/test_utils/test_namespace.pytests/test_utils/test_plugins.py
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test_io/test_index/test_catalog.py`:
- Line 6: Alias the imported concurrent.futures TimeoutError to a distinct name
in test_catalog.py, then update the affected assertion around lines 90–91 to
reference that alias while preserving the existing timeout 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
Run ID: b552a808-747a-4949-88cd-96ec42421686
📒 Files selected for processing (9)
.github/workflows/runtests.ymldascore/io/index/catalog.pydascore/io/index/indexer.pydascore/utils/namespace.pydascore/utils/remote_io.pytests/test_io/test_index/test_catalog.pytests/test_io/test_indexer.pytests/test_utils/test_config.pytests/test_utils/test_io_utils.py
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/runtests.yml
- tests/test_utils/test_config.py
- tests/test_io/test_indexer.py
- dascore/io/index/indexer.py
- dascore/io/index/catalog.py
- tests/test_utils/test_io_utils.py
- dascore/utils/namespace.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f8e53db7c
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2710ad86c5
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/io/core.py`:
- Line 468: Rename the format parameter in the surrounding function to a
non-shadowing name to satisfy Ruff A001, and update all references within that
function, including the uppercasing assignment and any downstream format checks
or calls. Preserve the existing behavior when the value is None or provided.
🪄 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
Run ID: 98908d2c-1f36-489b-827b-2ee725d0caa9
📒 Files selected for processing (4)
dascore/io/core.pydascore/io/index/indexer.pytests/test_io/test_indexer.pytests/test_io/test_io_core.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_io/test_io_core.py
- dascore/io/index/indexer.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4eddc22cd9
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e9d657e8c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e9d657e8c
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test_io/test_io_core.py`:
- Around line 591-596: Update the load worker’s exception handler in load to
catch Exception instead of BaseException, while preserving the existing
errors_lock synchronization and errors.append 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
Run ID: 98df60fe-8b5a-4681-834c-ada1aa3b28d7
📒 Files selected for processing (10)
dascore/config.pydascore/io/core.pydascore/io/index/indexer.pydascore/utils/plugins.pydascore/utils/remote_io.pytests/test_io/test_indexer.pytests/test_io/test_io_core.pytests/test_utils/test_config.pytests/test_utils/test_io_utils.pytests/test_utils/test_plugins.py
🚧 Files skipped from review as they are similar to previous changes (7)
- dascore/utils/remote_io.py
- dascore/utils/plugins.py
- dascore/config.py
- dascore/io/core.py
- dascore/io/index/indexer.py
- tests/test_utils/test_io_utils.py
- tests/test_io/test_indexer.py
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7a763265e
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fc5d3cffa
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4257bb96c
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2930a45 to
3d21025
Compare
d-chambers
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #763 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 164 164
Lines 17713 17819 +106
==========================================
+ Hits 17713 17819 +106
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:
|
3d21025 to
4edd9b6
Compare
Return a FrozenDict from get_entry_point_loaders so the one mapping every caller shares cannot be mutated, and document why the plugin caches stay unsynchronized: entry_point.load() imports the plugin, and CPython runs a module body exactly once behind its per-module import lock.
d-chambers
changed the title
4edd9b6 to
0da55a5
Compare
1 participant
Description
Last remnant of the original free-threading PR. Everything else landed through the PRs this was split into — #772, #773, #778, #779, #781, #785, #786 — and the merged versions are newer and more refined, so this branch was rebased onto
devand rescoped rather than replayed.The one piece never split out was deduplicating concurrent plugin entry-point loading, via a synchronized cache that elected one loader per key plus a wait graph that turned mutual dependencies into a
PluginLoadCycleError. That has been dropped. Its premise was that "loading a plugin imports third-party code, so running the loader more than once is not merely wasted work" — butentry_point.load()is an import, and CPython runs a module body exactly once behind its per-module import lock. Racing callers block on that lock and resolve the same already-registered class, on the free-threaded build as much as the default one. So the 195 lines of lock choreography and 500 lines of tests bought a savedentry_points()scan, and reintroduced exactly the claim/wait graph #779 deliberately declined to build in favor of holding the manager lock across plugin imports. Neither production caller needed it either:_FiberIOManager._epsonly stores uncalled boundloadmethods and does its real loading under the managerRLock, leaving_MethodNameSpace.__getattr__as the only racing path.What is left is the part that survives that argument:
get_entry_point_loadersreturns aFrozenDict. It isfunctools.cache'd, so every caller shares one mapping; handing out a mutable dict is a hazard regardless of threads. This matchesget_registered_namespaces, which already returns aFrozenDict.sys.modulesand retried by the next caller, which is the behavior we want anyway.Net effect on the branch: +701/-5 across three files becomes +96/-2 across two, and most of what remains is the test.
dascore/exceptions.pyis untouched again.Checklist
I have (if applicable):