config: the record is not an object that gets passed around (#36622)
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
fd40a331bf
commit
6ff2a20ccf
@@ -40,7 +40,7 @@ def _make_dspark_server_args(
|
||||
server_args.speculative_algorithm = "DSPARK"
|
||||
server_args.speculative_draft_model_path = None
|
||||
server_args.speculative_dspark_block_size = 5
|
||||
server_args.model_config = SimpleNamespace(hf_config=hf_config)
|
||||
server_args._model_config = SimpleNamespace(hf_config=hf_config)
|
||||
return server_args
|
||||
|
||||
|
||||
|
||||
@@ -72,7 +72,7 @@ class TestMultimodalPiecewiseCudaGraph(CustomTestCase):
|
||||
|
||||
def test_supported_multimodal_model_upgrades_default_to_tc_piecewise(self):
|
||||
args = ServerArgs(model_path="dummy")
|
||||
args.model_config = SimpleNamespace(
|
||||
args._model_config = SimpleNamespace(
|
||||
is_multimodal_piecewise_cuda_graph_supported=True,
|
||||
is_multimodal_breakable_cuda_graph_supported=False,
|
||||
)
|
||||
@@ -101,7 +101,7 @@ class TestMultimodalPiecewiseCudaGraph(CustomTestCase):
|
||||
args = ServerArgs(model_path="dummy")
|
||||
# trtllm_mla skips the tc_piecewise upgrade and keeps breakable, which
|
||||
# now serves MLA by falling back to the flashinfer MLA impl for extend.
|
||||
args.model_config = SimpleNamespace(
|
||||
args._model_config = SimpleNamespace(
|
||||
is_multimodal_piecewise_cuda_graph_supported=True,
|
||||
is_multimodal=False,
|
||||
is_multimodal_breakable_cuda_graph_supported=False,
|
||||
@@ -169,7 +169,7 @@ class TestMultimodalPiecewiseCudaGraph(CustomTestCase):
|
||||
|
||||
def test_embedding_gemma_forces_breakable_prefill(self):
|
||||
args = ServerArgs(model_path="dummy")
|
||||
args.model_config = SimpleNamespace(
|
||||
args._model_config = SimpleNamespace(
|
||||
is_embedding_gemma=True,
|
||||
is_multimodal=False,
|
||||
context_len=2048,
|
||||
@@ -183,7 +183,7 @@ class TestMultimodalPiecewiseCudaGraph(CustomTestCase):
|
||||
args.chunked_prefill_size = 2048
|
||||
|
||||
with (
|
||||
patch.object(args, "get_model_config", return_value=args.model_config),
|
||||
patch.object(args, "get_model_config", return_value=args._model_config),
|
||||
patch("sglang.srt.server_args.is_cuda", return_value=True),
|
||||
):
|
||||
args._handle_model_capability_adjustments()
|
||||
@@ -202,7 +202,7 @@ class TestMultimodalPiecewiseCudaGraph(CustomTestCase):
|
||||
def test_encoder_embedding_model_enables_embedding_mode_without_flag(self):
|
||||
args = ServerArgs(model_path="dummy")
|
||||
args.is_embedding = False
|
||||
args.model_config = SimpleNamespace(
|
||||
args._model_config = SimpleNamespace(
|
||||
embedding_model_spec=resolve_embedding_model_spec(
|
||||
["BertModel"],
|
||||
is_embedding_requested=False,
|
||||
@@ -212,7 +212,7 @@ class TestMultimodalPiecewiseCudaGraph(CustomTestCase):
|
||||
hf_config=SimpleNamespace(architectures=["BertModel"]),
|
||||
)
|
||||
|
||||
with patch.object(args, "get_model_config", return_value=args.model_config):
|
||||
with patch.object(args, "get_model_config", return_value=args._model_config):
|
||||
args._handle_model_capability_adjustments()
|
||||
|
||||
self.assertTrue(resolution_result(args, "is_embedding"))
|
||||
|
||||
@@ -116,7 +116,6 @@ class TestDecodeRetractionBackup(unittest.TestCase):
|
||||
mode=HiCacheDraftMode.SIDECAR,
|
||||
device_pools=(draft_pool,),
|
||||
),
|
||||
server_args=server_args,
|
||||
)
|
||||
self.assertIn(PoolName.DRAFT, cache.host_pool_group.entry_map)
|
||||
cache.validate_retraction_host_capacity()
|
||||
|
||||
@@ -86,7 +86,6 @@ class TestDraftSidecarPoolDispatch(CustomTestCase):
|
||||
specs, entries = build_full_draft_pools(
|
||||
draft_kv_pool=draft_kv_pool,
|
||||
tree_cache=None,
|
||||
server_args=None,
|
||||
)
|
||||
|
||||
self.assertEqual(specs, [])
|
||||
@@ -124,7 +123,6 @@ class TestDraftSidecarPoolDispatch(CustomTestCase):
|
||||
specs, entries = build_full_draft_pools(
|
||||
draft_kv_pool=draft_kv_pool,
|
||||
tree_cache=tree_cache,
|
||||
server_args=server_args,
|
||||
)
|
||||
|
||||
self.assertEqual(build_host_pool.call_args.kwargs["host_to_device_ratio"], 1.0)
|
||||
|
||||
@@ -110,7 +110,7 @@ class TestTheModelConfigCache(CustomTestCase):
|
||||
server_args = self._resolved(
|
||||
model_path=_OBJECT_STORE_URI, load_format="runai_streamer"
|
||||
)
|
||||
cached = server_args.__dict__["model_config"]
|
||||
cached = server_args.__dict__["_model_config"]
|
||||
self.assertIsInstance(cached, ModelConfig)
|
||||
# The record still carries the URI the operator typed, and the
|
||||
# configuration carries the directory it read the metadata from.
|
||||
@@ -162,7 +162,7 @@ class TestTheModelConfigCache(CustomTestCase):
|
||||
invalidates it."""
|
||||
server_args = ServerArgs(model_path=self._checkpoint(), device="cuda")
|
||||
stand_in = SimpleNamespace(model_path="somewhere/else")
|
||||
server_args.model_config = stand_in
|
||||
server_args._model_config = stand_in
|
||||
|
||||
self.assertIs(server_args.get_model_config(), stand_in)
|
||||
|
||||
|
||||
@@ -0,0 +1,83 @@
|
||||
"""The record grows no attribute the projection cannot see.
|
||||
|
||||
A publicly-named attribute that is not a dataclass field is invisible to every
|
||||
other guard here: the namespace coverage walks fields, the projection walks
|
||||
fields, and the read ratchets watch field reads. Three of them accumulated that
|
||||
way -- a `ModelConfig` cache, an `moe_ep_size` that only a log line read, and an
|
||||
env-derived `grpc_worker_threads` that one entry point read across the boundary.
|
||||
|
||||
Leading-underscore names are the record's own bookkeeping and stay: the
|
||||
read-only guard classifies writability by that spelling, so a private name is
|
||||
already outside the config tier by construction.
|
||||
"""
|
||||
|
||||
import ast
|
||||
import dataclasses
|
||||
import pathlib
|
||||
import unittest
|
||||
|
||||
import sglang
|
||||
from sglang.srt.server_args import ServerArgs
|
||||
from sglang.test.ci.ci_register import register_cpu_ci
|
||||
from sglang.test.test_utils import CustomTestCase
|
||||
|
||||
register_cpu_ci(est_time=4, suite="base-a-test-cpu")
|
||||
|
||||
|
||||
def _self_written_attributes() -> set:
|
||||
"""Names `ServerArgs` writes on itself, by either spelling."""
|
||||
source = (
|
||||
pathlib.Path(next(iter(sglang.__path__))) / "srt" / "server_args.py"
|
||||
).read_text(encoding="utf-8-sig")
|
||||
tree = ast.parse(source)
|
||||
cls = next(
|
||||
node
|
||||
for node in tree.body
|
||||
if isinstance(node, ast.ClassDef) and node.name == "ServerArgs"
|
||||
)
|
||||
written = set()
|
||||
for node in ast.walk(cls):
|
||||
if isinstance(node, ast.Assign):
|
||||
for target in node.targets:
|
||||
if (
|
||||
isinstance(target, ast.Attribute)
|
||||
and isinstance(target.value, ast.Name)
|
||||
and target.value.id == "self"
|
||||
):
|
||||
written.add(target.attr)
|
||||
if (
|
||||
isinstance(node, ast.Call)
|
||||
and getattr(node.func, "attr", None) == "__setattr__"
|
||||
and getattr(getattr(node.func, "value", None), "id", None) == "object"
|
||||
and len(node.args) >= 2
|
||||
and isinstance(node.args[1], ast.Constant)
|
||||
):
|
||||
written.add(node.args[1].value)
|
||||
return written
|
||||
|
||||
|
||||
class TestNoPublicNonFieldSlot(CustomTestCase):
|
||||
def test_every_public_attribute_is_a_field(self):
|
||||
written = _self_written_attributes()
|
||||
self.assertGreater(
|
||||
len(written),
|
||||
5,
|
||||
f"only {len(written)} self-writes found; the scan is broken, not the "
|
||||
"record",
|
||||
)
|
||||
fields = {field.name for field in dataclasses.fields(ServerArgs)}
|
||||
stray = sorted(
|
||||
name for name in written if not name.startswith("_") and name not in fields
|
||||
)
|
||||
self.assertEqual(
|
||||
[],
|
||||
stray,
|
||||
"these are written on the record under a public name but are not "
|
||||
"fields, so the projection cannot see them and no other guard "
|
||||
"watches them: make each a field, or give it the leading underscore "
|
||||
f"that says it is the record's own bookkeeping: {stray}",
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
@@ -283,7 +283,7 @@ class TestImageProcessorBackend(CustomTestCase):
|
||||
class TestMultimodalFeatureTransport(CustomTestCase):
|
||||
@staticmethod
|
||||
def _set_model_type(server_args, *, is_multimodal):
|
||||
server_args.model_config = SimpleNamespace(is_multimodal=is_multimodal)
|
||||
server_args._model_config = SimpleNamespace(is_multimodal=is_multimodal)
|
||||
|
||||
@patch("sglang.srt.server_args.is_cuda", return_value=True)
|
||||
def test_cuda_ipc_is_explicit_and_bounded(self, _mock_is_cuda):
|
||||
@@ -920,8 +920,8 @@ class TestFa4PageSizeAutoForce(CustomTestCase):
|
||||
# use_mla_backend() (mocked) and is_sm100_supported() (mocked), not a
|
||||
# real model_config. Pre-set the attribute so get_model_config returns
|
||||
# early without touching ModelConfig.from_server_args.
|
||||
args.model_config = MagicMock()
|
||||
args.model_config.hf_config.dual_chunk_attention_config = None
|
||||
args._model_config = MagicMock()
|
||||
args._model_config.hf_config.dual_chunk_attention_config = None
|
||||
return args
|
||||
|
||||
@patch("sglang.srt.arg_groups.overrides.is_sm100_supported", return_value=True)
|
||||
@@ -1809,7 +1809,7 @@ class TestCudaGraphConfigDataclassAccess(CustomTestCase):
|
||||
class TestCudaGraphDisaggregationRoles(CustomTestCase):
|
||||
def _handled_args(self, **overrides):
|
||||
args = ServerArgs(model_path="dummy", **overrides)
|
||||
args.model_config = SimpleNamespace(
|
||||
args._model_config = SimpleNamespace(
|
||||
hf_config=SimpleNamespace(architectures=["LlamaForCausalLM"]),
|
||||
is_piecewise_cuda_graph_disabled_model=False,
|
||||
is_multimodal=False,
|
||||
@@ -1882,7 +1882,7 @@ class TestPrefillCudaGraphLoRACompatibility(CustomTestCase):
|
||||
|
||||
def _handled_args(self, **overrides):
|
||||
args = ServerArgs(model_path="dummy", **overrides)
|
||||
args.model_config = SimpleNamespace(
|
||||
args._model_config = SimpleNamespace(
|
||||
hf_config=SimpleNamespace(architectures=["LlamaForCausalLM"]),
|
||||
is_piecewise_cuda_graph_disabled_model=False,
|
||||
is_multimodal=False,
|
||||
@@ -1915,7 +1915,7 @@ class TestPrefillCudaGraphLoRACompatibility(CustomTestCase):
|
||||
# Pin the tc_piecewise LoRA rule itself, with the hardware rule
|
||||
# neutralized so this runs on CPU-only CI.
|
||||
args = ServerArgs(model_path="dummy", enable_lora=True)
|
||||
args.model_config = SimpleNamespace(
|
||||
args._model_config = SimpleNamespace(
|
||||
hf_config=SimpleNamespace(architectures=["LlamaForCausalLM"]),
|
||||
is_piecewise_cuda_graph_disabled_model=False,
|
||||
is_multimodal=False,
|
||||
@@ -1945,7 +1945,7 @@ class TestBreakableCudaGraphMultimodalAllowlist(CustomTestCase):
|
||||
|
||||
def _handled_args(self, *, architectures, is_multimodal, allowlisted):
|
||||
args = ServerArgs(model_path="dummy")
|
||||
args.model_config = SimpleNamespace(
|
||||
args._model_config = SimpleNamespace(
|
||||
hf_config=SimpleNamespace(architectures=architectures),
|
||||
is_piecewise_cuda_graph_disabled_model=False,
|
||||
is_multimodal=is_multimodal,
|
||||
@@ -2129,7 +2129,7 @@ class TestDeepEPv2Args(CustomTestCase):
|
||||
|
||||
def _args(self, **overrides):
|
||||
server_args = ServerArgs(model_path="dummy", moe_a2a_backend="deepep_v2")
|
||||
server_args.model_config = SimpleNamespace(
|
||||
server_args._model_config = SimpleNamespace(
|
||||
hf_config=SimpleNamespace(architectures=["DeepseekV4ForCausalLM"])
|
||||
)
|
||||
# The dummy path does not initialize phase configs.
|
||||
@@ -2152,7 +2152,7 @@ class TestDeepEPv2Args(CustomTestCase):
|
||||
"Qwen3MoeForCausalLM",
|
||||
):
|
||||
args = self._args(moe_runner_backend="deep_gemm")
|
||||
args.model_config.hf_config.architectures = [architecture]
|
||||
args._model_config.hf_config.architectures = [architecture]
|
||||
args._handle_a2a_moe()
|
||||
|
||||
def test_unvalidated_and_missing_architectures_rejected(self):
|
||||
@@ -2163,7 +2163,7 @@ class TestDeepEPv2Args(CustomTestCase):
|
||||
None,
|
||||
):
|
||||
args = self._args(moe_runner_backend="deep_gemm")
|
||||
args.model_config.hf_config.architectures = architectures
|
||||
args._model_config.hf_config.architectures = architectures
|
||||
with self.assertRaisesRegex(ValueError, "not validated"):
|
||||
args._handle_a2a_moe()
|
||||
|
||||
@@ -2188,7 +2188,7 @@ class TestDeepEPv2Args(CustomTestCase):
|
||||
moe_runner_backend="deep_gemm",
|
||||
rl_on_policy_target="fsdp",
|
||||
)
|
||||
args.model_config.hf_config.architectures = ["Qwen3MoeForCausalLM"]
|
||||
args._model_config.hf_config.architectures = ["Qwen3MoeForCausalLM"]
|
||||
with (
|
||||
envs.SGLANG_VLM_CACHE_SIZE_MB.override(envs.SGLANG_VLM_CACHE_SIZE_MB.get()),
|
||||
envs.SGLANG_ENABLE_DETERMINISTIC_INFERENCE.override(
|
||||
@@ -2514,7 +2514,7 @@ class TestGrpcServerArgs(CustomTestCase):
|
||||
with envs.SGLANG_GRPC_WORKER_THREADS.override(8):
|
||||
sa._handle_deprecated_args()
|
||||
self.assertEqual(resolution_result(sa, "grpc_port"), 50051)
|
||||
self.assertEqual(sa.grpc_worker_threads, 8)
|
||||
self.assertEqual(resolution_result(sa, "grpc_worker_threads"), 8)
|
||||
|
||||
def test_env_grpc_port_enables_native(self):
|
||||
sa = self._args(port=30000)
|
||||
@@ -2698,13 +2698,11 @@ class TestGrpcServerArgs(CustomTestCase):
|
||||
|
||||
fake_core = SimpleNamespace(start_server=MagicMock(return_value="handle"))
|
||||
fake_bridge = SimpleNamespace(RuntimeHandle=MagicMock(return_value="rt"))
|
||||
# The host comes from the `serving` bag; `grpc_worker_threads` is not a
|
||||
# field (resolution sets it from the environment), so it stays on the
|
||||
# stand-in the call site is handed.
|
||||
override = get_context().override_server_args(host="127.0.0.1", grpc_port=50051)
|
||||
override.install()
|
||||
override = get_context().override_server_args(
|
||||
host="127.0.0.1", grpc_port=50051, grpc_worker_threads=4
|
||||
)
|
||||
server_args = override.install()
|
||||
self.addCleanup(override.restore)
|
||||
server_args = SimpleNamespace(grpc_worker_threads=4)
|
||||
with (
|
||||
patch(
|
||||
"sglang.srt.rust_extensions.load_rust_extension",
|
||||
@@ -2728,6 +2726,7 @@ class TestGrpcServerArgs(CustomTestCase):
|
||||
self.assertEqual(
|
||||
set(kwargs), {"host", "port", "runtime_handle", "worker_threads"}
|
||||
)
|
||||
self.assertEqual(kwargs["worker_threads"], 4)
|
||||
self.assertNotIn("max_prefill_tokens", kwargs)
|
||||
|
||||
|
||||
|
||||
@@ -0,0 +1,86 @@
|
||||
"""A function does not take the record it never reads.
|
||||
|
||||
A `server_args` parameter that the body never names keeps a reference to the
|
||||
whole record alive across a call boundary, and it reads as an invitation: the
|
||||
next person to need one value takes it off the parameter that is already there,
|
||||
instead of deciding where that value should come from. Removing one usually
|
||||
uncovers the next -- the caller that only had a record to pass it along.
|
||||
|
||||
Class methods are exempt: a base class, an override, or one implementation of a
|
||||
strategy carries the parameter for its contract, and the body of any single one
|
||||
of them is not evidence. This walks module-level functions only.
|
||||
"""
|
||||
|
||||
import ast
|
||||
import pathlib
|
||||
import unittest
|
||||
|
||||
import sglang
|
||||
from sglang.test.ci.ci_register import register_cpu_ci
|
||||
from sglang.test.test_utils import CustomTestCase
|
||||
|
||||
register_cpu_ci(est_time=6, suite="base-a-test-cpu")
|
||||
|
||||
_PACKAGE_ROOT = pathlib.Path(next(iter(sglang.__path__)))
|
||||
|
||||
# The resolution pipeline builds the record, so a parameter there is the subject
|
||||
# rather than a passenger. `multimodal_gen` has a different, same-named class
|
||||
# outside this contract, as the mutation ratchet also records.
|
||||
_EXCLUDED = ("srt/arg_groups", "srt/server_args.py", "multimodal_gen")
|
||||
|
||||
_BASELINE = 0
|
||||
|
||||
|
||||
def _dead_parameters():
|
||||
found = []
|
||||
scanned = 0
|
||||
for path in sorted(_PACKAGE_ROOT.rglob("*.py")):
|
||||
rel = path.relative_to(_PACKAGE_ROOT).as_posix()
|
||||
if rel.startswith(_EXCLUDED):
|
||||
continue
|
||||
source = path.read_text(encoding="utf-8-sig")
|
||||
if "server_args" not in source:
|
||||
continue
|
||||
scanned += 1
|
||||
try:
|
||||
tree = ast.parse(source)
|
||||
except SyntaxError:
|
||||
continue
|
||||
for node in tree.body:
|
||||
if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
|
||||
continue
|
||||
taken = [a.arg for a in node.args.args] + [
|
||||
a.arg for a in node.args.kwonlyargs
|
||||
]
|
||||
if "server_args" not in taken:
|
||||
continue
|
||||
named = any(
|
||||
isinstance(inner, ast.Name) and inner.id == "server_args"
|
||||
for inner in ast.walk(node)
|
||||
if inner is not node
|
||||
)
|
||||
if not named:
|
||||
found.append(f"{rel}:{node.lineno} {node.name}")
|
||||
return found, scanned
|
||||
|
||||
|
||||
class TestNoDeadServerArgsParameter(CustomTestCase):
|
||||
def test_no_module_level_function_takes_a_record_it_ignores(self):
|
||||
found, scanned = _dead_parameters()
|
||||
self.assertGreater(
|
||||
scanned,
|
||||
50,
|
||||
f"only {scanned} files mention server_args; the scan is broken, not "
|
||||
"the tree",
|
||||
)
|
||||
self.assertEqual(
|
||||
_BASELINE,
|
||||
len(found),
|
||||
"these functions take `server_args` and never name it; drop the "
|
||||
"parameter and the argument at every call site, then check whether "
|
||||
f"the caller still needs its own: {found}",
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user