config: resolution declares, and nothing writes a field - #36618
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f99e81b7c8
ℹ️ 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".
| if getattr(server_args, field.name) is not raw[field.name] | ||
| and getattr(server_args, field.name) != raw[field.name] |
There was a problem hiding this comment.
Detect equal-valued field replacements
When a resolver replaces a caller-supplied mutable with an equal-valued copy (for example, self.lora_paths = list(self.lora_paths)), the is not condition is true but the equality condition is false, so this guard passes even though the record no longer references the operator's original object. This contradicts the test's stated no-rebinding contract and leaves most mutable fields unprotected; treat identity changes as moved fields rather than requiring both identity and value to differ.
Useful? React with 👍 / 👎.
f19507c to
ef10c68
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef10c68ee4
ℹ️ 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".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`run_post_process_pass` refused the published record before running the pass,
so a pass that declares nothing could not run twice. `Engine.shutdown()` leaves
the launch published, and `Engine(server_args=sa)` with the same instance calls
`check_server_args()` again before republishing; `_hisparse_validation` reaches
the runner from there and returns `{}`, so the second launch raised.
The refusal now fires once there is something to record, which is what it was
for: a declaration made after publish is a silent no-op because the stash is
projected at publish and never again. A pass that declares nothing is a
validation, and validations are meant to run on every launch.
Reported by the codex reviewer on #36618. Reproduced: resolve, check, publish,
check again -> `ValueError: run_post_process_pass('_hisparse_validation')
called on the published config`.
`test_a_pass_that_declares_nothing_runs_on_the_published_record` pins both
directions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`run_post_process_pass` refused the published record before running the pass,
so a pass that declares nothing could not run twice. `Engine.shutdown()` leaves
the launch published, and `Engine(server_args=sa)` with the same instance calls
`check_server_args()` again before republishing; `_hisparse_validation` reaches
the runner from there and returns `{}`, so the second launch raised.
The refusal now fires once there is something to record, which is what it was
for: a declaration made after publish is a silent no-op because the stash is
projected at publish and never again. A pass that declares nothing is a
validation, and validations are meant to run on every launch.
Reported by the codex reviewer on #36618. Reproduced: resolve, check, publish,
check again -> `ValueError: run_post_process_pass('_hisparse_validation')
called on the published config`.
`test_a_pass_that_declares_nothing_runs_on_the_published_record` pins both
directions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`run_post_process_pass` refused the published record before running the pass,
so a pass that declares nothing could not run twice. `Engine.shutdown()` leaves
the launch published, and `Engine(server_args=sa)` with the same instance calls
`check_server_args()` again before republishing; `_hisparse_validation` reaches
the runner from there and returns `{}`, so the second launch raised.
The refusal now fires once there is something to record, which is what it was
for: a declaration made after publish is a silent no-op because the stash is
projected at publish and never again. A pass that declares nothing is a
validation, and validations are meant to run on every launch.
Reported by the codex reviewer on #36618. Reproduced: resolve, check, publish,
check again -> `ValueError: run_post_process_pass('_hisparse_validation')
called on the published config`.
`test_a_pass_that_declares_nothing_runs_on_the_published_record` pins both
directions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`run_post_process_pass` refused the published record before running the pass,
so a pass that declares nothing could not run twice. `Engine.shutdown()` leaves
the launch published, and `Engine(server_args=sa)` with the same instance calls
`check_server_args()` again before republishing; `_hisparse_validation` reaches
the runner from there and returns `{}`, so the second launch raised.
The refusal now fires once there is something to record, which is what it was
for: a declaration made after publish is a silent no-op because the stash is
projected at publish and never again. A pass that declares nothing is a
validation, and validations are meant to run on every launch.
Reported by the codex reviewer on #36618. Reproduced: resolve, check, publish,
check again -> `ValueError: run_post_process_pass('_hisparse_validation')
called on the published config`.
`test_a_pass_that_declares_nothing_runs_on_the_published_record` pins both
directions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`run_post_process_pass` refused the published record before running the pass,
so a pass that declares nothing could not run twice. `Engine.shutdown()` leaves
the launch published, and `Engine(server_args=sa)` with the same instance calls
`check_server_args()` again before republishing; `_hisparse_validation` reaches
the runner from there and returns `{}`, so the second launch raised.
The refusal now fires once there is something to record, which is what it was
for: a declaration made after publish is a silent no-op because the stash is
projected at publish and never again. A pass that declares nothing is a
validation, and validations are meant to run on every launch.
Reported by the codex reviewer on #36618. Reproduced: resolve, check, publish,
check again -> `ValueError: run_post_process_pass('_hisparse_validation')
called on the published config`.
`test_a_pass_that_declares_nothing_runs_on_the_published_record` pins both
directions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…36618) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…36618) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Motivation
PR 1 of a five-PR series on top of the raw-input
ServerArgswork (#36250–#36255), based onf775db03aaa. Each builds on the previous one; review them in order.cheng/gc-p1— config: resolution declares, and nothing writes a field ← this PRcheng/gc-p2— config: every handler declares its cuda-graph decisionscheng/gc-p3— config: a parallel leaf with no live counterpart is read barecheng/gc-p4— config: a parallel size has one spelling; a patched scope declares its owncheng/gc-p5— config: the record is not an object that gets passed aroundThey are grouped by how they have to be read, not by topic: PR 3 is 118 files of one mechanical rewrite, reviewed by checking the rule and sampling; PR 4 is the design change that rewrite made possible, and its production files each need reading.
CI for the whole series runs on a separate vehicle PR, whose branch sits one placeholder commit above PR 5: #36623.
The landed stack made
ServerArgshold the operator's input and movedresolution's answers into the declaration stash. Two channels could still write
a field afterwards, and nothing asserted the result. This closes both and
asserts it.
Modifications
``ef10c68ee45
· 5 files (1 production, 3 test, 1 skill doc) · +323 / −37The guard
test_record_holds_the_raw_input.pystates the whole contract in one place:after
resolve_once(), every field still equals the_raw_inputsnapshot takenbefore any handler ran. Fifteen launch shapes.
Two directions, because they are independent and neither catches the other:
and
getattrmust still answer with it;points at the caller's own dict or list, so a handler that reaches into one
changes a value the caller still holds, and a field-by-field comparison
cannot see it (the snapshot stores the reference).
Both were verified by injection: a
setattrinside resolution fails the firstand not the second;
cuda_graph_config.setdefault(...)fails the second and notthe first.
The write-through
run_post_process_passcalled_apply_fieldswhen a pass ran after resolutionfinished. The declaration is already in the stash at that point and publish
projects from the stash, so the write was redundant for the projection and only
served field readers — which the record no longer has. This is the last
production caller. The helper itself stays where it is, with one caller left:
RuntimeContext.override_server_args, the test stand-in, which publishes acontext for tests that need bags. It lives in
arg_groups/, the module themutation ratchet exempts by module, and #36622 explains why closing that last
one is a separate pass.
I first made this an error, and that was wrong:
check_server_argshosts such aslot (
_hisparse_validation), so the error fired in production. Instrumentingrun_post_process_passacross five launch shapes showed that slot is the onlyone that runs after resolution, and it declares nothing. Dropping the write and
keeping the stash append is both correct and smaller.
The registry hole
_a2a_fusion_adjustmentsand_hrm_text_attention_forcewere invoked from aslot but carried no
@register_post_process, so every check that enumeratesPOST_PROCESS_PASSES— the chain-read ratchet, the declaration scan — walkedpast them. Both are registered now, and a static scan of every
run_post_process_pass(sa, fn)call site pins the registry against them.A runtime spy found only the first:
_hrm_text_attention_forceruns for onemodel family, so a fixture that resolves a Llama config never reaches it.
While there: the list's comment claimed to be the end-state execution order.
Measured, it is not —
_hisparse_validationis registered sixteenth and alwaysruns last, because
check_server_argsis a later stage than__post_init__.The comment now says the list is a registry and names the stage split.
The doc
SKILL.mdsaidpublish"snapshots the resolved field values into the configbags". It projects them from the declarations over the raw fields. A reader who
believed the old sentence would look for resolved values on the record.
Accuracy Tests
No model-output change: this series moves where a configuration value is read
from, not what resolution decides. The equivalent check for that claim is a
resolution dump — every field's resolved value for 24 launch shapes (plain, tp2,
tp4_pp2, dp2, EAGLE, NEXTN, page32, page64_chunk2k, cuda-graph knobs,
disaggregation, deterministic, hierarchical cache, symmetric memory, …) — taken
in both trees and compared field by field:
0 differences across 24 shapes × 478 shared fields, against
f775db03aaa.The one field the series has and the base does not is
grpc_worker_threads: onmain it is a public non-field slot assigned in
_handle_deprecated_args, andthis series makes it a declared field. Its value is 4 on both sides.
Every guard also runs at each commit of the series, not only at the head — a
PR that is green only on top of its successors is not reviewable on its own. The
set is the config guards plus every registered test the series touches, ~33 files
per boundary, all green.
No GPU accuracy run. Everything above is CPU-side: resolution, projection and the
guards. A launch-path change that only shows up with real process groups is not
covered by any of it.
Speed Tests and Profiling
No benchmark run, and none is expected to move: nothing here changes a kernel, a
schedule, or the shape of any batch. What changes is the source of a
configuration read — a published dataclass attribute instead of a process-group
getter or an accessor hop.
The one place that could have mattered is
torch.compile: gate helpers readparallel leaves inside compiled forwards, and
object.__getattribute__graph-breaks. That was measured rather than assumed — the reads this series
introduces trace under
torch.compile(fullgraph=True), which is pinned by aregression test.
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ci🤖 Generated with Claude Code
CI States
Latest PR Test (Base): ❌ Run #33110212604
Latest PR Test (Extra): ❌ Run #33110429178
Latest PR Test (AMD ROCm 7.2): ❌ Run #33110212668