config: read resolved config via namespace accessors - #33013
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![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: d69f8d2050
ℹ️ 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".
6c4cb47 to
6e31c47
Compare
d69f8d2 to
f587f57
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f587f57770
ℹ️ 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".
6e31c47 to
0528fc7
Compare
f587f57 to
46dd4c0
Compare
0528fc7 to
1b2b750
Compare
46dd4c0 to
8a1e7f9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a1e7f9ec4
ℹ️ 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".
| is_rank_zero=is_rank_zero, | ||
| skip_tokenizer_init=self.server_args.skip_tokenizer_init, | ||
| metrics_enabled=self.server_args.enable_metrics | ||
| metrics_enabled=get_observability().enable_metrics |
There was a problem hiding this comment.
In the Ray deployment path, SchedulerActor.__init__ calls configure_scheduler_process and then constructs Scheduler directly without the publish(server_args, role="scheduler") performed by run_scheduler_process. Consequently this unconditional namespace read reaches RuntimeContext.config_bag() with no published bags and raises ValueError("config namespace 'observability' not published"), so every non-NPU Ray scheduler actor fails during startup; publish the possibly replaced server_args in the actor before constructing Scheduler.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — real. The Ray scheduler actor now does publish(server_args, role="scheduler") (on its possibly-replaced copy, after configure_scheduler_process) before constructing Scheduler, matching the run_scheduler_process entry. Landed in the role PR of this stack (#33012), where the other process-entry publishes live.
1b2b750 to
4b28cf3
Compare
8a1e7f9 to
353c91c
Compare
4b28cf3 to
9fe3312
Compare
9a93c14 to
7e6dd38
Compare
9fe3312 to
14ea373
Compare
7e6dd38 to
5c69631
Compare
14ea373 to
2a6c692
Compare
5c69631 to
848c607
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3263a04b7
ℹ️ 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".
|
|
||
| def update_active_ranks(self, ranks: ActiveRanksOutput): | ||
| if self.server_args.elastic_ep_backend is not None: | ||
| if get_exec().moe.elastic_ep_backend is not None: |
There was a problem hiding this comment.
When multiple Ray Engines coexist in one process, each RayDataParallelController runs in that shared Engine process (ray/engine.py starts its event loop in a thread), while get_exec() reflects whichever tokenizer most recently published its configuration. If an earlier engine uses elastic EP and a later engine does not, this handler processes the earlier engine's ActiveRanksOutput using the later engine's setting, bypassing the dp_active mask and potentially changing inactive pre-bound worker slots; retain this controller's self.server_args.elastic_ep_backend check.
Useful? React with 👍 / 👎.
Mechanical sweep (AST, alias-aware): get_server_args().FIELD / self.server_args.FIELD / local-alias reads flip to the namespace accessors (get_exec()/get_memory()/...), routed by each field's NS metadata. 628 reads across 160 files. Kept (by rule, not omission): parallel-namespace leaves (their own unit), per-runner fork fields (attention backends / context_length / load_format / skip_tokenizer_init / kv_cache_dtype — resolved values belong to the runner, bags hold intent), per-instance manager files (tokenizer family, multimodal processors — multi-engine reads stay on self.server_args), the resolution pipeline, construction-boundary files, and whole-object / parameter-form reads. check_cuda_graph_backend keeps its stdlib-only module contract (lazy accessor import) and its soft gate: unpublished config still returns False instead of raising.
1 participant
Part 3 of the 3-PR stack (base: #33012). RFC: #30696. Re-lands the reader migration reverted in #32100, regenerated from scratch against current main with the revert's defects fixed at their origin.
get_server_args().FIELD/self.server_args.FIELD/ local-alias reads flip to the namespace accessors (get_exec()/get_memory()/ …), routed by each field's NS metadata — 628 reads across 160 files.self.server_args), the resolution pipeline, construction-boundary files, and parameter-form reads.declare_load_time_overridenow writes the config bags viaget_context().override(namespace readers see the declaration immediately);server_argsstays the pristine startup record. During a draft build the declaration lands on — and is discarded with — the draft's own bags.get_model().kv_cache_dtypebag leaf now always holds the RAW user intent. Resolved values are per-runner (model_runner.kv_cache_dtype_str, fix(attention): read per-runner kv cache dtype off model_runner #32251); the_record_kv_cache_dtypebag write-back is removed and resolved-value consumers (kv/pool configurators via a constructor argument, the PD handshake viakv_args) read the runner's value.get_context().override): mamba pool sizing,update_model_fields, and the adaptive-speculative runtime overrides — including 27 direct*.server_args.speculative_*reads across 13 attention backends and 6 graph runners flipped toget_spec()/get_exec().graph.check_cuda_graph_backendkeeps its stdlib-only module contract (lazy accessor import) and its soft gate: unpublished config still returns False instead of raising.Verification:
test/registered/unitsuite vs the stack base: zero regressions in both directions (failure sets name-identical)./get_server_inforeadback) and a plain dense model.Note: the ~15 unit tests module-skipped during the earlier migration remain skipped here; they are recovered with real publish fixtures in a follow-up PR of this stack (kept out of this diff to keep the mechanical sweep reviewable). This stack merges only after the follow-ups land and full CI is green.
🤖 Generated with Claude Code
CI States
Latest PR Test (Base): 🚫 Run #30668890155
Latest PR Test (Extra): 🚫 Run #30668890010