From bd4bb1781a95e8298546feab69122897e0aaec1d Mon Sep 17 00:00:00 2001 From: Cheng Wan <54331508+ch-wan@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:53:17 -0700 Subject: [PATCH] config: resolution declares, and nothing writes a field (#36618) Co-authored-by: Claude Opus 5 --- .../skills/sglang-runtime-context/SKILL.md | 19 +-- python/sglang/srt/arg_groups/overrides.py | 48 ++++-- .../test_record_holds_the_raw_input.py | 161 ++++++++++++++++++ .../unit/test_chain_read_ratchet.py | 81 +++++++++ test/registered/unit/test_model_overrides.py | 24 +-- 5 files changed, 296 insertions(+), 37 deletions(-) create mode 100644 test/registered/unit/server_args/test_record_holds_the_raw_input.py diff --git a/.claude/skills/sglang-runtime-context/SKILL.md b/.claude/skills/sglang-runtime-context/SKILL.md index b4e996fb7..0964e2dae 100644 --- a/.claude/skills/sglang-runtime-context/SKILL.md +++ b/.claude/skills/sglang-runtime-context/SKILL.md @@ -11,7 +11,7 @@ One container owns process-static runtime state: `sglang.srt.runtime_context.Run | Tier | Accessor | Holds | Lifecycle | |------|----------|-------|-----------| | raw config seed | `get_server_args()` | the published `ServerArgs` — the startup record, for debugging, dumps and provenance. **Business code does not read fields off it**: the read ratchet pins that at zero, and "Reading config: the seed is off limits" below says what to read instead, which forms the ratchet sees, and what is outside it by construction (a runtime-computed name; a whole-object hand-off) | published at process entry; re-publish is **last-publish-wins** (the tokenizer publish in the launcher process; sequential engine rebuild in one process, e.g. unit tests) and re-projects the bags; read-only | -| resolved config | `get_exec()` `get_memory()` `get_schedule()` `get_model()` `get_spec()` `get_serving()` `get_observability()` `get_disagg()` `get_lora()` `get_mm()` `get_device()` | namespace **config bags** — the single source of truth for resolved config; leaves are real attributes (dynamo-traceable). Each is a **module function of no arguments**, and a module binds the name once: `manager.get_disagg()`, `self.get_disagg = get_disagg`, or a same-named import next to the bag one (`from model_loader import get_model`) all import fine and fail only when that path runs. `ruff --select F811` catches the import collision; `RuntimeContext` has no bag-named member and no `__getattr__`, so the member-call shapes are an `AttributeError` at call time — give it a delegating `__getattr__` and they go silent instead | projected from `server_args` at `publish`; mutated only via `get_context().override` | +| resolved config | `get_exec()` `get_memory()` `get_schedule()` `get_model()` `get_spec()` `get_serving()` `get_observability()` `get_disagg()` `get_lora()` `get_mm()` `get_device()` | namespace **config bags** — the single source of truth for resolved config; leaves are real attributes (dynamo-traceable). Each is a **module function of no arguments**, and a module binds the name once: `manager.get_disagg()`, `self.get_disagg = get_disagg`, or a same-named import next to the bag one (`from model_loader import get_model`) all import fine and fail only when that path runs. `ruff --select F811` catches the import collision; `RuntimeContext` has no bag-named member and no `__getattr__`, so the member-call shapes are an `AttributeError` at call time — give it a delegating `__getattr__` and they go silent instead | projected at `publish` from the declarations over `server_args`' raw fields; mutated only via `get_context().override` | | runtime flags | `get_flags()` | state that is *not* a pure function of config: `capture` (cuda-graph lifecycle), `moe` (ACTIVE backends, swappable), `dp` (DP-attention runtime flags) | materialized at subsystem init; groups offer `override()` for tests | | resources | `get_resources()`, `get_stream(name)`, `get_buffer(name, factory)` | process-level handles: graph pools, EPLB state, EP dispatcher state, named side streams, workspace buffers | lazy; cleared by `reset_context()` | | per-forward | `get_forward()` | forward-scoped flags (multi-stream switch, MoE output buffer, attn-TP inputs, extend-in-batch) | contextvar-backed; `scoped(**kw)` restores on exit; new threads see defaults | @@ -24,9 +24,8 @@ flags/resources/forward tiers. **`ServerArgs` holds the raw input and nothing else. Resolution writes no field: it declares, and the declarations are what the namespace bags are projected from. -Business code never reads the record for a decision — and after this cut, a field -read there answers with what the operator typed, not with what resolution -decided.** +Business code never reads the record for a decision: a field read there answers +with what the operator typed, not with what resolution decided.** - Every publishing process entry calls `publish(server_args, role=...)` (`run_scheduler_process`, the Ray `SchedulerActor`, the DP controller, tokenizer, @@ -39,8 +38,8 @@ decided.** `run_multi_detokenizer_router_process`: it *is* handed a `ServerArgs`, and uses it only for `configure_logger(server_args)` today, so it has nothing to publish for — a bag read added under that entry needs a `publish` at the entry first. - `publish` snapshots the resolved field - values into the config bags; the accessors (`get_exec()` etc.) fail closed before it + `publish` projects the config bags from the declarations over the record's raw + fields; the accessors (`get_exec()` etc.) fail closed before it runs. `role` records which process type published, and keys per-role namespace enforcement: `SGLANG_ROLE_NAMESPACES=record` audits which namespaces each role's process actually reads (per-pair persisted via `SGLANG_ROLE_NAMESPACES_OUT`; @@ -161,10 +160,10 @@ bag to override at all. and the callee runs in a process that has published.** That second case is a decision, not a style question: the record carries the user's raw input, so a resolution-filled field read off it inside a runner-owned constructor answers - with the pre-resolution value instead of the effective one. Debt means a decision, not automatically a bag read: pick where the - value should come from — usually the `get_*()` bag, sometimes a runner stamp - or a constructor argument (the per-mode attention pair and the encode-server - `gpu_id` above are dispositions of exactly this debt). The per-instance + with the pre-resolution value instead of the effective one. The answer is not + automatically a bag read: pick where the value should come from — usually the + `get_*()` bag, sometimes a runner stamp or a constructor argument (the per-mode + attention pair and the encode-server `gpu_id` above are both this). The per-instance boundaries above are **not** exempt from this unless-clause (the multi-Engine exemption is retracted); each one gets its own disposition. `test_supplied_instance_exposure_ratchet.py` diff --git a/python/sglang/srt/arg_groups/overrides.py b/python/sglang/srt/arg_groups/overrides.py index 6163dfa58..e63520227 100644 --- a/python/sglang/srt/arg_groups/overrides.py +++ b/python/sglang/srt/arg_groups/overrides.py @@ -190,10 +190,8 @@ def resolving_view(server_args: Any) -> ResolvingConfig: return ResolvingConfig(server_args) -# Ordered post-process passes (the normalization stage). List order is the -# end-state execution order and mirrors today's handler call sequence in -# __post_init__; during the transition each pass is invoked from its legacy -# slot via run_post_process_pass, so ordering is preserved byte-for-byte. +# Registered post-process passes. This is a registry, not an execution order: +# each pass is invoked from its own slot via run_post_process_pass. POST_PROCESS_PASSES: List[Callable[..., dict]] = [] @@ -223,11 +221,29 @@ def run_post_process_pass(server_args: Any, fn: Callable[..., dict]) -> None: Evaluates the pass on the resolving state (a read-only view with the accumulated declarations overlaid from the stash) and appends its - declaration to the stash. During ``__post_init__`` the fields stay - untouched: the stash is what the config bags are projected from. A pass - invoked after resolution finished (a post-init slot) writes through - immediately, because there is no later projection to pick it up. + declaration to the stash, which is what the config bags are projected from. + The fields stay untouched. + + A slot that runs after resolution -- ``check_server_args`` hosts one -- lands + in the same stash, which publish projects from later, so it needs no field + write either. After *publish* there is no such later projection: the stash + would grow an entry nothing reads. So, like ``declare_late_resolution``, + this refuses the published record -- post-publish changes go to the bags + through ``get_context().override(...)``. """ + from sglang.srt.runtime_context import get_context + + try: + published = get_context().server_args + except ValueError: + published = None + if published is server_args: + raise ValueError( + f"run_post_process_pass({fn.__qualname__!r}) called on the published " + "config; the stash is projected at publish and never again, so a " + "declaration made here would be a silent no-op -- post-publish " + "changes go to the bags via get_context().override(...)" + ) declared = fn(ResolvedView(server_args, overlay=_declaration_overlay(server_args))) if not isinstance(declared, dict): raise TypeError( @@ -246,13 +262,15 @@ def run_post_process_pass(server_args: Any, fn: Callable[..., dict]) -> None: stash = server_args._resolved_overrides = [] stash.append(entry) validate_declarations(server_args, [entry]) - if getattr(server_args, "_resolution_finished", False): - _apply_fields(server_args, declared) def _apply_fields(server_args: Any, fields: Dict[str, Any]) -> None: - """Write fields on behalf of the pipeline (bypasses the strict bare- - assignment guard that protects post-resolution mutation).""" + """Write record fields past the guard that forbids post-resolution writes. + + Resolution declares, so nothing in the pipeline calls this. It exists for + ``RuntimeContext.override_server_args``, the launch stand-in tests use: there + the caller's values are both the operator's input and resolution's answer. + """ object.__setattr__(server_args, "_internal_write", True) try: for field, value in fields.items(): @@ -1745,9 +1763,7 @@ def _step3p_overrides(server_args: Any, hf_config: Any) -> dict: # --------------------------------------------------------------------------- -# Post-process passes (normalization stage), in end-state execution order. -# Faithful ports of the legacy __post_init__ handlers; each is invoked from -# its legacy slot via run_post_process_pass during the transition. +# Post-process passes (normalization stage). # --------------------------------------------------------------------------- @@ -2803,6 +2819,7 @@ def _moe_runner_fusion_disable(view: Any) -> dict: return {} +@register_post_process def _a2a_fusion_adjustments(view: Any) -> dict: """A2A-backend-driven shared-experts fusion adjustments, declared at the legacy write slots in _handle_a2a_moe: Waterfill requires the @@ -2982,6 +2999,7 @@ def validate_declarations( ) +@register_post_process def _hrm_text_attention_force(view: Any) -> dict: """HRM-Text's bidirectional prefix attention only works on the Triton backend. Invoked as the last attention declaration of the resolution diff --git a/test/registered/unit/server_args/test_record_holds_the_raw_input.py b/test/registered/unit/server_args/test_record_holds_the_raw_input.py new file mode 100644 index 000000000..219595a31 --- /dev/null +++ b/test/registered/unit/server_args/test_record_holds_the_raw_input.py @@ -0,0 +1,161 @@ +"""After resolution the record still holds exactly what the caller passed. + +Two directions: a field must not be *rebound* (the snapshot holds the value the +caller passed, and `getattr` must still answer with it), and an +operator-supplied mutable must not be *edited in place* -- the record points at +the caller's own dict or list, so a handler that reaches into one changes a +value the caller still holds and the snapshot cannot see it. +""" + +import copy +import dataclasses +import json +import os +import shutil +import tempfile +import unittest + +from sglang.srt.server_args import ServerArgs +from sglang.test.ci.ci_register import register_cpu_ci +from sglang.test.test_utils import CustomTestCase + +register_cpu_ci(est_time=30, suite="base-a-test-cpu") + + +_MINI_CONFIG = { + "architectures": ["LlamaForCausalLM"], + "hidden_size": 128, + "intermediate_size": 256, + "max_position_embeddings": 2048, + "model_type": "llama", + "num_attention_heads": 4, + "num_hidden_layers": 2, + "num_key_value_heads": 4, + "rms_norm_eps": 1e-6, + "torch_dtype": "bfloat16", + "vocab_size": 1000, +} + +# One shape per family of handlers that decides something. +_SHAPES = { + "plain": {}, + "data_parallel": {"dp_size": 2}, + "tensor_parallel": {"tp_size": 2}, + "speculative": {"speculative_algorithm": "EAGLE"}, + "speculative_mtp": {"speculative_algorithm": "NEXTN"}, + "cuda_graph_knobs": {"cuda_graph_max_bs_decode": 16, "page_size": 32}, + "explicit_graph_json": {"cuda_graph_config": {"decode": {"max_bs": 12}}}, + "attention_backend": {"attention_backend": "triton"}, + "lora": {"lora_paths": ["adapter=/tmp/does-not-need-to-exist"]}, + "quantization": {"quantization": "fp8"}, + "disaggregation": {"disaggregation_mode": "prefill"}, + "deterministic": {"enable_deterministic_inference": True}, + "hierarchical_cache": {"enable_hierarchical_cache": True}, + "kv_events": {"kv_events_config": '{"publisher":"zmq"}'}, + "chunked_prefill": {"chunked_prefill_size": 1024}, +} + + +class TestRecordHoldsTheRawInput(CustomTestCase): + def setUp(self): + # Resolution writes environment variables, which outlive the record. + super().setUp() + environment = dict(os.environ) + + def restore(): + os.environ.clear() + os.environ.update(environment) + + self.addCleanup(restore) + + def _model_path(self): + path = tempfile.mkdtemp(prefix="raw_input_") + self.addCleanup(shutil.rmtree, path, ignore_errors=True) + with open(os.path.join(path, "config.json"), "w") as handle: + json.dump(_MINI_CONFIG, handle) + return path + + def _resolve(self, **supplied): + """A fully-resolved record: a real config.json, so the pipeline runs + past its dummy-model early return.""" + server_args = ServerArgs( + model_path=self._model_path(), + device="cuda", + random_seed=42, + **supplied, + ) + server_args.resolve_once() + server_args.check_server_args() + return server_args + + def test_no_field_moves_from_what_the_caller_passed(self): + for name, supplied in _SHAPES.items(): + with self.subTest(shape=name): + server_args = self._resolve(**supplied) + raw = server_args._raw_input + + def _moved(current, original): + if current is original: + return False + if isinstance(original, (list, dict, set, bytearray)) or isinstance( + current, (list, dict, set, bytearray) + ): + # A mutable is only unmoved when it is the *same* + # object: an equal copy no longer shares with the caller. + return True + # Equal ints and strings are not always the same object. + return current != original + + moved = { + field.name: (raw[field.name], getattr(server_args, field.name)) + for field in dataclasses.fields(server_args) + if _moved(getattr(server_args, field.name), raw[field.name]) + } + self.assertEqual( + {}, + moved, + f"resolution moved these fields on the {name} shape, so the " + "record no longer answers with the operator's input and a " + "reader that takes a decision off it disagrees with the bags: " + f"{moved}", + ) + + def test_the_snapshot_is_the_value_the_caller_passed(self): + paths = ["adapter=/tmp/does-not-need-to-exist"] + supplied = {"lora_paths": paths, "cuda_graph_max_bs_decode": 16} + expected = copy.deepcopy(supplied) + server_args = self._resolve(**supplied) + for field, value in expected.items(): + self.assertEqual( + value, + server_args._raw_input[field], + f"the snapshot of {field} is not what the caller passed, so " + "every comparison against it is vacuous", + ) + + def test_an_operator_supplied_mutable_is_not_edited_in_place(self): + supplied = { + "cuda_graph_config": {"decode": {"max_bs": 7}}, + "lora_paths": ["adapter=/tmp/does-not-need-to-exist"], + } + before = copy.deepcopy(supplied) + server_args = self._resolve(**supplied) + + for field, value in before.items(): + self.assertEqual( + value, + supplied[field], + f"resolution edited the {field} object the caller still holds; " + "the raw-input snapshot stores the reference, so a field-by-field " + "comparison cannot see this", + ) + self.assertIs( + supplied["cuda_graph_config"], + server_args.cuda_graph_config, + "the record stopped pointing at the caller's object, so the reads " + "above are no longer testing what the caller can observe", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/test/registered/unit/test_chain_read_ratchet.py b/test/registered/unit/test_chain_read_ratchet.py index 1f96123d2..f7d06f8fa 100644 --- a/test/registered/unit/test_chain_read_ratchet.py +++ b/test/registered/unit/test_chain_read_ratchet.py @@ -411,6 +411,87 @@ def _chain_reads(written): return sorted(found) +def _passes_named_at_call_sites() -> set: + """Names passed to ``run_post_process_pass(sa, fn)`` anywhere in the tree. + + A call whose pass is not a bare name is a hard failure, not a skip: this + scan is the ground truth every registry-driven check below is derived from, + so `run_post_process_pass(self, overrides._new_pass)` (an `ast.Attribute`) + or `run_post_process_pass(self, fn=_new_pass)` (a keyword) would otherwise + walk past all of them silently. Keeping the call shape uniform is the + price of the scan being complete. + """ + names = set() + for path in sorted(pathlib.Path(next(iter(sglang.__path__))).rglob("*.py")): + source = path.read_text(encoding="utf-8-sig") + if "run_post_process_pass" not in source: + continue + try: + tree = ast.parse(source) + except SyntaxError: + continue + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func = node.func + if isinstance(func, ast.Name): + called = func.id + elif isinstance(func, ast.Attribute): + called = func.attr + else: + called = None + if called != "run_post_process_pass": + continue + if ( + len(node.args) != 2 + or node.keywords + or not isinstance(node.args[1], ast.Name) + ): + raise AssertionError( + f"{path}:{node.lineno}: run_post_process_pass takes the pass " + "as a bare name in its second positional argument; " + f"{ast.unparse(node)!r} is invisible to this scan and to " + "every registry-driven check derived from it" + ) + names.add(node.args[1].id) + return names + + +class TestEveryInvokedPassIsRegistered(CustomTestCase): + """The registry is what the scans above enumerate, so a pass missing from it + is a pass nothing checks. + + Being invoked and being registered are two edits, and `_a2a_fusion_adjustments` + shipped with only the first: it ran in production while the registry-driven + scans walked past it. The call sites are the ground truth here -- the registry + is derived from a decorator someone has to remember. + """ + + def test_the_registry_covers_every_call_site(self): + from sglang.srt.arg_groups import overrides + + invoked = _passes_named_at_call_sites() + self.assertGreater( + len(invoked), + 20, + f"only {len(invoked)} call sites found; the scan is broken, not the tree", + ) + registered = {fn.__name__ for fn in overrides.POST_PROCESS_PASSES} + self.assertEqual( + set(), + invoked - registered, + "these passes are invoked but carry no @register_post_process, so " + "every check that walks POST_PROCESS_PASSES skips them", + ) + self.assertEqual( + set(), + registered - invoked, + "these passes carry @register_post_process but no slot invokes " + "them; deleting a call site and leaving the decorator behind " + "leaves a pass that only the scans can see", + ) + + class TestNoChainReadsOfResolvedConfig(CustomTestCase): def test_the_census_has_something_to_count(self): """A written set that collapsed would make the pin vacuous. diff --git a/test/registered/unit/test_model_overrides.py b/test/registered/unit/test_model_overrides.py index bb9fb4ff3..891648105 100644 --- a/test/registered/unit/test_model_overrides.py +++ b/test/registered/unit/test_model_overrides.py @@ -1214,26 +1214,26 @@ class TestGoldenModelOverrides(_IsolatedPublish): (self._publish(sa), self._leaf("attention_backend"))[1], declared_values[-1] ) - def test_post_materialize_pass_writes_through(self): + def test_a_pass_after_resolution_declares_without_writing(self): from sglang.srt.arg_groups.overrides import run_post_process_pass - # A pass invoked after materialization (a post-init slot, like the - # legacy runner-side adjustments) declares AND writes through, so - # field readers and the publish see the same end state. sa = self._construct("LlamaForCausalLM", "llama") - resolved_before = self._resolved(sa, "attention_backend") + raw_before = sa.attention_backend def _force_triton(view): - if view.attention_backend != "triton": - return {"attention_backend": "triton"} - return {} + return {"attention_backend": "triton"} run_post_process_pass(sa, _force_triton) - if resolved_before != "triton": - self.assertEqual(self._resolved(sa, "attention_backend"), "triton") + + self.assertEqual("triton", self._resolved(sa, "attention_backend")) self.assertEqual( - (self._publish(sa), self._leaf("attention_backend"))[1], - self._resolved(sa, "attention_backend"), + (self._publish(sa), self._leaf("attention_backend"))[1], "triton" + ) + self.assertEqual( + raw_before, + sa.attention_backend, + "the pass wrote the field, so the record stopped answering with the " + "operator's input", ) def test_attention_backend_user_choice_declares_nothing_extra(self):