diff --git a/test/registered/unit/server_args/test_model_config_reads_resolved_input.py b/test/registered/unit/server_args/test_model_config_reads_resolved_input.py new file mode 100644 index 000000000..1deefd8db --- /dev/null +++ b/test/registered/unit/server_args/test_model_config_reads_resolved_input.py @@ -0,0 +1,655 @@ +"""`ModelConfig` is built from values resolution has already decided. + +Resolution builds a `ModelConfig` partway through and keys later decisions off +it, so the pipeline reads its own output through that object. The loop is only +benign while every field `ModelConfig.from_server_args` reads has been resolved +by the time it is built -- otherwise the model configuration describes a +half-resolved input, and every handler downstream of it inherits that. + +Nothing enforces the ordering today; it holds because the path and quantization +handlers happen to run early. So this derives both sides from the source -- the +fields the constructor reads, and the step each is declared at -- and pins the +one field that is deliberately read before resolution touches it. +""" + +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=5, suite="base-a-test-cpu") + +_SRT = pathlib.Path(sglang.__file__).resolve().parent / "srt" + +# Read for what the caller asked for: the constructor passes it through and +# never stores it, while resolution later overwrites the field with the value +# the architecture implies. Two quantities sharing one name. +_READ_BEFORE_RESOLUTION = frozenset({"is_embedding"}) + +# Declared after the first `get_model_config()`, so the cached configuration +# holds the earlier value. Nothing reads the stale copy today (its one consumer +# is on the `is_draft_model` branch, built after resolution), and fixing it +# means moving the build or the hook. Pinned so a second field in this position +# has to be looked at. +_STALE_IN_THE_MODEL_CONFIG = frozenset({"speculative_algorithm"}) + +# The same staleness through the registries: `_handle_model_specific_adjustments` +# builds the model configuration and *then* collects the override declarations, +# both inside one handler body. Named rather than fixed (that means moving the +# build or the collection), so a fifth field here has to be looked at -- and so +# does fixing the ordering. +_STALE_FROM_THE_REGISTRIES = frozenset( + { + "disable_hybrid_swa_memory", + "dtype", + "enable_multi_layer_eagle", + "quantization", + } +) + + +def _registry_declared_fields(): + """What the live registries and passes declare. + + Imported from the chain ratchet by path instead of re-derived: two + derivations of the same set drift, and the one that drifts narrower makes + this check quietly vacuous. Keying on `self._declare(...)` alone is what + hid these four -- 26 of the providers register through a helper call, and + none of them spell a keyword this file can see. + """ + import importlib.util + + ratchet = ( + pathlib.Path(__file__).resolve().parent.parent / "test_chain_read_ratchet.py" + ) + spec = importlib.util.spec_from_file_location("_chain_ratchet_for_pin", ratchet) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module._declared_by_registry_and_passes() + + +def _registry_collection_is_after_the_build(): + """(collection line, first build line) inside the model-specific handler. + + Handler-local ordering only -- the caller still has to compare against the + pipeline-wide first build, which sits in an *earlier* step: hoisting the + collection above this handler's own `get_model_config()` call does not move + it above the configuration another handler already cached. + """ + tree = ast.parse((_SRT / "server_args.py").read_text(encoding="utf-8-sig")) + handler = next( + node + for node in ast.walk(tree) + if isinstance(node, ast.FunctionDef) + and node.name == "_handle_model_specific_adjustments" + ) + build = collect = None + for node in ast.walk(handler): + if not isinstance(node, ast.Call): + continue + # Both spellings: an Attribute call and a bare Name call. + func = node.func + if isinstance(func, ast.Attribute): + name = func.attr + elif isinstance(func, ast.Name): + name = func.id + else: + continue + if name == "get_model_config" and build is None: + build = node.lineno + if name == "collect_model_override_declarations" and collect is None: + collect = node.lineno + return collect, build + + +def _server_args_names(tree, path): + names = {"self"} if path.name == "server_args.py" else {"server_args"} + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + args = node.args + for arg in args.posonlyargs + args.args + args.kwonlyargs: + annotation = arg.annotation + if isinstance(annotation, ast.Constant): + text = annotation.value + elif isinstance(annotation, ast.Name): + text = annotation.id + elif isinstance(annotation, ast.Attribute): + text = annotation.attr + else: + continue + if text == "ServerArgs": + names.add(arg.arg) + return names + + +def _constructor_reads(): + """Fields `ModelConfig.from_server_args` takes off the record.""" + path = _SRT / "configs/model_config.py" + tree = ast.parse(path.read_text(encoding="utf-8-sig")) + constructor = next( + node + for node in ast.walk(tree) + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and node.name == "from_server_args" + ) + names = _server_args_names(tree, path) + reads = { + node.attr + for node in ast.walk(constructor) + if isinstance(node, ast.Attribute) + and isinstance(node.value, ast.Name) + and node.value.id in names + and isinstance(node.ctx, ast.Load) + } + # `getattr(server_args, "field", default)` is the normal spelling for an + # optional input and is a `Call`, not an `Attribute`. + for node in ast.walk(constructor): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id == "getattr" + and len(node.args) >= 2 + and isinstance(node.args[0], ast.Name) + and node.args[0].id in names + and isinstance(node.args[1], ast.Constant) + and isinstance(node.args[1].value, str) + ): + reads.add(node.args[1].value) + return reads + + +def _late_resolution_fields(): + """Fields written through `_late_resolution` / `declare_late_resolution`. + + All of them land after the model configuration is built: the launcher's + validation stage runs long after `__post_init__`. + """ + fields = set() + for name in ( + "server_args.py", + "arg_groups/overrides.py", + "utils/template_detection.py", + ): + path = _SRT / name + if not path.exists(): + continue + tree = ast.parse(path.read_text(encoding="utf-8-sig")) + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + called = ( + node.func.attr + if isinstance(node.func, ast.Attribute) + else getattr(node.func, "id", "") + ) + if called in ("_late_resolution", "declare_late_resolution"): + fields |= {kw.arg for kw in node.keywords if kw.arg} + return fields + + +def _hook_declarations(dispatch, source_module): + """{field: dispatcher line} for hooks the dispatch calls on other objects. + + `handle_speculative_decoding(self)` is not a `self.()` call, so a + scan of the dispatcher's own method calls never reaches its + `declare_resolution` sites -- and the speculative hooks decide + `speculative_algorithm`, which the model configuration reads. + + The platform hook is *not* covered here: it reaches the pipeline as a + callback argument, so there is no call node to follow and its writes live + outside this tree. Its position is pinned instead -- + `test_every_opaque_callback_is_still_late`. + """ + imported = {} + for node in ast.walk(ast.parse(source_module.read_text(encoding="utf-8-sig"))): + if isinstance(node, ast.ImportFrom) and node.module: + for alias in node.names: + imported[alias.asname or alias.name] = node.module + + out = {} + for node in ast.walk(dispatch): + if not isinstance(node, ast.Call): + continue + name = ( + node.func.id + if isinstance(node.func, ast.Name) + else (node.func.attr if isinstance(node.func, ast.Attribute) else None) + ) + module = imported.get(name) + if not module or not module.startswith("sglang.srt."): + continue + path = _SRT / (module[len("sglang.srt.") :].replace(".", "/") + ".py") + if not path.exists(): + continue + for inner in ast.walk(ast.parse(path.read_text(encoding="utf-8-sig"))): + if ( + isinstance(inner, ast.Call) + and isinstance(inner.func, ast.Name) + and inner.func.id == "declare_resolution" + ): + for keyword in inner.keywords: + if keyword.arg: + out[keyword.arg] = max(out.get(keyword.arg, 0), node.lineno) + return out + + +def _pipeline(): + """(ordered steps, {step: methods it reaches}) for the resolution dispatch.""" + source = (_SRT / "server_args.py").read_text(encoding="utf-8-sig") + tree = ast.parse(source) + record = next( + node + for node in tree.body + if isinstance(node, ast.ClassDef) and node.name == "ServerArgs" + ) + methods = { + node.name: node for node in record.body if isinstance(node, ast.FunctionDef) + } + dispatch = methods["_run_resolution_pipeline"] + steps = [ + name + for _line, name in sorted( + (node.lineno, node.func.attr) + for node in ast.walk(dispatch) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "self" + ) + ] + + def reaches(name, seen=None): + seen = seen if seen is not None else set() + if name in seen or name not in methods: + return seen + seen.add(name) + for node in ast.walk(methods[name]): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "self" + and node.func.attr in methods + ): + reaches(node.func.attr, seen) + return seen + + step_lines = {} + for node in ast.walk(dispatch): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "self" + ): + step_lines.setdefault(node.func.attr, node.lineno) + return steps, methods, {name: reaches(name) for name in steps}, step_lines + + +def _opaque_callback_positions(dispatch, source_module): + """{callback spelling: dispatcher line} for every resolver handed in. + + `declare_direct_writes(record, source, callback)` runs a callable instead of + code in this tree -- a platform plugin, a registered speculative algorithm. + Which fields such a callback writes is not a static question; only *when* it + runs is, so the position is what gets pinned. + + Two spellings reach the pipeline: the dispatcher wraps a callback itself, or + it calls a hook in this tree that wraps one. The line recorded is always the + dispatcher's, because that is where the ordering against the build is + decided -- a hook body sits further down its own file and says nothing about + it. + """ + imported = {} + for node in ast.walk(ast.parse(source_module.read_text(encoding="utf-8-sig"))): + if isinstance(node, ast.ImportFrom) and node.module: + for alias in node.names: + imported[alias.asname or alias.name] = node.module + + def callbacks_in(tree): + found = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + name = ( + node.func.id + if isinstance(node.func, ast.Name) + else (node.func.attr if isinstance(node.func, ast.Attribute) else None) + ) + if name == "declare_direct_writes" and len(node.args) > 2: + found.append(ast.unparse(node.args[2])) + return found + + positions = {} + for spelling in callbacks_in(dispatch): + positions[spelling] = min( + positions.get(spelling, 10**9), + next( + node.lineno + for node in ast.walk(dispatch) + if isinstance(node, ast.Call) + and getattr(node.func, "id", None) == "declare_direct_writes" + ), + ) + for node in ast.walk(dispatch): + if not isinstance(node, ast.Call): + continue + name = ( + node.func.id + if isinstance(node.func, ast.Name) + else (node.func.attr if isinstance(node.func, ast.Attribute) else None) + ) + module = imported.get(name) + if not module or not module.startswith("sglang.srt."): + continue + path = _SRT / (module[len("sglang.srt.") :].replace(".", "/") + ".py") + if not path.exists(): + continue + for spelling in callbacks_in(ast.parse(path.read_text(encoding="utf-8-sig"))): + positions[spelling] = min(positions.get(spelling, 10**9), node.lineno) + return positions + + +def _declaration_positions(): + """({field: position}, first_build) over the fields the constructor reads. + + A position is `(step index, rank)`, and `rank` is 0 only for a declaration + that sits *above* the build in the very method that builds: a declaration + applies where it is written, so one statement earlier in the same body is + genuinely earlier. Everything else in the build's step gets rank 1 and + counts as late -- line numbers say nothing across two method bodies, since + a handler sits further down the file than the dispatcher that calls it. + + One derivation, two callers: the check below asks which fields land after + the build, and the pin check asks whether an exempted field is still one + of them. Two derivations of that answer drift apart. + """ + steps, methods, reached, step_lines = _pipeline() + wanted = _constructor_reads() + + def build_site(): + """(step index, method name, line) of the first `get_model_config()`.""" + for index, step in enumerate(steps): + for method in reached[step]: + for node in ast.walk(methods[method]): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "get_model_config" + ): + return index, step, method, node.lineno + return None + + site = build_site() + if site is None: + return {}, None + build_index, build_step, build_method, build_line_in_body = site + first_build = (build_index, build_step) + + source_module = _SRT / "server_args.py" + imported = {} + for node in ast.walk(ast.parse(source_module.read_text(encoding="utf-8-sig"))): + if isinstance(node, ast.ImportFrom) and node.module: + for alias in node.names: + imported[alias.asname or alias.name] = node.module + + def _hook_declared_fields(name): + """Fields a hook imported from `sglang.srt` declares, by callable name.""" + module = imported.get(name) + if not module or not module.startswith("sglang.srt."): + return frozenset() + path = _SRT / (module[len("sglang.srt.") :].replace(".", "/") + ".py") + if not path.exists(): + return frozenset() + fields = set() + for node in ast.walk(ast.parse(path.read_text(encoding="utf-8-sig"))): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id == "declare_resolution" + ): + fields |= {kw.arg for kw in node.keywords if kw.arg} + return frozenset(fields) + + declared_at = {} + for index, step in enumerate(steps): + for method in reached[step]: + for node in ast.walk(methods[method]): + if not isinstance(node, ast.Call): + continue + same_body = index == build_index and method == build_method + rank = 0 if same_body and node.lineno < build_line_in_body else 1 + if ( + isinstance(node.func, ast.Attribute) + and node.func.attr == "_declare" + ): + fields = {kw.arg for kw in node.keywords if kw.arg} + # A handler that calls an imported hook (the Kimi and DeepSeek + # defaults live in arg_groups modules) declares through it, and + # the hook can sit below the build inside the same handler. + elif isinstance(node.func, ast.Name): + fields = _hook_declared_fields(node.func.id) + else: + continue + for field in fields: + if field in wanted: + # The *last* declaration is the one that has to precede + # the build. + declared_at[field] = max( + declared_at.get(field, (index, rank)), (index, rank) + ) + + # Hooks the dispatch calls on other objects declare too, and a hook below + # the first build is late by definition. Both positions are read *inside + # the dispatcher*: a handler body sits further down the file than the + # dispatcher that calls it, so a line number taken from one scope says + # nothing about ordering against the other. + dispatch = methods["_run_resolution_pipeline"] + build_line = step_lines[first_build[1]] + for field, line in _hook_declarations(dispatch, _SRT / "server_args.py").items(): + if field in wanted and line > build_line: + declared_at[field] = max( + declared_at.get(field, (build_index, 1)), (10**6, 1) + ) + + # Late resolution is the other channel that can decide a field the + # constructor reads, and it runs after every build. + for field in _late_resolution_fields(): + if field in wanted: + declared_at[field] = (10**6, 1) + return declared_at, first_build + + +class TestModelConfigReadsResolvedInput(CustomTestCase): + def test_every_field_it_reads_is_resolved_before_it_is_built(self): + declared_at, first_build = _declaration_positions() + self.assertIsNotNone( + first_build, "no handler builds a ModelConfig; the scan broke" + ) + + known = ( + _READ_BEFORE_RESOLUTION + | _STALE_IN_THE_MODEL_CONFIG + | _STALE_FROM_THE_REGISTRIES + ) + late = sorted( + field + for field, position in declared_at.items() + if position >= (first_build[0], 1) and field not in known + ) + self.assertEqual( + late, + [], + "resolution decides these after it builds the ModelConfig that reads " + f"them, so the model configuration describes a half-resolved input " + f"(first build: step {first_build[0]}, {first_build[1]}): {late}", + ) + + def test_the_registry_stale_set_is_exactly_what_is_late(self): + """Equality, not membership. + + A fifth field the registries decide after the build fails here, and so + does fixing the ordering -- either way someone has to come back and + read this. The earlier version of this file derived declarations only + from `self._declare(...)` keywords, so it passed while these four were + already stale. + """ + collect_line, build_line = _registry_collection_is_after_the_build() + self.assertIsNotNone( + collect_line, "the handler no longer collects registry declarations" + ) + reads = _constructor_reads() + registry = _registry_declared_fields() + self.assertGreater( + len(registry), 20, "the registry-declared set collapsed; nothing to compare" + ) + # Late against the *pipeline-wide* first build, not only the build in + # the collection's own handler: `_handle_gpu_memory_settings` builds + # the configuration many steps earlier, so hoisting the collection + # above the local build still leaves that cache describing raw input. + steps, methods, reached, _step_lines = _pipeline() + _declared_at, first_build = _declaration_positions() + self.assertIsNotNone( + first_build, "no handler builds a ModelConfig; the scan broke" + ) + collecting_steps = [ + index + for index, step in enumerate(steps) + for method in reached[step] + if any( + isinstance(node, ast.Call) + and ( + node.func.attr + if isinstance(node.func, ast.Attribute) + else getattr(node.func, "id", None) + ) + == "collect_model_override_declarations" + for node in ast.walk(methods[method]) + ) + ] + self.assertTrue(collecting_steps, "no pipeline step collects the registry") + collection_is_late = min(collecting_steps) > first_build[0] or ( + build_line is not None and collect_line > build_line + ) + late = frozenset(reads & registry) if collection_is_late else frozenset() + self.assertEqual( + sorted(late), + sorted(_STALE_FROM_THE_REGISTRIES), + "the set of ModelConfig-read fields the registries decide after the " + f"build changed (collection at line {collect_line}, build at line " + f"{build_line}); read the comment on _STALE_FROM_THE_REGISTRIES " + "before editing it", + ) + + def test_the_pinned_stale_field_is_still_stale(self): + """If the ordering gets fixed, this pin has to be retired, not kept. + + A pin that outlives the defect it describes is worse than none: it + documents a hazard that no longer exists and hides the day one appears. + """ + steps, methods, reached, step_lines = _pipeline() + dispatch = methods["_run_resolution_pipeline"] + hooks = _hook_declarations(dispatch, _SRT / "server_args.py") + build_line = min( + step_lines[step] + for step in steps + for method in reached[step] + if any( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "get_model_config" + for node in ast.walk(methods[method]) + ) + ) + for field in _STALE_IN_THE_MODEL_CONFIG: + self.assertIn( + field, + hooks, + f"{field} is pinned as decided after the build, but no hook " + "declares it any more; retire the pin", + ) + self.assertGreater( + hooks[field], + build_line, + f"{field} is now decided before the model configuration is " + "built; retire the pin", + ) + + def test_every_opaque_callback_is_still_late(self): + """The opaque resolvers all run after the model configuration is built. + + A plugin that rewrites `dtype` or `model_path` in one of them is + invisible to the configuration already cached, and no scan can say + whether it does: the implementations are out of tree. So the positions + are the pin, and the set of callbacks is pinned with them -- a new one + has to be placed against the build by whoever adds it. Moving them all + above the build fixes the hazard and fails this test; retire the pin + then, rather than keeping a note about a hazard that is gone. + """ + steps, methods, reached, step_lines = _pipeline() + dispatch = methods["_run_resolution_pipeline"] + positions = _opaque_callback_positions(dispatch, _SRT / "server_args.py") + self.assertEqual( + sorted(positions), + [ + "algo.handle_server_args", + "algo.validate_server_args", + "current_platform.apply_server_args_defaults", + ], + "the set of resolvers handed to declare_direct_writes changed; each " + "one needs its position against the ModelConfig build looked at", + ) + _declared_at, first_build = _declaration_positions() + self.assertIsNotNone( + first_build, "no handler builds a ModelConfig; the scan broke" + ) + build_line = step_lines[first_build[1]] + for spelling, line in sorted(positions.items()): + self.assertGreater( + line, + build_line, + f"{spelling} now runs before the model configuration is built, " + "so a plugin's writes reach it; retire the pin", + ) + + def test_the_documented_exception_is_still_the_only_one(self): + """A field pinned as read-before-resolution has to still be all three. + + Read by the constructor, written by resolution, and written *after* the + build -- the last one is what makes the exemption load-bearing. Without + it, moving the declaration earlier leaves the name sitting in the + exempt set with nothing to exempt, and the next field that lands in + this position gets waved through by a pin nobody re-read. + """ + wanted = _constructor_reads() + declared_at, first_build = _declaration_positions() + self.assertIsNotNone( + first_build, "no handler builds a ModelConfig; the scan broke" + ) + for field in sorted(_READ_BEFORE_RESOLUTION): + self.assertIn( + field, + wanted, + f"{field} is pinned as read before resolution, but the " + "constructor no longer reads it; retire the pin", + ) + self.assertIn( + field, + declared_at, + f"{field} is pinned as read before resolution, but resolution " + "no longer writes it; retire the pin", + ) + self.assertGreaterEqual( + declared_at[field], + (first_build[0], 1), + f"{field} is now decided before the model configuration is " + "built, so the exemption covers nothing; retire the pin", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/test/registered/unit/test_chain_read_ratchet.py b/test/registered/unit/test_chain_read_ratchet.py new file mode 100644 index 000000000..30d0d75ec --- /dev/null +++ b/test/registered/unit/test_chain_read_ratchet.py @@ -0,0 +1,470 @@ +"""Nobody reaches the startup record through another object for a resolved value. + +The supplied-instance census counts three spellings, all of which start from a +`server_args` parameter -- the caller chose the object, which is the contract +that makes those reads defensible. This pins the fourth: `model_runner. +server_args.field`, `self.scheduler.server_args.field`, `tokenizer_manager. +server_args.field`. A reference lifted off whatever object happened to hold the +record carries no contract at all, and it was invisible to every census, which +is how it grew to 105 reads across 36 files unnoticed. + +They are gone, and this is what keeps them gone. Only fields resolution writes +are pinned: reading `model_runner.server_args.host` off the record answers with +what the caller asked for, which is what the record is for. The written set is +derived from the declaration sites rather than listed, so a field that stops +being resolution-written drops out on its own -- and a field that stops being +*declared* cannot slip out that way, because bare assignment during resolution +is refused by `server_args/test_resolution_declarations.py`. +""" + +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=5, suite="base-a-test-cpu") + +_PACKAGE = pathlib.Path(sglang.__file__).resolve().parent +_SRT = _PACKAGE / "srt" + +# The pipeline and its extension points: reading the in-flight record is their +# job, and they run before anything is published. +_OWNERS = ("server_args.py", "runtime_context.py", "arg_groups/") + +# Where the writers are. Resolution lives in `srt` -- nothing outside it +# declares -- so the written-field derivations scan `srt` while the *reads* are +# counted across the whole shipped package: a borrowed read answers with the +# startup default wherever it is written, and `benchmark/` ships too. +_READS_SCANNED = _PACKAGE + +_DECLARERS = ("_declare", "declare_resolution", "declare_late_resolution") + + +def _declared_by_keyword(): + """Fields named as a keyword at a declaration site.""" + written = set() + for path in sorted(_SRT.rglob("*.py")): + try: + tree = ast.parse(path.read_text(encoding="utf-8-sig")) + except SyntaxError: + raise AssertionError(f"unparsable module in the census: {path}") + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + if isinstance(node.func, ast.Attribute): + name = node.func.attr + elif isinstance(node.func, ast.Name): + name = node.func.id + else: + continue + if name in _DECLARERS: + written |= {kw.arg for kw in node.keywords if kw.arg} + return written + + +def _returned_field_names(function): + """Field names a provider/pass writes: the keys of the mapping it returns. + + Only the returned mapping counts -- walking every `ast.Dict` in the body + also collects a dict-valued field's *nested* keys and any unrelated local + mapping, and those stray names would reject valid borrowed reads of fields + resolution never writes. The mapping is traced through four spellings: a + returned literal, assignments (annotated or not) to a returned name, a + literal-key subscript write on it, and `.update(field=...)` on it. A + spelling this cannot see raises instead of skipping. + """ + names = set() + returned = set() + + def top_level_keys(mapping): + for key in mapping.keys: + if not (isinstance(key, ast.Constant) and isinstance(key.value, str)): + raise AssertionError(f"non-literal key in {function.name}") + names.add(key.value) + + for node in ast.walk(function): + if isinstance(node, ast.Return) and node.value is not None: + value = node.value + if isinstance(value, ast.Dict): + top_level_keys(value) + elif isinstance(value, ast.Name): + returned.add(value.id) + elif isinstance(value, ast.Constant) and value.value is None: + pass + else: + raise AssertionError( + f"opaque return in {function.name}: {ast.unparse(value)}" + ) + for node in ast.walk(function): + if isinstance(node, (ast.Assign, ast.AnnAssign)): + targets = node.targets if isinstance(node, ast.Assign) else [node.target] + for target in targets: + if ( + isinstance(target, ast.Name) + and target.id in returned + and isinstance(node.value, ast.Dict) + ): + top_level_keys(node.value) + if isinstance(target, ast.Subscript) and ( + isinstance(target.value, ast.Name) and target.value.id in returned + ): + if not isinstance(target.slice, ast.Constant): + raise AssertionError(f"non-literal key in {function.name}") + names.add(target.slice.value) + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "update" + and isinstance(node.func.value, ast.Name) + and node.func.value.id in returned + ): + names |= {kw.arg for kw in node.keywords if kw.arg} + receiver = node.func.value + if isinstance(receiver, ast.Name) and receiver.id == "overrides": + # Positional dict literals are collected by the Dict walk; + # anything else is invisible. + for arg in node.args: + if not isinstance(arg, ast.Dict): + raise AssertionError( + f"opaque overrides.update() argument in {function.name}" + ) + if any(kw.arg is None for kw in node.keywords): + raise AssertionError( + f"**kwargs overrides.update() in {function.name}" + ) + return names + + +def _declared_by_registry_and_passes(): + """Fields the model-override registry and the post-process passes write. + + These field names are *data* -- dict keys, not keywords -- so a keyword + scan misses every one of them. The callables are collected from the live + registries rather than by matching decorator names: 26 of the 27 providers + register through a `_register_for(...)` helper, so a scan for + `@register_model_override*` sees exactly one of them and reports a healthy + census over a channel it cannot see. + """ + from sglang.srt.arg_groups import overrides + + tree = ast.parse((_SRT / "arg_groups/overrides.py").read_text(encoding="utf-8-sig")) + bodies = { + node.name: node for node in ast.walk(tree) if isinstance(node, ast.FunctionDef) + } + callables = {fn for fns in overrides._MODEL_OVERRIDE_FNS.values() for fn in fns} + callables |= { + fn for _predicate, fn in getattr(overrides, "_PREDICATE_OVERRIDE_FNS", ()) + } + callables |= set(overrides.POST_PROCESS_PASSES) + + fields = set() + for fn in callables: + body = bodies.get(getattr(fn, "__name__", "")) + if body is not None: + fields |= _returned_field_names(body) + # The literal arch -> {field: value} table, which has no callable at all. + for node in tree.body: + target = None + if isinstance(node, ast.Assign) and isinstance(node.targets[0], ast.Name): + target = node.targets[0].id + elif isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name): + target = node.target.id + if target != "MODEL_OVERRIDES" or node.value is None: + continue + for inner in ast.walk(node.value): + if not isinstance(inner, ast.Dict): + continue + for key, value in zip(inner.keys, inner.values): + if isinstance(value, ast.Dict): + continue + if not isinstance(key, ast.Constant): + raise AssertionError("non-literal override key") + fields.add(key.value) + return fields + + +def _declared_by_late_resolution(): + """Keywords of `self._late_resolution(...)`, the fourth declarer spelling. + + It forwards `**fields` to `declare_late_resolution`, so the keywords sit at + its call sites and a scan for the declarer's own name finds none of them. + """ + tree = ast.parse((_SRT / "server_args.py").read_text(encoding="utf-8-sig")) + fields = set() + for node in ast.walk(tree): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "_late_resolution" + ): + fields |= {keyword.arg for keyword in node.keywords if keyword.arg} + return fields + + +def _written_after_publish(): + """Fields the runtime overrides once the bags exist. + + Imported from the supplied-instance ratchet rather than re-derived: it + already enumerates `get_context().override(...)` and its named wrapper, and + a second derivation of the same channel is what drifts narrower. A borrowed + read of one of these answers with the startup value the same way a + resolution-written one does -- the write just lands later. + """ + import importlib.util + + companion = ( + pathlib.Path(__file__).resolve().parent + / "test_supplied_instance_exposure_ratchet.py" + ) + spec = importlib.util.spec_from_file_location("_exposure_for_ratchet", companion) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return set(module.TestSuppliedInstanceExposure._override_written_fields()) + + +def _resolution_written(): + """Every field the startup record answers wrong once it stays raw.""" + return ( + _declared_by_keyword() + | _declared_by_registry_and_passes() + | _declared_by_late_resolution() + | _written_after_publish() + ) + + +def _borrowed_parking_spans(tree): + """[(span, parked attribute names)] for classes that park a borrowed record. + + The supplied-instance census counts `self.server_args.` only where + the class was handed the record as a parameter -- `self.server_args = + server_args` inside a method that takes one. A class that borrows it off + another object instead (`self.server_args = scheduler.server_args`) is + covered by neither census, and a read through that attribute is exactly the + borrowed-record chain read this file is about. The parked name is whatever + the class chose -- `self.args = scheduler.server_args` hides the same read, + so the assignment target is recorded, not assumed. + """ + spans = [] + for cls in (n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)): + parked_names = set() + for node in ast.walk(cls): + # Both assignment spellings -- `self.args = x.server_args` and the + # annotated `self.args: ServerArgs = x.server_args`. + if isinstance(node, ast.Assign): + targets = node.targets + elif isinstance(node, ast.AnnAssign) and node.value is not None: + targets = [node.target] + else: + continue + # A bare name on the right is the parameter the companion census + # follows; an attribute chain ending in `.server_args` is a record + # taken off another object. + if not ( + isinstance(node.value, ast.Attribute) + and node.value.attr == "server_args" + ): + continue + for target in targets: + if ( + isinstance(target, ast.Attribute) + and isinstance(target.value, ast.Name) + and target.value.id == "self" + ): + parked_names.add(target.attr) + if parked_names: + spans.append(((cls.lineno, cls.end_lineno), parked_names)) + return spans + + +def _subtrees_with_their_own_record(): + """Top-level package directories that define a second `ServerArgs`. + + `multimodal_gen` ships one, so `x.server_args.` inside it names a + field of *that* class -- and `model_path` is a field of both. Keying on the + spelling alone once put a resolution call into a diffusion entry point. + Derived from the class definitions rather than named here, so a third + record would be excluded the same way instead of silently counting. + """ + roots = set() + for path in _PACKAGE.rglob("*.py"): + rel = path.relative_to(_PACKAGE).as_posix() + if rel.startswith("srt/") or "/" not in rel: + continue + try: + tree = ast.parse(path.read_text(encoding="utf-8-sig")) + except SyntaxError: + continue + if any( + isinstance(node, ast.ClassDef) and node.name == "ServerArgs" + for node in ast.walk(tree) + ): + roots.add(rel.split("/")[0]) + return roots + + +def _reads_the_startup_record(tree): + """True when this module's `server_args` is the one `srt` resolves. + + Inside a subtree that owns another record, only a module that imports the + startup record is talking about it. + """ + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and node.module: + if node.module.startswith("sglang.srt"): + # The *original* names: `ServerArgs as SrtServerArgs` is still + # the startup record, whatever this module calls it. + names = {alias.name for alias in node.names} + if names & {"ServerArgs", "server_args", "prepare_server_args"}: + return True + return False + + +def _chain_reads(written): + """`.server_args.` where the field is resolution-written.""" + found = [] + other_records = _subtrees_with_their_own_record() + for path in sorted(_READS_SCANNED.rglob("*.py")): + rel = path.relative_to(_READS_SCANNED).as_posix() + in_srt = rel.startswith("srt/") + if in_srt: + under_srt = rel[len("srt/") :] + if path.name in _OWNERS or under_srt.startswith(_OWNERS[-1]): + continue + try: + tree = ast.parse(path.read_text(encoding="utf-8-sig")) + except SyntaxError: + raise AssertionError(f"unparsable module in the census: {rel}") + if ( + not in_srt + and rel.split("/")[0] in other_records + and not _reads_the_startup_record(tree) + ): + continue + parked = _borrowed_parking_spans(tree) + + def parked_alias(lineno, name): + return any( + start <= lineno <= end and name in names + for (start, end), names in parked + ) + + for node in ast.walk(tree): + if not ( + isinstance(node, ast.Attribute) + and isinstance(node.ctx, ast.Load) + and node.attr in written + ): + continue + base = node.value + if not isinstance(base, ast.Attribute): + continue + through_self = isinstance(base.value, ast.Name) and base.value.id == "self" + if base.attr == "server_args": + # `self.server_args.field` is the parked spelling the + # supplied-instance census counts -- but only where the record + # arrived as a parameter. + if through_self and not parked_alias(node.lineno, base.attr): + continue + elif not (through_self and parked_alias(node.lineno, base.attr)): + # Any other attribute counts only as a recorded parked alias + # (`self.args = scheduler.server_args` and later `self.args.x`). + continue + suffix = " (parked borrowed record)" if through_self else "" + found.append(f"{rel}:{node.lineno} {ast.unparse(base)}.{node.attr}{suffix}") + # `getattr(model_runner.server_args, "field", default)` reads the same + # borrowed record through a `Call`, with the same stale default. + for node in ast.walk(tree): + if not ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id == "getattr" + and len(node.args) >= 2 + and isinstance(node.args[1], ast.Constant) + and node.args[1].value in written + ): + continue + base = node.args[0] + if not isinstance(base, ast.Attribute): + continue + through_self = isinstance(base.value, ast.Name) and base.value.id == "self" + if base.attr == "server_args": + if through_self and not parked_alias(node.lineno, base.attr): + continue + elif not (through_self and parked_alias(node.lineno, base.attr)): + continue + found.append( + f"{rel}:{node.lineno} getattr({ast.unparse(base)}, " + f"{node.args[1].value!r})" + ) + return sorted(found) + + +class TestNoChainReadsOfResolvedConfig(CustomTestCase): + def test_the_census_has_something_to_count(self): + """A written set that collapsed would make the pin vacuous. + + Each mechanism is checked on its own, because they fail + independently: the keyword scan cannot see a field name that is data, + and a scan for `@register_model_override*` sees one provider out of + twenty-seven because the rest register through a helper. A hand-written + expectation of the resulting field names is what hid that -- it stayed + green while a whole channel went unscanned -- so each mechanism is + pinned by a floor derived from the live registry instead. + """ + from sglang.srt.arg_groups import overrides + + by_keyword = _declared_by_keyword() + by_data = _declared_by_registry_and_passes() + by_late = _declared_by_late_resolution() + + self.assertGreater( + len(by_keyword), + 100, + f"only {len(by_keyword)} fields are declared by keyword; the scan broke", + ) + providers = {fn for fns in overrides._MODEL_OVERRIDE_FNS.values() for fn in fns} + providers |= { + fn for _predicate, fn in getattr(overrides, "_PREDICATE_OVERRIDE_FNS", ()) + } + self.assertGreater( + len(providers) + len(overrides.POST_PROCESS_PASSES), + 50, + "the registry and pass tables collapsed; the data-channel scan is " + "reading an empty registry", + ) + self.assertGreater( + len(by_data), + 25, + f"only {len(by_data)} fields come from the registry and the passes, " + f"across {len(providers)} providers and " + f"{len(overrides.POST_PROCESS_PASSES)} passes; the scan of the " + "dict-key channel broke", + ) + self.assertGreaterEqual( + len(by_late), + 3, + f"only {len(by_late)} fields are declared late; the " + "`_late_resolution` keyword scan broke", + ) + # The three mechanisms are not the same set: if any became a subset of + # the keyword scan, that scan would be doing all the work and a + # regression in the others would be invisible. + self.assertTrue(by_data - by_keyword, "the data channel adds nothing") + self.assertTrue(by_late - by_keyword, "late resolution adds nothing") + + def test_nothing_reads_a_resolved_field_off_a_borrowed_record(self): + found = _chain_reads(_resolution_written()) + self.assertEqual( + found, + [], + "these reach the startup record through another object for a value " + "resolution decides, so they answer with the CLI default once the " + "record stays raw; read the config bag instead:\n " + "\n ".join(found), + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/test/registered/unit/test_supplied_instance_exposure_ratchet.py b/test/registered/unit/test_supplied_instance_exposure_ratchet.py index 8f88d2b28..edb4ad87b 100644 --- a/test/registered/unit/test_supplied_instance_exposure_ratchet.py +++ b/test/registered/unit/test_supplied_instance_exposure_ratchet.py @@ -731,11 +731,12 @@ class TestSuppliedInstanceExposure(CustomTestCase): name, and the *parked* form -- ``self.x = server_args`` in a method that takes the parameter, read as ``self.x.field`` anywhere in the class. Parking under a different object, a container, or a computed - name stays invisible, like in every census of this family -- the - loudest known boundary is the *chain* spelling, - ``model_runner.server_args.field`` off some other parameter, which - this census does not count (~150 reads tree-wide; extending the pin - to that spelling is its own step, not a by-product of this one).""" + name stays invisible, like in every census of this family. The + loudest boundary *was* the *chain* spelling, + ``model_runner.server_args.field`` off some other object, which this + census still does not count -- but those reads are gone for every + resolution-written field and ``test_chain_read_ratchet.py`` holds them + at zero, so the gap is no longer where the risk is.""" pairs = set() for path in sorted(_PACKAGE_ROOT.rglob("*.py")): rel = path.relative_to(_PACKAGE_ROOT).as_posix()