diff --git a/.claude/skills/sglang-runtime-context/SKILL.md b/.claude/skills/sglang-runtime-context/SKILL.md index a5690abcd..a2664e245 100644 --- a/.claude/skills/sglang-runtime-context/SKILL.md +++ b/.claude/skills/sglang-runtime-context/SKILL.md @@ -60,17 +60,16 @@ resolved configuration lives in the namespace bags.** multi-tokenizer workers it is serialized for, the schedulers it forks. Returning a variant here is a bug: the launcher rebinds its local and everyone else keeps the unresolved object. -- **A config another runner / worker / process is built from**: `server_args.derive( - source, **fields)` returns a variant (an encode worker's `base_gpu_id`/`tp_size`). - The receiver — and any bags projected from it — are untouched; resolution does - **not** re-run, so do not reach for it to "re-resolve" a config. -- **Per-runner values inside one process are constructor arguments, not a variant.** - The draft worker's `context_length`, load format and attention backend travel as - arguments to `TpModelWorker` / `ModelRunner` and live on the runner - (`ModelRunner.draft_attention_backend`, `kv_cache_dtype_str`, …), because target - and draft coexist and the process-wide bags can only describe one of them. +- **A value another runner / worker owns is a constructor argument, not a config + copy.** The draft worker's `context_length`, load format and attention backend + travel as arguments to `TpModelWorker` / `ModelRunner` and live on the runner + (`ModelRunner.draft_attention_backend`, `kv_cache_dtype_str`, …); the encoder + DP worker's device is `MMEncoder(gpu_id=...)`. There is no `ServerArgs.derive` + any more — a config object is never copied-and-edited; test doubles that need a + modified copy use `sglang.test.test_utils.server_args_variant`. -**Why a bag override cannot stand in for the last two.** The bags are projected at +**Why a bag override cannot stand in for late resolution or per-runner +construction.** The bags are projected at publish *from the instance's fields*, so anything the runtime must read has to be on the instance before publish — an override afterwards puts instance and bags back out of agreement, and whole-object readers (`ModelConfig.from_server_args`, @@ -98,10 +97,11 @@ bag to override at all. - **Per-instance boundaries** — the tokenizer-manager family, everything under `entrypoints/`, and the tokenizer-process multimodal processors read `self.server_args`: several `Engine`s can share one process, and the process-global - bags are last-publish-wins across engines. `base_gpu_id` also differs per worker - (the encode-server DP workers each specialize their own copy), so no process-global - value can stand in for it — `BaseMultimodalProcessor._fast_image_processor_device` - is the shape to copy. + bags are last-publish-wins across engines. `base_gpu_id` also differs per engine, + so no process-global value can stand in for it — + `BaseMultimodalProcessor._fast_image_processor_device` is the shape to copy. (The + 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 field is runtime-mutated (see the elastic-EP `ep_size` case in @@ -240,16 +240,16 @@ ONE thread — do not design for TBO threads that don't exist. 1. **Strict mutation guard** (always on): bare `server_args.x = ...` after resolution raises unconditionally in `ServerArgs.__setattr__` — this *is* the guarantee that no writer can desync the bags, so there is no writer ratchet any more. Change - resolved config with `get_context().override`, build a per-runner config with - `server_args.derive`. Projected bags are sealed the same way (leaf assignment - raises). + resolved config with `get_context().override`; hand a per-runner value to its + runner as a constructor argument. Projected bags are sealed the same way (leaf + assignment raises). 2. **Mutation ratchet** (`test_server_args_mutation_ratchet.py`, exact pin 0 over the whole package minus the pipeline / multimodal_gen): textual scan for assignment forms. Never raise the baseline. -3. **Derive contract** (`test_server_args_derive.py`): deriving leaves the receiver - intact, a published config still refuses assignment, and deriving does not publish. - Rerouting a writer to the bags means flipping **all its readers in the same commit** - (no transitional dual-write). +3. **No-copy contract** (`test_server_args_no_instance_mutation_entry.py`): neither + `ServerArgs.override` nor `ServerArgs.derive` exists, and nothing in the package + calls either form. Rerouting a writer to the bags means flipping **all its readers + in the same commit** (no transitional dual-write). 4. **Legacy-accessor ratchet** (`test_legacy_global_ratchet.py`): `get_global_server_args` call sites must not grow — new code uses `runtime_context.get_server_args()` (and business decisions should read the bags). diff --git a/python/sglang/srt/disaggregation/encode_server.py b/python/sglang/srt/disaggregation/encode_server.py index 600942960..3fbd16497 100644 --- a/python/sglang/srt/disaggregation/encode_server.py +++ b/python/sglang/srt/disaggregation/encode_server.py @@ -288,7 +288,12 @@ class MMEncoder: schedule_path=None, dist_init_method=None, rank: int = 0, + gpu_id: Optional[int] = None, ): + """``gpu_id`` pins this encoder to a device other than + ``base_gpu_id + rank`` — the DP launcher's per-worker placement. It is + this instance's value, not a config change, so it travels as an + argument.""" logger.info(f"init MMEncoder {rank}/{server_args.tp_size}") self.server_args = server_args publish(server_args, role="encoder") @@ -315,7 +320,7 @@ class MMEncoder: ).lower() self.device = server_args.device - self.gpu_id = server_args.base_gpu_id + rank + self.gpu_id = server_args.base_gpu_id + rank if gpu_id is None else gpu_id self.device_config = DeviceConfig( device=self.device, @@ -3517,10 +3522,13 @@ async def run_dp_worker( ) # gpu_id is the device chosen by maybe_reindex_device_id in the parent: - # 0 when CVD is pinned to one GPU, else the absolute id. rank=0, so - # MMEncoder runs set_device(base_gpu_id). - args = server_args.derive("encode_server.dp_worker", base_gpu_id=gpu_id, tp_size=1) - enc = MMEncoder(args, dist_init_method=f"tcp://127.0.0.1:{get_free_port()}", rank=0) + # 0 when CVD is pinned to one GPU, else the absolute id. + enc = MMEncoder( + server_args, + dist_init_method=f"tcp://127.0.0.1:{get_free_port()}", + rank=0, + gpu_id=gpu_id, + ) global encoder_metrics_collector if server_args.enable_metrics: diff --git a/python/sglang/srt/runtime_context.py b/python/sglang/srt/runtime_context.py index 9c742a864..78bd6908c 100644 --- a/python/sglang/srt/runtime_context.py +++ b/python/sglang/srt/runtime_context.py @@ -1027,9 +1027,19 @@ class _ServerArgsOverride: self._prev_publish_role = ctx._publish_role self._prev_parallel_config = ctx.parallel._config self._prev_capture = ctx.flags.capture.enable_torch_compile + from sglang.srt.arg_groups.overrides import _apply_fields + server_args = ServerArgs(model_path="dummy") - if self._fields: - server_args = server_args.derive("test-override", **self._fields) + # Underscore names seed private property caches (the strict guard + # exempts them); everything else must be a real config field. + unknown = {name for name in self._fields if not name.startswith("_")} - set( + type(server_args).__dataclass_fields__ + ) + if unknown: + raise ValueError( + f"override_server_args: unknown ServerArgs field(s): {sorted(unknown)}" + ) + _apply_fields(server_args, self._fields) # The dummy boundary skips materialization, which would leave the # strict mutation guard unarmed on the published object — mark it # materialized so bare post-publish writes raise like they do on a diff --git a/python/sglang/srt/server_args.py b/python/sglang/srt/server_args.py index 780a2f6f8..ea1f02b32 100644 --- a/python/sglang/srt/server_args.py +++ b/python/sglang/srt/server_args.py @@ -16,7 +16,6 @@ from __future__ import annotations import argparse -import copy import dataclasses import glob import importlib @@ -8609,52 +8608,12 @@ class ServerArgs: declare_late_resolution(self, source, **fields) - def derive(self, source: str, **fields) -> ServerArgs: - """A copy carrying variant values: a draft worker's context length, an - encode worker's device, a launcher's late port pick. - - The receiver is untouched, so a config already published from it -- and - the namespace bags projected out of it -- stay true; the variant is a - second config, to be published in its own right or handed to whoever - owns it. Resolution does not re-run: the values being set are decided - *after* it, from inputs resolution never had, and re-resolving a - resolved config re-derives conditional decisions from the wrong ones. - - Whitelisted resolvable fields also join the copy's declaration stash so - a later re-resolution keeps them; ``source`` is recorded for provenance. - """ - from sglang.srt.arg_groups.arg_utils import resolvable_fields - - variant = copy.deepcopy(self) - whitelist = resolvable_fields(type(variant)) - declared = {k: v for k, v in fields.items() if k in whitelist} - rest = {k: v for k, v in fields.items() if k not in whitelist} - if declared: - stash = getattr(variant, "_resolved_overrides", None) - if stash is None: - stash = [] - object.__setattr__(variant, "_resolved_overrides", stash) - stash.append((source, dict(declared))) - if rest: - log = getattr(variant, "_runtime_mutations", None) - if log is None: - log = [] - object.__setattr__(variant, "_runtime_mutations", log) - log.append((source, dict(rest))) - object.__setattr__(variant, "_internal_write", True) - try: - for field, value in fields.items(): - setattr(variant, field, value) - finally: - object.__setattr__(variant, "_internal_write", False) - return variant - def __setattr__(self, name, value): # After materialization the fields are the resolved startup # configuration -- the pristine, READ-ONLY record that the config bags # were projected from. Resolved config changes go to the bags via - # get_context().override(source, ...); a config that differs per runner - # or per worker is a separate object, built with derive(). + # get_context().override(source, ...); a value one runner or worker + # owns travels as a constructor argument to it. if ( not name.startswith("_") and getattr(self, "_declarations_materialized", False) @@ -8663,8 +8622,8 @@ class ServerArgs: raise AttributeError( f"server_args.{name} assigned after resolution; server_args is " "read-only -- use get_context().override(source, ...) to change " - "resolved config, or server_args.derive(source, ...) to build a " - "variant for one runner." + "resolved config; a value one runner owns travels as a " + "constructor argument." ) object.__setattr__(self, name, value) diff --git a/python/sglang/test/kits/attention_unittest/runner_modes/speculative_draft_runner.py b/python/sglang/test/kits/attention_unittest/runner_modes/speculative_draft_runner.py index 46db9aebf..964eec4c8 100644 --- a/python/sglang/test/kits/attention_unittest/runner_modes/speculative_draft_runner.py +++ b/python/sglang/test/kits/attention_unittest/runner_modes/speculative_draft_runner.py @@ -326,10 +326,9 @@ def _configure_runner_for_eagle_draft( "use_mla_backend": runner.use_mla_backend, } from sglang.srt.runtime_context import get_context + from sglang.test.test_utils import server_args_variant - runner.server_args = runner.server_args.derive( - "attention-unittest-eagle-draft", **updates - ) + runner.server_args = server_args_variant(runner.server_args, **updates) get_context().set_server_args(runner.server_args) runner.spec_algorithm = SpeculativeAlgorithm.EAGLE @@ -392,9 +391,10 @@ def _build_frozen_kv_mtp_fixture( ) _configure_runner_for_eagle_draft(fixture.runner, case, settings) from sglang.srt.runtime_context import get_context + from sglang.test.test_utils import server_args_variant - fixture.runner.server_args = fixture.runner.server_args.derive( - "attention_unittest.frozen_kv_draft", speculative_algorithm="FROZEN_KV_MTP" + fixture.runner.server_args = server_args_variant( + fixture.runner.server_args, speculative_algorithm="FROZEN_KV_MTP" ) get_context().set_server_args(fixture.runner.server_args) fixture.runner.spec_algorithm = SpeculativeAlgorithm.FROZEN_KV_MTP diff --git a/python/sglang/test/test_utils.py b/python/sglang/test/test_utils.py index 2a9c1a68d..074ad24ae 100644 --- a/python/sglang/test/test_utils.py +++ b/python/sglang/test/test_utils.py @@ -2345,6 +2345,28 @@ def _wait_for_gpu_idle_in_ci( pass +def server_args_variant(server_args, **fields): + """A modified deep copy of a config, for a test double whose fixture + differs from the (possibly published, read-only) config it starts from. + The receiver is untouched; the copy keeps its read-only guard. + + A name may also shadow a method with a fixture value (the runner kits set + ``use_mla_backend``, a method ModelRunner itself overwrites at init); + names that exist nowhere on the class fail loudly.""" + variant = copy.deepcopy(server_args) + cls = type(variant) + unknown = { + name + for name in fields + if name not in cls.__dataclass_fields__ and not hasattr(cls, name) + } + if unknown: + raise ValueError(f"unknown ServerArgs field(s): {sorted(unknown)}") + for name, value in fields.items(): + object.__setattr__(variant, name, value) + return variant + + class CustomTestCase(unittest.TestCase): def __init_subclass__(cls, **kwargs): diff --git a/test/registered/attention/unittests/hybrid_linear/test_flashinfer_mla_chunk_metadata.py b/test/registered/attention/unittests/hybrid_linear/test_flashinfer_mla_chunk_metadata.py index e5cebe281..0201b27e7 100644 --- a/test/registered/attention/unittests/hybrid_linear/test_flashinfer_mla_chunk_metadata.py +++ b/test/registered/attention/unittests/hybrid_linear/test_flashinfer_mla_chunk_metadata.py @@ -46,9 +46,10 @@ class _ChunkKVMLARunner(MockMLAModelRunner): def __init__(self, **kwargs): super().__init__(**kwargs) from sglang.srt.runtime_context import get_context + from sglang.test.test_utils import server_args_variant - self.server_args = self.server_args.derive( - "attention-unittest", + self.server_args = server_args_variant( + self.server_args, disable_chunked_prefix_cache=False, flashinfer_mla_disable_ragged=False, ) diff --git a/test/registered/unit/test_runtime_context.py b/test/registered/unit/test_runtime_context.py index 6079f9614..b64ab5f79 100644 --- a/test/registered/unit/test_runtime_context.py +++ b/test/registered/unit/test_runtime_context.py @@ -261,9 +261,9 @@ class TestServerArgsScopedOverride(_IsolatedServerArgs): # unnamed fields keep their dataclass defaults self.assertEqual(published.tp_size, 1) - def test_fields_carry_provenance(self): - published = get_context().override_server_args(tp_size=4).install() - self.assertIn(("test-override", {"tp_size": 4}), published._runtime_mutations) + def test_unknown_fields_are_rejected(self): + with self.assertRaises(ValueError): + get_context().override_server_args(not_a_config_field=1).install() def test_restore_reinstates_previous_publish(self): previous = object() @@ -299,12 +299,10 @@ class TestServerArgsScopedOverride(_IsolatedServerArgs): def test_installed_config_arms_the_strict_guard(self): # The published dummy must behave like a resolved config: bare writes - # raise, and a differing value lives on a derived variant. + # raise. published = get_context().override_server_args(tp_size=2).install() with self.assertRaises(AttributeError): published.tp_size = 4 - variant = published.derive("test", tp_size=4) - self.assertEqual(variant.tp_size, 4) self.assertEqual(published.tp_size, 2) def test_restore_resets_the_capture_seed(self): diff --git a/test/registered/unit/test_runtime_context_override.py b/test/registered/unit/test_runtime_context_override.py index 93c344242..462c0b812 100644 --- a/test/registered/unit/test_runtime_context_override.py +++ b/test/registered/unit/test_runtime_context_override.py @@ -118,9 +118,7 @@ class TestContextOverride(CustomTestCase): rc.get_context().override( "ModelRunner.configure_kv_cache_dtype", kv_cache_dtype="fp8_e4m3" ) - draft = ServerArgs(model_path="dummy").derive( - "draft-build", kv_cache_dtype="bf16" - ) + draft = ServerArgs(model_path="dummy", kv_cache_dtype="bf16") with rc.get_context().preserve_config(): rc.get_context().set_server_args(draft) # Inside the scope the draft's bags are live... diff --git a/test/registered/unit/test_server_args_derive.py b/test/registered/unit/test_server_args_derive.py deleted file mode 100644 index 3ddca37e5..000000000 --- a/test/registered/unit/test_server_args_derive.py +++ /dev/null @@ -1,79 +0,0 @@ -"""``ServerArgs.derive`` is the only way one config becomes another. - -After resolution the instance is the process's read-only startup record and the -object the config bags were projected from, so it cannot be mutated: a change to -resolved config goes to the bags (``get_context().override``), and a config that -differs for one runner or worker — a draft's context length, an encode worker's -device — is a second object. -""" - -from sglang.test.ci.ci_register import register_cpu_ci - -register_cpu_ci(est_time=5, suite="base-a-test-cpu") - -import unittest - -from sglang.srt.runtime_context import get_context, reset_context -from sglang.srt.server_args import ServerArgs -from sglang.test.test_utils import CustomTestCase - - -class TestServerArgsDerive(CustomTestCase): - def tearDown(self): - reset_context() - - def _resolved(self) -> ServerArgs: - """A config in the post-resolution state, without a real model.""" - server_args = ServerArgs(model_path="dummy") - object.__setattr__(server_args, "_declarations_materialized", True) - return server_args - - def test_the_receiver_is_untouched(self): - server_args = self._resolved() - variant = server_args.derive("draft_worker.copy", context_length=1024) - - self.assertEqual(variant.context_length, 1024) - self.assertIsNot(variant, server_args) - self.assertIsNone(server_args.context_length) - - def test_a_published_config_rejects_assignment_but_still_derives(self): - override = get_context().override_server_args(tp_size=2) - published = override.install() - self.addCleanup(override.restore) - - with self.assertRaises(AttributeError): - published.tp_size = 4 - - self.assertEqual(published.derive("worker", tp_size=4).tp_size, 4) - self.assertEqual(published.tp_size, 2) - - def test_the_variant_is_not_published_by_deriving(self): - override = get_context().override_server_args(tp_size=2) - published = override.install() - self.addCleanup(override.restore) - - published.derive("worker", tp_size=4) - - self.assertIs(get_context().server_args, published) - - def test_the_variant_is_frozen_too(self): - variant = self._resolved().derive("worker", context_length=1024) - with self.assertRaises(AttributeError): - variant.context_length = 2048 - - def test_provenance_is_recorded(self): - variant = self._resolved().derive("draft_worker.copy", watchdog_timeout=1.0) - self.assertIn( - ("draft_worker.copy", {"watchdog_timeout": 1.0}), - getattr(variant, "_runtime_mutations", []), - ) - - def test_a_resolvable_field_joins_the_declaration_stash(self): - """So a re-resolution of the variant keeps the derived value.""" - variant = self._resolved().derive("draft_worker.build", kv_cache_dtype="bf16") - stash = getattr(variant, "_resolved_overrides", []) - self.assertIn(("draft_worker.build", {"kv_cache_dtype": "bf16"}), stash) - - -if __name__ == "__main__": - unittest.main() diff --git a/test/registered/unit/test_server_args_mutation_ratchet.py b/test/registered/unit/test_server_args_mutation_ratchet.py index 4eea1aefe..f3dad1c12 100644 --- a/test/registered/unit/test_server_args_mutation_ratchet.py +++ b/test/registered/unit/test_server_args_mutation_ratchet.py @@ -10,9 +10,9 @@ baseline to lock in the progress. There is no post-resolution mutation entry point on the instance any more: resolved config changes go to the context bags via -``get_context().override(source, **fields)``, and a config that differs for one -runner or worker is a separate object built with ``ServerArgs.derive(source, -**fields)``. The baseline is therefore zero. ``ServerArgs.__setattr__`` raises +``get_context().override(source, **fields)``, and a value that differs for one +runner or worker travels as a constructor argument to it. The baseline is +therefore zero. ``ServerArgs.__setattr__`` raises on a bare assignment after resolution; this ratchet catches the sites the tests never execute. """ @@ -70,8 +70,8 @@ class TestServerArgsMutationRatchet(CustomTestCase): f"{count} > baseline {_BASELINE}. Configuration is resolved in " "ServerArgs.__post_init__; declare through the pipeline " "(passes / declare_load_time_override), change resolved config " - "with get_context().override(source, ...), or build a variant " - "with server_args.derive(source, ...) — do not assign fields." + "with get_context().override(source, ...), or hand the value " + "to its runner as a constructor argument — do not assign fields." ) if count < _BASELINE: self.fail( diff --git a/test/registered/unit/test_server_args_no_instance_mutation_entry.py b/test/registered/unit/test_server_args_no_instance_mutation_entry.py index 2656589ad..1afbf9fa1 100644 --- a/test/registered/unit/test_server_args_no_instance_mutation_entry.py +++ b/test/registered/unit/test_server_args_no_instance_mutation_entry.py @@ -1,13 +1,14 @@ """``ServerArgs`` has no in-place mutation entry, and nothing calls one. -``ServerArgs.override(source, **fields)`` used to mutate a resolved instance; -after resolution the fields are the record the config bags were projected from, -so such a write desyncs every namespace reader. The method is gone and the two -sanctioned replacements are ``get_context().override`` (post-publish, writes the -bags) and ``ServerArgs.derive`` (a variant for another runner / process). Late -launcher-stage resolution writes in place through -``arg_groups.overrides.declare_late_resolution``, which refuses the published -instance. +``ServerArgs.override(source, **fields)`` used to mutate a resolved instance, +and ``ServerArgs.derive(source, **fields)`` used to copy-and-edit one; after +resolution the fields are the record the config bags were projected from, so a +write desyncs every namespace reader, and a copy invites publishing stale +variants. Both are gone: post-publish changes go to the bags +(``get_context().override``), a value one runner or worker owns travels as a +constructor argument, and late launcher-stage resolution writes in place +through ``arg_groups.overrides.declare_late_resolution``, which refuses the +published instance. The textual half of this guard matters because the resolution pipeline's own file is exempt from the mutation ratchet: a ``self.override(...)`` there — exactly @@ -41,11 +42,19 @@ _EXCLUDED = ("multimodal_gen",) class TestNoServerArgsMutationEntry(CustomTestCase): - def test_the_method_is_gone(self): + def test_the_methods_are_gone(self): self.assertFalse( hasattr(ServerArgs, "override"), "ServerArgs.override is back; post-publish changes belong on the bags " - "(get_context().override) and per-runner values on a derive() variant.", + "(get_context().override); a value one runner owns travels as a " + "constructor argument.", + ) + self.assertFalse( + hasattr(ServerArgs, "derive"), + "ServerArgs.derive is back; a value one runner or worker owns travels " + "as a constructor argument (draft_attention_backend, MMEncoder " + "gpu_id), and test doubles copy via " + "sglang.test.test_utils.server_args_variant.", ) def test_nothing_calls_an_instance_override(self): @@ -63,11 +72,31 @@ class TestNoServerArgsMutationEntry(CustomTestCase): self.fail( "in-place ServerArgs mutation call-sites:\n" + "\n".join(offenders) - + "\n\nUse get_context().override(source, ...) for resolved config, " - "server_args.derive(source, ...) for a per-runner variant, or " - "declare_late_resolution(...) for pre-publish launcher resolution." + + "\n\nUse get_context().override(source, ...) for resolved config " + "or declare_late_resolution(...) for pre-publish launcher " + "resolution." ) + def test_nothing_derives(self): + """A config is never copied-and-edited in the package: a value one + runner consumes travels as a constructor argument, and test doubles + copy via ``server_args_variant`` (test_utils).""" + derive_pattern = re.compile(r"\.derive\(") + offenders = [] + for path in sorted(_SGLANG_ROOT.rglob("*.py")): + rel = path.relative_to(_SGLANG_ROOT).as_posix() + if rel.startswith(_EXCLUDED): + continue + source = path.read_text() + for match in derive_pattern.finditer(source): + line = source.count("\n", 0, match.start()) + 1 + offenders.append(f"{rel}:{line}") + self.assertFalse( + offenders, + ".derive( call-sites in the package (the method no longer exists):\n" + + "\n".join(offenders), + ) + def test_late_resolution_refuses_the_published_config(self): from sglang.srt.arg_groups.overrides import declare_late_resolution from sglang.srt.runtime_context import get_context