ENH: two-tier runtime config (permanent set_config + scoped config_context) - #773
Conversation
…ntext) Split the runtime configuration API into two clear tiers so it is safe and predictable under free-threaded execution: - set_config(...) now sets the process-wide base permanently (visible from every thread and task) and returns the new config; it is no longer a context manager. - config_context(...) is a new context manager for temporary, thread/task- local overrides. It stores the override in a ContextVar, so concurrent overrides in different threads are isolated and never clobber one another, and it restores on block exit. - get_config() returns the ContextVar override when set, else the global base. Spool.map(...) captures the config active at the call and re-applies it in each worker via config_context, so both thread- and process-pool workers see the caller's config (process workers previously saw defaults). Migrated all internal `with set_config(...)` usages to config_context, and kept broad-scoped test fixtures (session/module) on the permanent base so their overrides remain visible to worker threads and forked processes.
…, wrapper - set_config/reset_config now serialize the global read-modify-write under a module lock so concurrent updates cannot lose fields or return another thread's config. - benchmarks/test_io_benchmarks.py module fixture uses permanent save/set/ restore instead of `with set_config(...)` (no longer a context manager). - config_context docstring: thread inheritance is runtime-dependent (sys.flags.thread_inherit_context), and an inherited copy is not undone on block exit. - _MapFuncWrapper takes config as a required arg (its only caller always supplies it), strengthening the call-time binding invariant.
📝 WalkthroughWalkthroughChangesRuntime configuration model
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
the
documentation
![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: 9e601e2513
ℹ️ 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".
|
|
||
|
|
||
| @contextmanager | ||
| def config_context(new_config: DascoreConfig | None = None, **kwargs): |
There was a problem hiding this comment.
Because this newly exported public function has no return annotation, type checkers cannot infer that with config_context(...) as config binds a DascoreConfig; annotate the generator return type, such as Iterator[DascoreConfig], to preserve typing for the new public API. .agents/agents.mdL75-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 805ec70 — config_context is now annotated -> Iterator[DascoreConfig].
| # overrides from `config_context(...)` live in a ContextVar so concurrent | ||
| # blocks stay isolated per thread/task and never clobber one another. | ||
| _GLOBAL_CONFIG: DascoreConfig = DascoreConfig() | ||
| _GLOBAL_CONFIG_LOCK = Lock() |
There was a problem hiding this comment.
On supported POSIX Python versions where a process pool uses fork, if a worker is forked while another thread is inside set_config() or reset_config(), it inherits _GLOBAL_CONFIG_LOCK in the locked state with no surviving owner; any subsequent config mutation in that worker then hangs indefinitely. Register an after-fork handler that replaces this lock in the child, or use a synchronization design that cannot carry a held thread lock across a fork.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. 805ec70 registers os.register_at_fork(after_in_child=_reinit_config_lock), which rebinds _GLOBAL_CONFIG_LOCK to a fresh Lock in the child, so an inherited held lock can no longer wedge config changes there. _GLOBAL_CONFIG itself needs nothing: the rebind in set_config is a single atomic store, so a forked child sees either the old or the new config, never a partial one. Covered by test_fork_handler_replaces_held_lock.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #773 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17356 17373 +17
=========================================
+ Hits 17356 17373 +17
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 (1)
dascore/config.py (1)
182-195: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject unknown config kwargs in
set_config/config_context.
DascoreConfigdoes not setextra="forbid", and Pydantic v2’s default isextra="ignore", so_build_config()can swallow typo’d overrides likedc.set_config(dispplay_float_precision=5)instead of raising. Addextra="forbid"toDascoreConfig’smodel_configand/or validate kwargs against allowed fields before applying them.🤖 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/config.py` around lines 182 - 195, Update DascoreConfig’s model_config to forbid extra fields, ensuring unknown keyword overrides passed through _build_config (including set_config/config_context) raise validation errors instead of being ignored. Preserve existing valid field overrides and full DascoreConfig replacement 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 `@dascore/io/dasdae/_compat.py`:
- Line 67: Update the error guidance string in the compatibility handling code
to show the activated context-manager syntax, including the trailing colon: with
dc.config_context(allow_dasdae_format_unpickle=True):. Preserve the existing
option name and surrounding guidance.
---
Nitpick comments:
In `@dascore/config.py`:
- Around line 182-195: Update DascoreConfig’s model_config to forbid extra
fields, ensuring unknown keyword overrides passed through _build_config
(including set_config/config_context) raise validation errors instead of being
ignored. Preserve existing valid field overrides and full DascoreConfig
replacement 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: b5887e2f-5002-4f07-8acc-2eb06c64d09f
📒 Files selected for processing (26)
benchmarks/test_io_benchmarks.pydascore/__init__.pydascore/config.pydascore/examples.pydascore/io/dasdae/_compat.pydascore/utils/misc.pydocs/changelog.qmddocs/tutorial/configuration.qmddocs/tutorial/patch.qmddocs/tutorial/remote_patches.qmdtests/conftest.pytests/test_io/test_dasdae/test_dasdae.pytests/test_io/test_index/test_index_edge_cases.pytests/test_io/test_index/test_plan.pytests/test_io/test_indexer.pytests/test_io/test_io_core.pytests/test_io/test_remote_common_io.pytests/test_io/test_remote_http.pytests/test_io/test_remote_memory.pytests/test_proc/test_rolling.pytests/test_utils/test_config.pytests/test_utils/test_display.pytests/test_utils/test_downloader.pytests/test_utils/test_io_utils.pytests/test_utils/test_patch_utils.pytests/test_utils/test_progress.py
|
✅ Documentation built: |
- Set extra="forbid" on DascoreConfig so misspelled overrides raise instead of being silently dropped. - Reinstall the config lock after a fork so a lock held by another thread at fork time cannot deadlock config changes in the child. - Annotate config_context's generator return type. - Show the full with-statement (and the permanent alternative) in the legacy DASDAE unpickle error message. - Document the two tiers in the configuration tutorial. - Expose the conftest permanent-config helper as a fixture so the remote common-IO module fixture stops hand-rolling save/restore, and have reset_config delegate to set_config.
|
Addressed the second review round in 805ec70:
Local validation: full suite 8166 passed / 89 skipped / 2 xfailed; doctests for |
1 participant
Description
Second extraction from the free-threaded work (follows #772). It splits the runtime configuration API into two clear tiers so config is safe and predictable under free-threaded (GIL-disabled) execution, and makes
Spool.mappropagate config to workers.This supersedes the config portion of the integration PR #763 with a much smaller surface: no generator/async decorator machinery, just a global base plus a
ContextVaroverride.What changed
set_config(...)now sets the process-wide base permanently (visible from every thread and task) and returns the new config. It is no longer a context manager. The read-modify-write is serialized under a lock so concurrent updates can't lose fields.config_context(...)is a new context manager for temporary, thread/task-local overrides. The override lives in aContextVar, so concurrent overrides in different threads are isolated and never clobber one another, and it restores on block exit.get_config()returns theContextVaroverride when set, else the global base.Spool.map(...)captures the config active at the call and re-applies it in each worker (threads, and pickled into processes), so workers observe the caller's config. Previously process-pool workers silently saw defaults — this is strictly better than the old global singleton.Migration
with dc.set_config(...)→with dc.config_context(...). All internal usages, docs, and tests are migrated. Broad-scoped test/benchmark fixtures (session/module) intentionally stay on the permanent tier so their overrides remain visible to worker threads and forked processes — a module-scopedconfig_contextwould leak itsContextVaroverride across modules.Validation
test_config.pycovers: permanent visibility across threads, scoped restore/stacking, concurrent scoped overrides staying isolated, andSpool.mapseeing call-time config under both thread and process pools.config.pydoctests pass; project pre-commit clean on changed files.with set_config; runtime-dependent thread-inheritance docs; making the map wrapper's config required) were all addressed.extra="forbid"), the global config lock is reinstalled after a fork so a lock held at fork time cannot deadlock a child,config_contextis annotated-> Iterator[DascoreConfig], and the legacy-DASDAE error message shows the fullwithstatement plus the permanent alternative.Part of the free-threading work; supersedes the config parts of #763.
Changelog
dc.set_config(...)sets the process-wide base permanently and returns the new config, anddc.config_context(...)applies thread- and task-local overrides that restore on exit.Spool.map(...)re-applies the caller's config in each worker, and an unknown field name raises.Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
config_contextfor temporary, thread- and task-local configuration overrides.Bug Fixes
Documentation