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".
This commit is contained in:
Cheng Wan
2026-08-15 00:40:37 -07:00
committed by GitHub
parent d804b6bd98
commit c87a2ced12
2 changed files with 155 additions and 11 deletions
+25 -4
View File
@@ -99,8 +99,10 @@ bag to override at all.
instance: **constructor arguments** (`ModelRunner(draft_attention_backend=...)`, instance: **constructor arguments** (`ModelRunner(draft_attention_backend=...)`,
`MMEncoder(gpu_id=...)`) and **runner attributes holding the resolved value** `MMEncoder(gpu_id=...)`) and **runner attributes holding the resolved value**
(`model_runner.kv_cache_dtype_str`, `prefill_attention_backend_str`, (`model_runner.kv_cache_dtype_str`, `prefill_attention_backend_str`,
`num_fused_shared_experts`) — threaded to consumers as arguments, never `num_fused_shared_experts`, `linear_attn_backends`) — threaded to consumers as
backfilled onto a shared object. The one sanctioned bend in that rule is 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 *scoped*: `ModelRunner._load_format_scope` exposes the draft's
`--speculative-draft-load-format` through `get_model().override(load_format=...)` `--speculative-draft-load-format` through `get_model().override(load_format=...)`
for exactly the duration of the draft build, because model construction 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; encode-server DP workers used to specialize a config copy for the same reason;
their device now travels as `MMEncoder(gpu_id=...)`.) their device now travels as `MMEncoder(gpu_id=...)`.)
- **Whole-object passes** (`f(server_args)` handing the instance along) keep the - **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 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 ### `get_parallel()`: config leaves vs live topology
@@ -59,6 +59,13 @@ _EXEC_SUBS = (
class TestConfigBags(CustomTestCase): 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): def setUp(self):
rc.reset_context() rc.reset_context()
@@ -76,14 +83,130 @@ class TestConfigBags(CustomTestCase):
with self.assertRaises(ValueError): with self.assertRaises(ValueError):
rc.get_memory() rc.get_memory()
def test_bag_values_match_server_args(self): def test_the_bags_carry_what_resolution_produced(self):
sa = self._publish() """The projection is faithful: each leaf is the *resolved* value.
self.assertEqual(rc.get_exec().moe.moe_runner_backend, sa.moe_runner_backend)
self.assertEqual(rc.get_exec().kernel.attention_backend, sa.attention_backend) Two requirements the dummy-model shortcut cannot meet: the record must
self.assertEqual(rc.get_memory().hicache_ratio, sa.hicache_ratio) 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_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): def test_all_accessors_and_exec_subgroups(self):
self._publish() self._publish()