[Config] Round 6.3: the record remembers how it was asked for, and is sealed while resolution runs (#38048)
Third of four; stacked on #38047. Two small changes, both about the same thing: the record holds the operator's input, and nothing else should be true of it. ## `/server_info` can answer what was actually typed It reports `resolved_dict()` -- what resolution decided. There was no way to ask the other question, and the two are not derivable from each other: a field nobody set reads the same as one set to the value resolution would have picked anyway. The launcher stores the arguments it parsed and the in-process `Engine` stores the call that built the record. All three readbacks report it beside the resolved values, so both surfaces come back in one request: HTTP `/server_info`, `Engine.get_server_info`, and the gRPC bridge's -- the last one builds from `resolved_dict()` and would otherwise have been the one surface of the three that answers only "what resolution decided". It rides on the record rather than in a field -- 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. Being on the record is what lets a subprocess copy answer the same question the launcher can, and `replace_resolved` carries it because a copy was launched by whatever launched its parent. The crash dump already collected all four surfaces (`server_args`, `config_updates`, `resolved_config`, `launch_command`); this is the one that `/server_info` was missing. ## The record is sealed for the length of resolution The read-only guard armed on `_resolution_finished`, so for the whole run of the pipeline nothing stopped a resolver from assigning a field. Nothing in `srt/` does -- 0 assignments statically, and 0 writes observed across the launch-shape matrix with a watching `__setattr__` -- but that was a convention, and the defect it permits is invisible: a value a resolver wrote onto the record is indistinguishable from a value the operator typed, which is the one distinction the record exists to preserve. It now arms when resolution starts. A resolver that assigns a field fails at boot with a message naming `declare_resolution`, which is where the decision belongs: the stash carries a source and leaves the input intact. `declare_direct_writes` asks for the seal by name through `record_writable`. It hands the record to an out-of-tree platform plugin that sets fields on it; those implementations cannot be converted by editing a resolver here, so the write stays and the diff is captured into the stash afterwards. Naming the exception is the point -- an in-tree resolver reaching for it is doing something it should be declaring. ## Verification Costs nothing: the 211 test-side assignments all happen before `resolve_once`, which a post-resolution write already refused. A full registered-unit sweep (648 files) against the stack's merge-base: 19 failures on both sides, the same 19, none of them config. Driving a deliberate write into a real handler produces the new error, so the seal is tested by more than its own unit test.
This commit is contained in:
@@ -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".
|
||||
|
||||
|
||||
Reference in New Issue
Block a user