From d4695af6e1f4ed74ebda81425d8cf88e7d541ef7 Mon Sep 17 00:00:00 2001 From: Cam Quilici Date: Tue, 14 Jul 2026 14:23:56 -0500 Subject: [PATCH] fix(agentic): align result metadata validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 中文：修复 Agentic 结果元数据验证，使其与 Pydantic 源模型保持一致。 --- .../aggregation/process_agentic_result.py | 54 ++++++++------- .../test_process_agentic_result.py | 63 ++++++++++++++++-- utils/process_changelog.py | 2 +- utils/process_result.py | 15 +++-- utils/test_process_changelog.py | 65 +++++++++++++++++++ utils/test_process_result.py | 5 +- 6 files changed, 164 insertions(+), 40 deletions(-) diff --git a/utils/agentic/aggregation/process_agentic_result.py b/utils/agentic/aggregation/process_agentic_result.py index 6b4acbc131..00197b7123 100644 --- a/utils/agentic/aggregation/process_agentic_result.py +++ b/utils/agentic/aggregation/process_agentic_result.py @@ -7,7 +7,14 @@ import os import sys from pathlib import Path -from typing import Any +from typing import Any, TypeVar + +from pydantic import BaseModel, ValidationError + +from utils.matrix_logic.validation import ( + ComponentMetadata, + KVOffloadBackendMetadata, +) from .aggregation_common import round_floats from .request_metrics import compute_request_metrics, load_aggregate, load_records_with_accounting @@ -36,8 +43,14 @@ def required_env(name: str) -> str: return value -def optional_component_metadata(env_name: str) -> dict[str, str] | None: - """Parse strict optional component metadata from a JSON environment value.""" +MetadataModel = TypeVar("MetadataModel", bound=BaseModel) + + +def optional_metadata( + env_name: str, + schema: type[MetadataModel], +) -> dict[str, str] | None: + """Parse optional JSON metadata through its source Pydantic schema.""" raw_value = os.environ.get(env_name) if raw_value in (None, "", "null"): return None @@ -47,33 +60,24 @@ def optional_component_metadata(env_name: str) -> dict[str, str] | None: except json.JSONDecodeError as exc: raise SystemExit(f"{env_name} must contain valid JSON") from exc - if not isinstance(metadata, dict) or set(metadata) != {"name", "version"}: - raise SystemExit(f"{env_name} must contain exactly 'name' and 'version'") - if not all(isinstance(metadata[key], str) and metadata[key] for key in metadata): - raise SystemExit(f"{env_name} name and version must be non-empty strings") - return metadata + try: + return schema.model_validate(metadata).model_dump(exclude_none=True) + except ValidationError as exc: + raise SystemExit( + f"{env_name} does not match {schema.__name__}: {exc}" + ) from exc + + +def optional_component_metadata(env_name: str) -> dict[str, str] | None: + """Parse strict optional component metadata from a JSON environment value.""" + return optional_metadata(env_name, ComponentMetadata) def optional_kv_offload_backend_metadata( env_name: str, ) -> dict[str, str] | None: """Parse KV offload backend metadata with an optional version.""" - raw_value = os.environ.get(env_name) - if raw_value in (None, "", "null"): - return None - - try: - metadata = json.loads(raw_value) - except json.JSONDecodeError as exc: - raise SystemExit(f"{env_name} must contain valid JSON") from exc - - if not isinstance(metadata, dict) or not set(metadata) <= {"name", "version"}: - raise SystemExit(f"{env_name} may contain only 'name' and 'version'") - if set(metadata) not in ({"name"}, {"name", "version"}): - raise SystemExit(f"{env_name} must contain 'name' and optional 'version'") - if not all(isinstance(value, str) and value for value in metadata.values()): - raise SystemExit(f"{env_name} values must be non-empty strings") - return metadata + return optional_metadata(env_name, KVOffloadBackendMetadata) def _validate_kv_offload_env() -> tuple[str, dict[str, str] | None]: @@ -86,7 +90,7 @@ def _validate_kv_offload_env() -> tuple[str, dict[str, str] | None]: if backend_name or backend_metadata is not None: raise SystemExit("KV_OFFLOAD_BACKEND must be empty when KV_OFFLOADING=none") else: - if not backend_name or backend_name == "none" or backend_metadata is None: + if not backend_name or backend_metadata is None: raise SystemExit("KV_OFFLOAD_BACKEND is required when KV_OFFLOADING is enabled") if backend_metadata["name"] != backend_name: raise SystemExit( diff --git a/utils/agentic/aggregation/test_process_agentic_result.py b/utils/agentic/aggregation/test_process_agentic_result.py index 15a6a1482c..ae6dbe1a67 100644 --- a/utils/agentic/aggregation/test_process_agentic_result.py +++ b/utils/agentic/aggregation/test_process_agentic_result.py @@ -26,7 +26,11 @@ load_aggregate, load_records, ) -from utils.agentic.aggregation.process_agentic_result import _gpu_shape +from utils.agentic.aggregation.process_agentic_result import ( + _gpu_shape, + optional_component_metadata, + optional_kv_offload_backend_metadata, +) from utils.agentic.aggregation.server_metrics import ( compute_server_metrics, load_server_metrics, @@ -409,15 +413,23 @@ def test_processor_omits_component_metadata_when_absent(tmp_path: Path): @pytest.mark.parametrize( - "metadata", + ("metadata", "expected"), [ - {"name": "lmcache"}, - {"name": "lmcache", "version": "0.5.1"}, + ({"name": "lmcache"}, {"name": "lmcache"}), + ( + {"name": "lmcache", "version": "0.5.1"}, + {"name": "lmcache", "version": "0.5.1"}, + ), + ( + {"name": "vllm-simple", "version": None}, + {"name": "vllm-simple"}, + ), ], ) def test_processor_emits_kv_offload_backend_metadata( tmp_path: Path, - metadata: dict[str, str], + metadata: dict[str, str | None], + expected: dict[str, str], ): result_dir = _write_fixture(tmp_path) agg = _run_processor( @@ -425,12 +437,49 @@ def test_processor_emits_kv_offload_backend_metadata( tmp_path / "out", env_overrides={ "KV_OFFLOADING": "dram", - "KV_OFFLOAD_BACKEND": "lmcache", + "KV_OFFLOAD_BACKEND": metadata["name"], "KV_OFFLOAD_BACKEND_METADATA": json.dumps(metadata), }, ) - assert agg["kv_offload_backend"] == metadata + assert agg["kv_offload_backend"] == expected + + +@pytest.mark.parametrize( + "metadata", + [ + {"name": "vllm-router"}, + {"name": "vllm-router", "version": "", "mode": "round-robin"}, + {"name": "vllm-router", "version": "image:vllm/vllm-openai:latest"}, + ], +) +def test_component_metadata_rejects_values_rejected_by_source_schema( + monkeypatch: pytest.MonkeyPatch, + metadata: dict[str, str], +): + monkeypatch.setenv("TEST_COMPONENT_METADATA", json.dumps(metadata)) + + with pytest.raises(SystemExit, match="does not match ComponentMetadata"): + optional_component_metadata("TEST_COMPONENT_METADATA") + + +@pytest.mark.parametrize( + "metadata", + [ + {"name": ""}, + {"name": "lmcache", "version": ""}, + {"name": "lmcache", "version": "0.5.1", "mode": "cpu"}, + {"name": "lmcache", "version": "image:vllm/vllm-openai:latest"}, + ], +) +def test_kv_offload_metadata_rejects_values_rejected_by_source_schema( + monkeypatch: pytest.MonkeyPatch, + metadata: dict[str, str], +): + monkeypatch.setenv("TEST_KV_OFFLOAD_METADATA", json.dumps(metadata)) + + with pytest.raises(SystemExit, match="does not match KVOffloadBackendMetadata"): + optional_kv_offload_backend_metadata("TEST_KV_OFFLOAD_METADATA") def test_processor_latency_units_are_seconds(tmp_path: Path): diff --git a/utils/process_changelog.py b/utils/process_changelog.py index 311c8c9e22..c293dc2f80 100644 --- a/utils/process_changelog.py +++ b/utils/process_changelog.py @@ -270,7 +270,7 @@ def main(): # Validate final results structure validated = ChangelogMatrixEntry.model_validate(final_results) - print(validated.model_dump_json(by_alias=True)) + print(validated.model_dump_json(by_alias=True, exclude_none=True)) if __name__ == "__main__": diff --git a/utils/process_result.py b/utils/process_result.py index bf3bcdaba1..f22ba063d0 100644 --- a/utils/process_result.py +++ b/utils/process_result.py @@ -3,6 +3,10 @@ import os from pathlib import Path +from pydantic import ValidationError + +from matrix_logic.validation import ComponentMetadata + def get_required_env_vars(required_vars): """Load and validate required environment variables.""" @@ -33,11 +37,12 @@ def get_optional_component_metadata(env_var): except json.JSONDecodeError as exc: raise ValueError(f"{env_var} must contain valid JSON") from exc - if not isinstance(metadata, dict) or set(metadata) != {"name", "version"}: - raise ValueError(f"{env_var} must contain exactly 'name' and 'version'") - if not all(isinstance(metadata[key], str) and metadata[key] for key in metadata): - raise ValueError(f"{env_var} name and version must be non-empty strings") - return metadata + try: + return ComponentMetadata.model_validate(metadata).model_dump() + except ValidationError as exc: + raise ValueError( + f"{env_var} does not match ComponentMetadata: {exc}" + ) from exc # Base required env vars diff --git a/utils/test_process_changelog.py b/utils/test_process_changelog.py index 6ed3a04701..d5a1df804f 100644 --- a/utils/test_process_changelog.py +++ b/utils/test_process_changelog.py @@ -572,3 +572,68 @@ def fake_run(command, **kwargs): assert "--all-evals" not in commands[1] assert _scenario_values(commands[1]) == ["fixed-seq-len"] json.loads(capsys.readouterr().out) + + +def test_agentic_matrix_omits_null_optional_metadata( + monkeypatch, + capsys, +): + added_yaml = """ +- config-keys: + - test-config + description: + - Preserve optional KV offload metadata + pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1 + scenario-type: + - agentic-coding +""" + generated = [{ + "image": "vllm/vllm-openai:test", + "model": "test/model", + "model-prefix": "test", + "precision": "fp8", + "framework": "vllm", + "runner": "cluster:test", + "tp": 4, + "pp": 1, + "dcp-size": 1, + "pcp-size": 1, + "ep": 4, + "dp-attn": True, + "spec-decoding": "none", + "conc": 8, + "kv-offloading": "dram", + "kv-offload-backend": {"name": "vllm-simple"}, + "total-cpu-dram-gb": 1024, + "duration": 3600, + "exp-name": "test_tp4_conc8_kvdram-vllm-simple", + "scenario-type": "agentic-coding", + }] + + monkeypatch.setattr( + process_changelog, + "get_added_lines", + lambda *_: added_yaml, + ) + monkeypatch.setattr( + process_changelog, + "load_config_files", + lambda _: {"test-config": {}}, + ) + monkeypatch.setattr( + subprocess, + "run", + lambda *_, **__: SimpleNamespace(stdout=json.dumps(generated)), + ) + monkeypatch.setattr(sys, "argv", [ + "process_changelog.py", + "--base-ref", "base", + "--head-ref", "head", + "--changelog-file", "perf-changelog.yaml", + ]) + + process_changelog.main() + + output = json.loads(capsys.readouterr().out) + entry = output["single_node"]["agentic"][0] + assert entry["kv-offload-backend"] == {"name": "vllm-simple"} diff --git a/utils/test_process_result.py b/utils/test_process_result.py index ec6d245cb8..6dcb7fa294 100644 --- a/utils/test_process_result.py +++ b/utils/test_process_result.py @@ -262,8 +262,9 @@ def test_component_metadata_is_emitted_when_present( @pytest.mark.parametrize("metadata", [ {"name": "vllm-router"}, {"name": "vllm-router", "version": "0.1.14", "mode": "round-robin"}, + {"name": "vllm-router", "version": "image:vllm/vllm-openai:latest"}, ]) - def test_component_metadata_rejects_partial_or_extra_fields( + def test_component_metadata_rejects_values_rejected_by_source_schema( self, tmp_path, sample_benchmark_result, single_node_env_vars, metadata ): env = {**single_node_env_vars, "ROUTER_METADATA": json.dumps(metadata)} @@ -271,7 +272,7 @@ def test_component_metadata_rejects_partial_or_extra_fields( result = run_script(tmp_path, env, sample_benchmark_result) assert result.returncode != 0 - assert "must contain exactly 'name' and 'version'" in result.stderr + assert "does not match ComponentMetadata" in result.stderr def test_homogeneous_multinode_omits_hardware_fields( self, tmp_path, sample_benchmark_result, multinode_env_vars