[Config] Round 5.2: the per-model declarations get their own modules (#37087)

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

test/registered/unit/server_args/test_model_source_paths.py

@@ -0,0 +1,326 @@
+"""The model-source axis, on PR CI.
+
+Four ways a model path can name something that is not a local directory, and
+until now only one of them was checked before merge:
+
+- an object-store URI (``s3://`` / ``gs://`` / ``az://``), covered by
+  ``test_model_config_cache.py`` and, end to end, by a ``nightly`` test;
+- a Hub reference to a ``.gguf`` file;
+- a ModelScope repo id;
+- a remote-connector URL, which is any other ``scheme://`` and is reached by a
+  different arm of ``ModelConfig`` than the object-store one.
+
+The last three had no registered test at all. That is how a change to
+``get_model_config()``'s cache semantics went green through PR CI and broke two
+days later in the nightly: the axis it broke was not being looked at.
+
+None of this needs a network. The GGUF arm asks one resolver for a local path,
+the ModelScope arm returns any path that already exists on disk untouched and
+otherwise goes through two imports that can be stood in for, and the
+remote-connector arm goes through one factory. Each case stubs exactly that
+seam and checks what the handler declares -- and, where the path moves, that
+the model-configuration cache notices.
+"""
+
+from sglang.test.ci.ci_register import register_cpu_ci
+
+register_cpu_ci(est_time=10, suite="base-a-test-cpu")
+
+import json
+import os
+import shutil
+import sys
+import tempfile
+import types
+import unittest
+from unittest import mock
+
+import sglang.srt.connector as connector_module
+from sglang.srt.arg_groups.model_path_hook import (
+    handle_modelscope_paths,
+    resolve_hf_gguf_model_path,
+)
+from sglang.srt.arg_groups.overrides import model_config_of, resolving_view
+from sglang.srt.configs.model_config import ModelConfig
+from sglang.srt.server_args import ServerArgs
+from sglang.test.test_utils import CustomTestCase
+
+_MINI_CONFIG = {
+    "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,
+}
+
+_GGUF_REFERENCE = "owner/repo"
+_MODELSCOPE_REPO = "org/model"
+_REMOTE_URL = "redis://host:6379/mini-llama"
+
+
+class _ModelSourceCase(CustomTestCase):
+    def _directory(self) -> str:
+        directory = tempfile.mkdtemp(prefix="model_source_")
+        self.addCleanup(shutil.rmtree, directory, ignore_errors=True)
+        return directory
+
+    def _checkpoint(self) -> str:
+        directory = self._directory()
+        with open(os.path.join(directory, "config.json"), "w") as handle:
+            json.dump(_MINI_CONFIG, handle)
+        return directory
+
+    def _gguf_file(self) -> str:
+        path = os.path.join(self._directory(), "model.gguf")
+        open(path, "w").close()
+        return path
+
+
+class TestTheGgufArm(_ModelSourceCase):
+    """`resolve_hf_gguf_model_path` turns a Hub reference into a local file."""
+
+    def _resolving_to(self, resolved):
+        """Stand in for the one Hub call, keyed on what it is asked about."""
+        table = resolved if isinstance(resolved, dict) else None
+
+        def _resolve(model, revision=None):
+            if table is not None:
+                return table.get(model)
+            return resolved
+
+        return mock.patch(
+            "sglang.srt.utils.hf_transformers_utils.resolve_hf_gguf_reference",
+            side_effect=_resolve,
+        )
+
+    def test_a_hub_reference_declares_the_local_path(self):
+        local = self._gguf_file()
+        server_args = ServerArgs(model_path=_GGUF_REFERENCE, device="cuda")
+        with self._resolving_to(local):
+            resolve_hf_gguf_model_path(server_args)
+
+        self.assertEqual(resolving_view(server_args).model_path, local)
+        # The record itself still carries what the operator typed.
+        self.assertEqual(server_args.model_path, _GGUF_REFERENCE)
+
+    def test_the_tokenizer_follows_only_when_it_was_the_same_reference(self):
+        local = self._gguf_file()
+        together = ServerArgs(
+            model_path=_GGUF_REFERENCE, tokenizer_path=_GGUF_REFERENCE, device="cuda"
+        )
+        with self._resolving_to(local):
+            resolve_hf_gguf_model_path(together)
+        self.assertEqual(resolving_view(together).tokenizer_path, local)
+
+        apart = ServerArgs(
+            model_path=_GGUF_REFERENCE, tokenizer_path="somewhere/else", device="cuda"
+        )
+        with self._resolving_to({_GGUF_REFERENCE: local}):
+            resolve_hf_gguf_model_path(apart)
+        self.assertEqual(resolving_view(apart).tokenizer_path, "somewhere/else")
+
+    def test_a_draft_gguf_is_resolved_on_its_own(self):
+        target, draft = self._gguf_file(), self._gguf_file()
+        server_args = ServerArgs(
+            model_path=_GGUF_REFERENCE,
+            speculative_draft_model_path="owner/draft",
+            device="cuda",
+        )
+        with self._resolving_to({_GGUF_REFERENCE: target, "owner/draft": draft}):
+            resolve_hf_gguf_model_path(server_args)
+
+        view = resolving_view(server_args)
+        self.assertEqual(view.model_path, target)
+        self.assertEqual(view.speculative_draft_model_path, draft)
+
+    def test_a_reference_that_is_not_a_gguf_declares_nothing(self):
+        server_args = ServerArgs(model_path=_GGUF_REFERENCE, device="cuda")
+        with self._resolving_to(None):
+            resolve_hf_gguf_model_path(server_args)
+        self.assertEqual(resolving_view(server_args).model_path, _GGUF_REFERENCE)
+
+    def test_the_declared_path_invalidates_the_model_configuration(self):
+        """The point of pinning the declaration: a configuration built before
+        it describes the Hub reference, not the file that was downloaded."""
+        first, second = self._checkpoint(), self._checkpoint()
+        server_args = ServerArgs(model_path=first, device="cuda")
+        before = model_config_of(server_args)
+        self.assertEqual(before.model_path, first)
+
+        with self._resolving_to(second):
+            resolve_hf_gguf_model_path(server_args)
+
+        after = model_config_of(server_args)
+        self.assertIsNot(after, before)
+        self.assertEqual(after.model_path, second)
+
+
+class TestTheModelScopeArm(_ModelSourceCase):
+    """`handle_modelscope_paths` resolves repo ids against the local cache."""
+
+    def _modelscope(self, cache_root: str, downloads: dict):
+        """Stand in for the two modules the handler imports on a cache miss."""
+        calls = []
+
+        def _snapshot_download(path, cache_dir=None, revision=None, **kwargs):
+            calls.append((path, cache_dir, revision, kwargs.get("ignore_patterns")))
+            return downloads[path]
+
+        hub = types.ModuleType("modelscope.hub.snapshot_download")
+        hub.snapshot_download = _snapshot_download
+        file_utils = types.ModuleType("modelscope.utils.file_utils")
+        file_utils.get_model_cache_root = lambda: cache_root
+        modules = {
+            "modelscope": types.ModuleType("modelscope"),
+            "modelscope.hub": types.ModuleType("modelscope.hub"),
+            "modelscope.hub.snapshot_download": hub,
+            "modelscope.utils": types.ModuleType("modelscope.utils"),
+            "modelscope.utils.file_utils": file_utils,
+        }
+        return mock.patch.dict(sys.modules, modules), calls
+
+    def test_a_path_already_on_disk_is_left_alone(self):
+        """And nothing is imported to decide that -- the arm has to stay usable
+        on a host with no modelscope installed."""
+        local = self._directory()
+        server_args = ServerArgs(model_path=local, tokenizer_path=local, device="cuda")
+        imported = {name for name in sys.modules if name.startswith("modelscope")}
+
+        handle_modelscope_paths(server_args)
+
+        view = resolving_view(server_args)
+        self.assertEqual(view.model_path, local)
+        self.assertEqual(view.tokenizer_path, local)
+        self.assertEqual(
+            imported, {name for name in sys.modules if name.startswith("modelscope")}
+        )
+
+    def test_a_repo_id_resolves_against_the_modelscope_cache(self):
+        cache_root = self._directory()
+        os.makedirs(os.path.join(cache_root, _MODELSCOPE_REPO))
+        patch, _ = self._modelscope(cache_root, {})
+        server_args = ServerArgs(
+            model_path=_MODELSCOPE_REPO, tokenizer_path=_MODELSCOPE_REPO, device="cuda"
+        )
+        with patch:
+            handle_modelscope_paths(server_args)
+
+        cached = os.path.join(cache_root, _MODELSCOPE_REPO)
+        view = resolving_view(server_args)
+        self.assertEqual(view.model_path, cached)
+        self.assertEqual(view.tokenizer_path, cached)
+
+    def test_a_cache_miss_downloads_and_the_tokenizer_skips_the_weights(self):
+        downloaded = self._directory()
+        patch, calls = self._modelscope(
+            self._directory(), {_MODELSCOPE_REPO: downloaded}
+        )
+        server_args = ServerArgs(
+            model_path=_MODELSCOPE_REPO, tokenizer_path=_MODELSCOPE_REPO, device="cuda"
+        )
+        with patch:
+            handle_modelscope_paths(server_args)
+
+        view = resolving_view(server_args)
+        self.assertEqual(view.model_path, downloaded)
+        self.assertEqual(view.tokenizer_path, downloaded)
+        # The tokenizer download does not drag the weights along with it.
+        self.assertEqual(
+            [call[3] for call in calls], [None, ["*.bin", "*.safetensors"]]
+        )
+
+    def test_the_download_directory_is_searched_before_the_hub(self):
+        download_dir = self._directory()
+        os.makedirs(os.path.join(download_dir, _MODELSCOPE_REPO))
+        patch, calls = self._modelscope(self._directory(), {})
+        server_args = ServerArgs(
+            model_path=_MODELSCOPE_REPO,
+            tokenizer_path=_MODELSCOPE_REPO,
+            download_dir=download_dir,
+            device="cuda",
+        )
+        with patch:
+            handle_modelscope_paths(server_args)
+
+        self.assertEqual(
+            resolving_view(server_args).model_path,
+            os.path.join(download_dir, _MODELSCOPE_REPO),
+        )
+        self.assertEqual(calls, [])
+
+    def test_a_draft_repo_id_is_resolved_with_its_own_revision(self):
+        cache_root = self._directory()
+        drafted = self._directory()
+        patch, calls = self._modelscope(cache_root, {"org/draft": drafted})
+        local = self._directory()
+        server_args = ServerArgs(
+            model_path=local,
+            tokenizer_path=local,
+            speculative_draft_model_path="org/draft",
+            speculative_draft_model_revision="v2",
+            device="cuda",
+        )
+        with patch:
+            handle_modelscope_paths(server_args)
+
+        self.assertEqual(
+            resolving_view(server_args).speculative_draft_model_path, drafted
+        )
+        self.assertEqual([call[2] for call in calls], ["v2"])
+
+
+class TestTheRemoteConnectorArm(_ModelSourceCase):
+    """`ModelConfig` repoints itself for any other ``scheme://``.
+
+    `redis://` is the shape the object-store arm does not claim, so it is the
+    one that reaches `_maybe_pull_model_tokenizer_from_remote`.
+    """
+
+    def _connected_to(self, directory):
+        state = {}
+
+        class _Client:
+            def pull_files(self, allow_pattern=None):
+                state["allow_pattern"] = allow_pattern
+
+            def get_local_dir(self):
+                return directory
+
+        return (
+            mock.patch.object(
+                connector_module, "create_remote_connector", return_value=_Client()
+            ),
+            state,
+        )
+
+    def test_the_configuration_reads_from_the_pulled_directory(self):
+        pulled = self._checkpoint()
+        patch, state = self._connected_to(pulled)
+        with patch:
+            config = ModelConfig(model_path=_REMOTE_URL)
+
+        self.assertEqual(config.model_path, pulled)
+        # The weights stay where they are; only the metadata was pulled.
+        self.assertEqual(config.model_weights, _REMOTE_URL)
+        self.assertEqual(state["allow_pattern"], ["*config.json"])
+
+    def test_the_record_keeps_the_url_and_the_cache_stays_keyed_on_it(self):
+        """Same movement the object-store arm makes: the configuration's path
+        moves, the record's does not, and the cache key follows the record."""
+        pulled = self._checkpoint()
+        patch, _ = self._connected_to(pulled)
+        server_args = ServerArgs(model_path=_REMOTE_URL, device="cuda")
+        with patch:
+            config = model_config_of(server_args)
+
+        self.assertEqual(server_args.model_path, _REMOTE_URL)
+        self.assertEqual(config.model_path, pulled)
+        self.assertIs(model_config_of(server_args), config)
+
+
+if __name__ == "__main__":
+    unittest.main()

test/registered/unit/server_args/test_no_public_non_field_slot.py

@@ -59,11 +59,14 @@ def _self_written_attributes() -> set:
 class TestNoPublicNonFieldSlot(CustomTestCase):
     def test_every_public_attribute_is_a_field(self):
         written = _self_written_attributes()
-        self.assertGreater(
-            len(written),
-            3,
-            f"only {len(written)} self-writes found; the scan is broken, not the "
-            "record",
+        # Anchor on a name, not a count: the count falls every time a derived
+        # read leaves the record, so a floor erodes with what it measures.
+        self.assertIn(
+            "_resolution_finished",
+            written,
+            f"the scan did not find the resolution flag the record sets on "
+            f"itself, so it is the scan that is broken, not the record: "
+            f"{sorted(written)}",
         )
         fields = {field.name for field in dataclasses.fields(ServerArgs)}
         stray = sorted(

test/registered/unit/server_args/test_record_member_calls_resolve.py

@@ -0,0 +1,144 @@
+"""Every `server_args.<name>()` in the tree names something the record has.
+
+Removing a member from `ServerArgs` means rewriting its callers, and the ones
+inside `server_args.py` are the ones you fix by reflex. The cross-file caller is
+what bites: `ServerArgs.ssl_verify()` moved to `serving_hook.ssl_verify_of()` and
+one call site kept the old spelling as `self.server_args.ssl_verify()` -- a grep
+for `server_args.ssl_verify()` does not find that, and nothing else looks. Every
+`HttpServerEngineAdapter` request raised `AttributeError` before sending.
+
+So this resolves the call sites instead of grepping for them: every attribute
+*called* on something statically known to be a record has to exist on the record.
+It is deliberately not limited to methods the refactor touched -- the next
+removal gets the same check for free.
+
+`multimodal_gen` carries a different, same-named class outside this contract, as
+the other record ratchets also record.
+"""
+
+from sglang.test.ci.ci_register import register_cpu_ci
+
+register_cpu_ci(est_time=6, suite="base-a-test-cpu")
+
+import ast
+import dataclasses
+import pathlib
+import unittest
+
+import sglang
+from sglang.srt.server_args import ServerArgs
+from sglang.test.test_utils import CustomTestCase
+
+_ROOTS = (
+    pathlib.Path(next(iter(sglang.__path__))) / "srt",
+    pathlib.Path(__file__).resolve().parents[3],  # test/
+)
+_EXCLUDED = ("multimodal_gen",)
+
+# Attribute names that hold a `ServerArgs`. `resolving_view` and `resolved_view`
+# proxy the record but answer for names it does not carry, so they are not here.
+_RECORD_NAMES = ("server_args", "_server_args")
+
+
+def _is_record(node) -> bool:
+    """`server_args`, `self.server_args`, `self._server_args`, `cls.server_args`."""
+    if isinstance(node, ast.Name):
+        return node.id in _RECORD_NAMES
+    if isinstance(node, ast.Attribute):
+        return node.attr in _RECORD_NAMES
+    return False
+
+
+def _rebound_locally(tree) -> set:
+    """Names assigned something that is plainly not a record.
+
+    `server_args` is also a natural name for a dict of CLI flags or a list of
+    argv strings in test helpers, and those legitimately answer `.update()` and
+    `.items()`. A function that assigns one of those to the name is not talking
+    about the record in that scope.
+    """
+    literal = (ast.Dict, ast.List, ast.DictComp, ast.ListComp)
+    builders = {"dict", "list", "tuple", "set"}
+
+    def _not_a_record(value) -> bool:
+        if isinstance(value, literal):
+            return True
+        # `dict(...)` / `list(...)`, and an annotated `server_args: list[str] = [...]`
+        return (
+            isinstance(value, ast.Call) and getattr(value.func, "id", None) in builders
+        )
+
+    rebound = set()
+    for node in ast.walk(tree):
+        if isinstance(node, ast.AnnAssign):
+            targets, value = [node.target], node.value
+        elif isinstance(node, ast.Assign):
+            targets, value = node.targets, node.value
+        else:
+            continue
+        if value is None or not _not_a_record(value):
+            continue
+        for target in targets:
+            if isinstance(target, ast.Name) and target.id in _RECORD_NAMES:
+                rebound.add(target.id)
+    return rebound
+
+
+def _called_members():
+    """{name: [file:line]} for every `<record>.<name>(...)` in the tree."""
+    found: dict[str, list[str]] = {}
+    for root in _ROOTS:
+        for path in sorted(root.rglob("*.py")):
+            text = path.as_posix()
+            if any(part in text for part in _EXCLUDED):
+                continue
+            source = path.read_text(encoding="utf-8-sig")
+            if "server_args" not in source:
+                continue
+            try:
+                tree = ast.parse(source)
+            except SyntaxError:
+                continue
+            rebound = _rebound_locally(tree)
+            for node in ast.walk(tree):
+                if (
+                    isinstance(node, ast.Call)
+                    and isinstance(node.func, ast.Attribute)
+                    and _is_record(node.func.value)
+                    and getattr(node.func.value, "id", None) not in rebound
+                ):
+                    found.setdefault(node.func.attr, []).append(
+                        f"{path.name}:{node.lineno}"
+                    )
+    return found
+
+
+class TestRecordMemberCallsResolve(CustomTestCase):
+    def test_every_called_member_exists_on_the_record(self):
+        called = _called_members()
+        self.assertGreater(
+            len(called),
+            5,
+            f"only {len(called)} members called on a record; the scan is broken, "
+            "not the tree",
+        )
+        available = set(dir(ServerArgs)) | {
+            field.name for field in dataclasses.fields(ServerArgs)
+        }
+        missing = {
+            name: sites
+            for name, sites in sorted(called.items())
+            if name not in available
+        }
+        self.assertEqual(
+            {},
+            missing,
+            "these are called on a ServerArgs but the record has no such member -- "
+            "each one raises AttributeError at the call. A member that moved out of "
+            "the record has to be rewritten at every call site, including the ones "
+            f"reached through `self.server_args`: {missing}",
+        )
+
+
+if __name__ == "__main__":
+    unittest.main()

test/registered/unit/server_args/test_server_args.py

@@ -55,6 +55,7 @@ from sglang.srt.arg_groups.serving_hook import (
     handle_multimodal_feature_transport,
     handle_ssl_validation,
     handle_tokenizer_batching,
+    ssl_verify_of,
 )
 from sglang.srt.arg_groups.speculative_hook import handle_speculative_decoding
 from sglang.srt.arg_groups.validation_hook import check_two_batch_overlap
@@ -1367,13 +1368,15 @@ class TestSSLArgs(unittest.TestCase):
         self.assertTrue(server_args.url().startswith("https://"))
 
     def test_ssl_verify_without_ssl(self):
+        # the derived read lives with the rest of the SSL handling now
+
         server_args = ServerArgs(model_path="dummy")
-        self.assertIs(server_args.ssl_verify(), True)
+        self.assertIs(ssl_verify_of(server_args), True)
 
     @patch("os.path.isfile", return_value=True)
     def test_ssl_verify_with_ssl_no_ca(self, _mock_isfile):
         server_args = self._validate_ssl(ssl_keyfile="key.pem", ssl_certfile="cert.pem")
-        self.assertIs(server_args.ssl_verify(), False)
+        self.assertIs(ssl_verify_of(server_args), False)
 
     @patch("os.path.isfile", return_value=True)
     def test_ssl_verify_with_ssl_and_ca(self, _mock_isfile):
@@ -1382,7 +1385,7 @@ class TestSSLArgs(unittest.TestCase):
             ssl_certfile="cert.pem",
             ssl_ca_certs="ca.pem",
         )
-        self.assertEqual(server_args.ssl_verify(), "ca.pem")
+        self.assertEqual(ssl_verify_of(server_args), "ca.pem")
 
     def test_ssl_ca_certs_without_certfile_raises(self):
         with self.assertRaises(ValueError) as context:
@@ -2840,8 +2843,13 @@ class TestDcpKvEventContract(CustomTestCase):
     def test_kv_event_block_size_widens_a_single_token_page(self):
         # page_size=1 + DCP is a real deployment shape: the allocator is still
         # paged, at dcp_size.
+        from sglang.srt.arg_groups.overrides import (
+            kv_event_block_size_of,
+            resolving_view,
+        )
+
         args = ServerArgs(model_path="dummy", tp_size=8, dcp_size=8, page_size=1)
-        self.assertEqual(args.kv_event_block_size, 8)
+        self.assertEqual(kv_event_block_size_of(resolving_view(args)), 8)
 
 
 if __name__ == "__main__":