diff --git a/python/sglang/srt/arg_groups/overrides.py b/python/sglang/srt/arg_groups/overrides.py index 8cf29de05..ad944e23e 100644 --- a/python/sglang/srt/arg_groups/overrides.py +++ b/python/sglang/srt/arg_groups/overrides.py @@ -250,7 +250,13 @@ def declare_direct_writes( for field in dataclasses.fields(server_args) } already = len(getattr(server_args, "_resolved_overrides", None) or ()) - result = resolve(server_args) + # The one place the input seal comes off. The plugin writes the record; + # the diff below captures what it moved into the stash so the projection + # and the bags carry it. + from sglang.srt.server_args import record_writable + + with record_writable(server_args): + result = resolve(server_args) stash = getattr(server_args, "_resolved_overrides", None) if stash is None: stash = [] diff --git a/python/sglang/srt/entrypoints/engine.py b/python/sglang/srt/entrypoints/engine.py index b103775b0..ca4c73f37 100644 --- a/python/sglang/srt/entrypoints/engine.py +++ b/python/sglang/srt/entrypoints/engine.py @@ -261,6 +261,14 @@ class Engine(EngineScoreMixin, EngineBase): # Do not print logs by default kwargs["log_level"] = "error" server_args = self.server_args_class(**kwargs) + # There was no command line, so the call is what the operator + # asked for. `log_level` is filled in above when absent, so it + # shows here even when the caller did not pass it. + object.__setattr__( + server_args, + "_launch_command", + "Engine(" + ", ".join(f"{k}={v!r}" for k, v in kwargs.items()) + ")", + ) self.server_args = server_args logger.info(f"server_args={server_args.resolved_dict()}") @@ -1362,6 +1370,7 @@ class Engine(EngineScoreMixin, EngineBase): return msgspec_to_builtins( { **self.tokenizer_manager.server_args.resolved_dict(), + "launch_command": self.tokenizer_manager.server_args.launch_command, **self._scheduler_init_result.scheduler_infos[0], "startup_time": self.tokenizer_manager.startup_time, "internal_states": internal_states, diff --git a/python/sglang/srt/entrypoints/grpc_bridge.py b/python/sglang/srt/entrypoints/grpc_bridge.py index 50870c93c..48d5b0b8d 100644 --- a/python/sglang/srt/entrypoints/grpc_bridge.py +++ b/python/sglang/srt/entrypoints/grpc_bridge.py @@ -425,6 +425,11 @@ class RuntimeHandle: def get_server_info(self) -> str: result: Dict[str, Any] = self.tokenizer_manager.server_args.resolved_dict() + # `resolved_dict` answers with what resolution decided; the launch + # command answers with what was asked for, and the two are not + # derivable from each other. The HTTP and in-process readbacks both + # carry it, so this one does too. + result["launch_command"] = self.tokenizer_manager.server_args.launch_command result.update(self.scheduler_info) result["kv_events"] = describe_kv_events_publisher( self.tokenizer_manager.server_args diff --git a/python/sglang/srt/entrypoints/http_server.py b/python/sglang/srt/entrypoints/http_server.py index 18c865af4..8e73b9d1d 100644 --- a/python/sglang/srt/entrypoints/http_server.py +++ b/python/sglang/srt/entrypoints/http_server.py @@ -812,8 +812,10 @@ async def get_server_info(): async def server_info(): """The startup configuration, plus live scheduler state. - The values here are the resolution result: what the launcher was given, - with every decision resolution made applied over it. Fields the control plane changes + Two surfaces, deliberately both: the field values are the resolution + result -- what the launcher was given with every decision resolution made + applied over it -- and `launch_command` is what was actually asked for, + which no amount of reading the resolved values recovers. Fields the control plane changes after publication -- the model a weight update swapped in, its load format, an operator-set weight version -- are reported by `/model_info`, and the HiCache mirror by `GET /hicache/storage-backend`. @@ -828,6 +830,7 @@ async def server_info(): return msgspec_to_builtins( { **server_args.resolved_dict(), + "launch_command": server_args.launch_command, **_global_state.scheduler_info, "startup_time": _global_state.tokenizer_manager.startup_time, "internal_states": internal_states, diff --git a/python/sglang/srt/server_args.py b/python/sglang/srt/server_args.py index 4bb8d857c..4b96ceb26 100644 --- a/python/sglang/srt/server_args.py +++ b/python/sglang/srt/server_args.py @@ -40,6 +40,7 @@ import functools import logging import tempfile import uuid +from contextlib import contextmanager from typing import Any, Dict, List, Optional from sglang.kernels.ops.kv_canary.consts import RealKvHashMode @@ -269,6 +270,11 @@ class ServerArgs: ) from sglang.srt.arg_groups.pipeline import run_resolution_pipeline + # Sealed for the duration, not just afterwards: everything below this + # line reads the input and declares against it, and the one channel + # that still writes the record (`declare_direct_writes`, for + # out-of-tree platform plugins) asks for the seal to be lifted by name. + self._input_frozen = True try: run_resolution_pipeline(self) except BaseException: @@ -276,11 +282,30 @@ class ServerArgs: # idempotent over their own output. self._resolution_failed = True raise + finally: + self._input_frozen = False # Set here too, because the dummy/absent-model path returns before the # end of the pipeline that normally sets it: the gate is about whether # the handlers ran, not how far they got. self._resolution_finished = True + @property + def launch_command(self) -> Optional[str]: + """How this record was created, verbatim. + + `resolved_dict` answers with what resolution decided; this answers with + what the operator asked for, which is a different question and the one + "why is this server configured like this?" usually means. The two are + not derivable from each other: a field the operator never set reads the + same as one they set to the value resolution would have picked anyway. + + The launcher stores the arguments it parsed; the in-process `Engine` + stores the call that built the record, since there was no command line. + `None` on a record built directly (a fixture, a subprocess copy that + predates this, a config being inspected). + """ + return getattr(self, "_launch_command", None) + def resolved_dict(self) -> Dict[str, Any]: """This configuration as a plain dict of resolved field values. @@ -318,6 +343,9 @@ class ServerArgs: copy's deep structure in-process mutates the parent's too. """ replacement = dataclasses.replace(self, **changes) + # Provenance, not resolution state: a copy was still launched by + # whatever launched its parent, resolved or not. + object.__setattr__(replacement, "_launch_command", self.launch_command) if not getattr(self, "_resolution_finished", False): # Not resolved yet: the copy goes through the gate itself. return replacement @@ -669,19 +697,28 @@ class ServerArgs: return cfg.startup_weight_load_mode == "overlap" def __setattr__(self, name, value): - # Once resolution has finished the record is the READ-ONLY raw input - # the config bags were projected from. Resolved config changes go to the bags via - # get_context().override(source, ...); a value one runner or worker - # owns travels as a constructor argument to it. - if getattr(self, "_resolution_finished", False) and ( - not name.startswith("_") or name in _underscore_field_names() - ): - raise AttributeError( - f"server_args.{name} assigned after resolution; server_args is " - "read-only -- use get_context().override(source, ...) to change " - "resolved config; a value one runner owns travels as a " - "constructor argument." - ) + # The record holds the operator's input. It is writable while the + # caller is still assembling it and sealed from the moment resolution + # starts: a resolver that writes a field would overwrite the very thing + # the record exists to remember, and the decision it meant to record + # belongs in the stash, where it carries a source and does not destroy + # the input it was derived from. + if not name.startswith("_") or name in _underscore_field_names(): + if getattr(self, "_input_frozen", False): + raise AttributeError( + f"server_args.{name} assigned during resolution; the record " + "is the operator's input and resolution does not write it -- " + "declare the decision with declare_resolution(server_args, " + "source, **fields) so it carries a source and leaves the " + "input intact." + ) + if getattr(self, "_resolution_finished", False): + raise AttributeError( + f"server_args.{name} assigned after resolution; server_args is " + "read-only -- use get_context().override(source, ...) to change " + "resolved config; a value one runner owns travels as a " + "constructor argument." + ) object.__setattr__(self, name, value) def enable_mamba_extra_buffer(self) -> bool: @@ -834,6 +871,27 @@ def get_global_server_args() -> ServerArgs: return get_context().server_args +@contextmanager +def record_writable(server_args: Any): + """Lift the input seal for a resolver that genuinely writes the record. + + There is exactly one: `declare_direct_writes`, which hands the record to an + out-of-tree platform plugin that sets fields on it. Those implementations + live outside this tree and cannot be converted by editing a resolver here, + so the write stays and is captured into the stash afterwards. Naming the + exception is the point -- an in-tree resolver that reaches for this is + doing something it should be declaring instead. + """ + frozen = getattr(server_args, "_input_frozen", False) + if frozen: + object.__setattr__(server_args, "_input_frozen", False) + try: + yield + finally: + if frozen: + object.__setattr__(server_args, "_input_frozen", True) + + def prepare_server_args(argv: List[str]) -> ServerArgs: """ Prepare the server arguments from the command line arguments. @@ -868,7 +926,12 @@ def prepare_server_args(argv: List[str]) -> ServerArgs: force=True, ) - return ServerArgs.from_cli_args(raw_args) + server_args = ServerArgs.from_cli_args(raw_args) + # Not a field: the record's fields are the configuration, and this is how + # the configuration was asked for. It rides along on the record so a + # subprocess copy can answer the same question the launcher can. + object.__setattr__(server_args, "_launch_command", " ".join(argv)) + return server_args # -------------------------------------------------------------------------- diff --git a/test/registered/unit/server_args/test_server_args.py b/test/registered/unit/server_args/test_server_args.py index 971fba6c9..7e2ed387a 100644 --- a/test/registered/unit/server_args/test_server_args.py +++ b/test/registered/unit/server_args/test_server_args.py @@ -2,6 +2,7 @@ import argparse import dataclasses import json import os +import pickle import shutil import socket import tempfile @@ -2934,6 +2935,121 @@ class TestDcpKvEventContract(CustomTestCase): self.assertEqual(kv_event_block_size_of(resolving_view(args)), 8) +class TestTheInputIsSealedDuringResolution(CustomTestCase): + """The record holds what the operator asked for, and resolution does not + write it -- enforced, not merely observed. + + The guard used to arm only once resolution had *finished*, so for the whole + length of the pipeline nothing stopped a resolver from assigning a field. + Nothing in-tree did, but a resolver that started to would overwrite the + input the record exists to remember, and the defect is invisible: the value + it wrote is indistinguishable from a value the operator typed. + """ + + def test_a_write_before_resolution_is_fine(self): + """Callers assemble the record however they like.""" + server_args = ServerArgs(model_path="/tmp/x") + server_args.tp_size = 2 + self.assertEqual(server_args.tp_size, 2) + + def test_a_write_during_resolution_is_refused(self): + server_args = ServerArgs(model_path="dummy", device="cuda") + # The seal is what the pipeline runs under; drive it directly rather + # than injecting a violation into a real handler. + object.__setattr__(server_args, "_input_frozen", True) + with self.assertRaisesRegex(AttributeError, "during resolution"): + server_args.tp_size = 4 + # and the message says what to do instead + try: + server_args.tp_size = 4 + except AttributeError as caught: + self.assertIn("declare_resolution", str(caught)) + + def test_the_seal_comes_off_when_resolution_ends(self): + """`_resolution_finished` takes over; the two messages are different + because the fix is different.""" + server_args = ServerArgs(model_path="dummy", device="cuda") + server_args.resolve_once() + self.assertFalse(getattr(server_args, "_input_frozen", False)) + with self.assertRaisesRegex(AttributeError, "after resolution"): + server_args.tp_size = 4 + + def test_the_named_exception_lifts_it(self): + """`declare_direct_writes` hands the record to an out-of-tree platform + plugin that sets fields on it; that is the only channel.""" + from sglang.srt.server_args import record_writable + + server_args = ServerArgs(model_path="dummy", device="cuda") + object.__setattr__(server_args, "_input_frozen", True) + with record_writable(server_args): + server_args.tp_size = 4 + self.assertEqual(server_args.tp_size, 4) + # and it goes back on afterwards + with self.assertRaisesRegex(AttributeError, "during resolution"): + server_args.tp_size = 8 + + def test_a_failed_resolution_does_not_leave_it_sealed(self): + """A record that failed resolution is still the operator's input, and + `resolve_once` already refuses to re-run on it. Leaving the seal armed + would make the failure look like a different one to anyone inspecting + the record afterwards.""" + server_args = ServerArgs( + model_path="dummy", device="cuda", prefill_decode_interval=-5 + ) + with self.assertRaisesRegex(ValueError, "prefill-decode-interval"): + server_args.resolve_once() + self.assertFalse(getattr(server_args, "_input_frozen", False)) + + +class TestLaunchCommand(CustomTestCase): + """The record answers what was asked for, not only what was decided. + + `resolved_dict` and `launch_command` are different questions, and neither + recovers the other: a field the operator never set resolves to the same + value as one they set to what resolution would have picked anyway. + """ + + def test_the_launcher_records_what_it_parsed(self): + server_args = prepare_server_args( + ["--model-path", "/tmp/x", "--tp-size", "2", "--log-level", "warning"] + ) + self.assertEqual( + server_args.launch_command, + "--model-path /tmp/x --tp-size 2 --log-level warning", + ) + + def test_a_record_nobody_launched_has_no_command(self): + self.assertIsNone(ServerArgs(model_path="/tmp/x").launch_command) + + def test_it_crosses_a_process_boundary(self): + """The scheduler and detokenizer get the record by pickle, and they + answer `/server_info` for their own process.""" + server_args = prepare_server_args(["--model-path", "/tmp/x"]) + self.assertEqual( + pickle.loads(pickle.dumps(server_args)).launch_command, + server_args.launch_command, + ) + + def test_a_copy_keeps_it(self): + """`replace_resolved` is how the Ray paths rewrite `dist_init_addr`; + the copy was launched by whatever launched its parent.""" + server_args = prepare_server_args(["--model-path", "/tmp/x"]) + self.assertEqual( + server_args.replace_resolved("test").launch_command, + server_args.launch_command, + ) + + def test_it_is_not_a_config_field(self): + """It describes how the configuration was asked for, so it is not part + of the configuration: no CLI flag, no namespace, not in the bags.""" + self.assertNotIn( + "launch_command", {f.name for f in dataclasses.fields(ServerArgs)} + ) + self.assertNotIn( + "launch_command", ServerArgs(model_path="/tmp/x").resolved_dict() + ) + + class TestNoneMeansUnset(CustomTestCase): """A valued field the resolution rewrites carries `None` for "not set".