From c87a2ced121a4af3d7ec58d3e5ac2ec88cea7ed7 Mon Sep 17 00:00:00 2001 From: Cheng Wan <54331508+ch-wan@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:40:37 -0700 Subject: [PATCH] test(step-12): state the bag contract as what resolution produced, and the skill rule that goes with it `test_bag_values_match_server_args` asserted `bag == field`. That holds today only because construction resolves in place; step 12 keeps the record raw, and the plan doc calls this test out as one that becomes **false by design** for every field resolution fills in. Rewritten against the resolved projection, which is the half that survives: the bag carries what resolution produced. The `bag == field` assertion stays as one line at the end, labelled as the tripwire -- when it starts failing for a resolution-written leaf, the flip has landed and the bag is the only place the effective value lives. The reference is an independent resolution of the same raw input (a fresh, never-published record) rather than `resolved_server_args_dict()`, which reads `vars(server_args)` back and therefore only restates the published instance. And the record goes through the real pipeline on a real mini config, published through `publish()`: the dummy-model path returns at the dummy boundary with every sampled leaf still raw, so the old comparison was raw==raw and vacuous (both Codex catches). Reproducibility (#34094) licenses the sibling as a stand-in for the pipeline output. The sample admits only leaves resolution writes on this input on both CI device shapes (attention_backend, page_size, chunked_prefill_size, mem_fraction_static), and the raw-differs guard asserts it per leaf -- a default-count threshold let supplied inputs like `model_path` (no dataclass default, so any path "differs") stand in for resolution work. Passthrough leaves (host, hicache_ratio, moe_runner_backend, model_path) move to a separate projection smoke that claims only what it checks: publish projected an unchanged field into its namespace. Between the two resolutions the test restores environ and the EnvField none-flags, so the sibling resolves the same pristine input rather than the first resolution's leftovers. And the class runs its body exactly once, like the other dual-resolve harnesses: a CI retry re-enters after the first attempt leaked process state, which is the hazard the pristine snapshot exists to rule out. docs(skill): a supplied-instance read is not automatically safe The whole-object rule said "keep the supplied-instance contract; don't rewrite the parameter reads unless the field is runtime-mutated". That is the right rule for the *object* and the wrong stopping point for the *field*: after step 12 the record carries the user's raw input, so `server_args.page_size` inside a runner-owned constructor reads the CLI default rather than the effective value. The rule now names that second case as step-12 debt with a guard attached (`test_supplied_instance_exposure_ratchet.py` fails on a new pair, so the decision is made when the read is written), and names the two shapes that stay parameter-form on purpose: a helper the resolution pipeline calls with a `resolved_view`, and a factory whose contract is "build X from the record you are handed". --- .../skills/sglang-runtime-context/SKILL.md | 29 +++- .../unit/test_runtime_context_config_bags.py | 137 +++++++++++++++++- 2 files changed, 155 insertions(+), 11 deletions(-) diff --git a/.claude/skills/sglang-runtime-context/SKILL.md b/.claude/skills/sglang-runtime-context/SKILL.md index cd668fa1c..0215c4110 100644 --- a/.claude/skills/sglang-runtime-context/SKILL.md +++ b/.claude/skills/sglang-runtime-context/SKILL.md @@ -99,8 +99,10 @@ bag to override at all. instance: **constructor arguments** (`ModelRunner(draft_attention_backend=...)`, `MMEncoder(gpu_id=...)`) and **runner attributes holding the resolved value** (`model_runner.kv_cache_dtype_str`, `prefill_attention_backend_str`, - `num_fused_shared_experts`) — threaded to consumers as arguments, never - backfilled onto a shared object. The one sanctioned bend in that rule is + `num_fused_shared_experts`, `linear_attn_backends`) — threaded to consumers as + arguments, never backfilled onto a shared object. A per-runner choice also stays + *out* of the bags: recording it there is how a second runner inherits the first + one's answer, which is exactly the bug `linear_attn_backends` replaced. The one sanctioned bend in that rule is *scoped*: `ModelRunner._load_format_scope` exposes the draft's `--speculative-draft-load-format` through `get_model().override(load_format=...)` for exactly the duration of the draft build, because model construction @@ -116,9 +118,28 @@ bag to override at all. encode-server DP workers used to specialize a config copy for the same reason; their device now travels as `MMEncoder(gpu_id=...)`.) - **Whole-object passes** (`f(server_args)` handing the instance along) keep the - supplied-instance contract; don't rewrite the parameter reads to bag reads unless the + supplied-instance contract; don't rewrite the parameter reads unless the field is runtime-mutated (see the elastic-EP `ep_size` case in - `eplb/expert_location.py`). + `eplb/expert_location.py`) — **or the field is one that resolution fills in + and the callee runs in a process that has published.** That second case is + step-12 debt, not a style question: the record is destined to carry the + user's raw input, so `server_args.page_size` inside a runner-owned + constructor will read the raw 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). And the per-instance + boundaries above stay exempt from this unless-clause: a multi-Engine site + must not become a process-global bag read even for a resolution-filled + field. `test_supplied_instance_exposure_ratchet.py` + pins the remaining set — three spellings of the read: `server_args.field`, + literal-name `getattr(server_args, "field", default)`, and the parked form + (`self.x = server_args` in a method that takes the parameter, read as + `self.x.field` anywhere in the class) — and fails on a new one, so the + disposition gets picked when the read is written. Two shapes stay parameter-form on purpose: a helper the + *resolution pipeline* calls with a `resolved_view` (its parameter happens to be + named `server_args`), and a factory whose contract is "build X from the record + you are handed" (`create_kt_config_from_server_args`, `DllmConfig.from_server_args`). ### `get_parallel()`: config leaves vs live topology diff --git a/test/registered/unit/test_runtime_context_config_bags.py b/test/registered/unit/test_runtime_context_config_bags.py index 4db5ddb22..8b901cc37 100644 --- a/test/registered/unit/test_runtime_context_config_bags.py +++ b/test/registered/unit/test_runtime_context_config_bags.py @@ -59,6 +59,13 @@ _EXEC_SUBS = ( class TestConfigBags(CustomTestCase): + def _callTestMethod(self, method): + # No retry: CustomTestCase retries once in CI, but `addCleanup` runs + # after the last attempt, so a retry of the dual-resolve case would + # re-enter with the first attempt's leaked process state instead of + # the pristine snapshot the sibling helper restores from. + unittest.TestCase._callTestMethod(self, method) + def setUp(self): rc.reset_context() @@ -76,14 +83,130 @@ class TestConfigBags(CustomTestCase): with self.assertRaises(ValueError): rc.get_memory() - def test_bag_values_match_server_args(self): - sa = self._publish() - self.assertEqual(rc.get_exec().moe.moe_runner_backend, sa.moe_runner_backend) - self.assertEqual(rc.get_exec().kernel.attention_backend, sa.attention_backend) - self.assertEqual(rc.get_memory().hicache_ratio, sa.hicache_ratio) + def test_the_bags_carry_what_resolution_produced(self): + """The projection is faithful: each leaf is the *resolved* value. + + Two requirements the dummy-model shortcut cannot meet: the record must + go through the real resolution pipeline (a dummy path returns at the + dummy-model boundary with every sampled leaf still raw, so raw==raw + would pass vacuously), and the reference must be an independent + resolution of the same raw input -- a fresh, never-published record, + resolved after restoring the process state (environ and the EnvField + none-flags) the first resolution may have written -- so the assertion + is "bag == what resolution produces", not "bag == the instance publish + copied from". Reproducibility (`test_resolution_is_reproducible`) + licenses the sibling as a stand-in for the pipeline's output. The + raw-differs guard keeps the comparison meaningful: every sampled leaf + must have moved off its dataclass default, so each equality compares a + value resolution demonstrably wrote. Supplied construction inputs + (`model_path`, `device`, `random_seed`) and leaves resolution leaves + alone never enter the sample -- projection coverage for those lives in + `test_passthrough_leaves_project_into_their_namespaces`. Step 12 keeps + records at the user's raw input; then the sibling goes raw and this + assertion starts failing for every sampled leaf, which is the signal + the bags became the only home of the effective value. + """ + import dataclasses + + sa, reference = self._resolve_published_and_sibling() + defaults = {f.name: f.default for f in dataclasses.fields(ServerArgs)} + # Leaves resolution writes on this input on both CI device shapes + # (CUDA host and CPU-only runner): each starts at a None default. + sampled = ( + (lambda: rc.get_exec().kernel.attention_backend, "attention_backend"), + (lambda: rc.get_schedule().page_size, "page_size"), + (lambda: rc.get_schedule().chunked_prefill_size, "chunked_prefill_size"), + (lambda: rc.get_schedule().mem_fraction_static, "mem_fraction_static"), + ) + for accessor, leaf in sampled: + with self.subTest(leaf=leaf): + # The raw-differs guard: a sampled leaf that still sits on its + # default (or has none to differ from) proves nothing. + self.assertIsNot(defaults[leaf], dataclasses.MISSING) + self.assertNotEqual(getattr(reference, leaf), defaults[leaf]) + self.assertEqual(accessor(), getattr(reference, leaf)) + # And the record agrees today, which is what step 12 changes: when this + # assertion starts failing for a resolution-written leaf, the flip + # landed and the bag is the only place the effective value lives. self.assertEqual(rc.get_schedule().page_size, sa.page_size) - self.assertEqual(rc.get_serving().host, sa.host) - self.assertEqual(rc.get_model().model_path, sa.model_path) + + def test_passthrough_leaves_project_into_their_namespaces(self): + """Thin projection smoke over leaves resolution does not move. + + `bag == instance` is all these equalities can claim (publish projected + an unchanged field into serving/memory/moe/model) -- resolution + faithfulness is `test_the_bags_carry_what_resolution_produced`'s job. + """ + sa = self._publish() + sampled = ( + (lambda: rc.get_serving().host, "host"), + (lambda: rc.get_memory().hicache_ratio, "hicache_ratio"), + (lambda: rc.get_exec().moe.moe_runner_backend, "moe_runner_backend"), + (lambda: rc.get_model().model_path, "model_path"), + ) + for accessor, leaf in sampled: + with self.subTest(leaf=leaf): + self.assertEqual(accessor(), getattr(sa, leaf)) + + def _resolve_published_and_sibling(self): + """Resolve a real mini config twice from the same pristine process + state: publish the first record, hand back the never-published sibling + as the reference.""" + import json + import os + import shutil + import tempfile + + from sglang.srt.environ import EnvField, envs + + config_dir = tempfile.mkdtemp(prefix="bag_contract_") + self.addCleanup(shutil.rmtree, config_dir, ignore_errors=True) + with open(os.path.join(config_dir, "config.json"), "w") as handle: + json.dump( + { + "architectures": ["LlamaForCausalLM"], + "model_type": "llama", + "hidden_size": 16, + "intermediate_size": 32, + "num_attention_heads": 2, + "num_key_value_heads": 2, + "num_hidden_layers": 2, + "vocab_size": 128, + "max_position_embeddings": 2048, + }, + handle, + ) + + # Resolution can write process state os.environ does not carry (the + # multimodal transport env sticky plus the EnvField descriptor flag + # `set()` flips); snapshot both so the sibling resolves the same + # pristine input. Walk the MRO: `vars(type(envs))` alone would miss + # fields declared on a base class. + env_fields = {} + for klass in reversed(type(envs).__mro__): + for name, field in vars(klass).items(): + if isinstance(field, EnvField): + env_fields[name] = field + environ_before = dict(os.environ) + none_flags_before = { + name: field._set_to_none for name, field in env_fields.items() + } + + def restore_process_state(): + os.environ.clear() + os.environ.update(environ_before) + for name, was_none in none_flags_before.items(): + getattr(type(envs), name)._set_to_none = was_none + + self.addCleanup(restore_process_state) + + def resolve(): + return ServerArgs(model_path=config_dir, device="cuda", random_seed=42) + + sa = resolve() + rc.publish(sa, role="scheduler") + restore_process_state() + return sa, resolve() def test_all_accessors_and_exec_subgroups(self): self._publish()