Align result metadata validation with Pydantic schemas / 统一结果元数据与 Pydantic 模式验证

[cquil11]

Summary / 摘要

• Use the source Pydantic models for result metadata validation and omit optional null fields.
• 使用源 Pydantic 模型验证结果元数据，并省略可选的空值字段。

Root cause / 根因

Result processing duplicated schema rules and rejected optional null versions accepted by the source model.

结果处理重复实现了模式规则，并错误拒绝源模型允许的可选空版本。

Tests / 测试

• 84 result-processing tests passed
• 221 matrix/config tests passed

[claude]

Claude finished @cquil11's task in 7m 24s —— View job

---

Review — PR #2208

• Gather context (diff, changed files)
• Read changed files and referenced Pydantic models
• Verify validation logic against source schemas
• Post review feedback

Summary

The refactor itself is sound: ComponentMetadata/KVOffloadBackendMetadata in utils/matrix_logic/validation.py match (and strengthen, via the image: version check) the hand-rolled rules they replace, and I verified the exclude_none=True change in process_changelog.py is safe — the only consumer is run-sweep.yml, whose expressions (matrix.config.router && ... || '') treat a missing key the same as null.

Two issues, both about where these scripts execute rather than the logic:

1. 🔴 utils/process_result.py now imports pydantic (and transitively PyYAML) at the top level, but it runs as python3 utils/process_result.py on the bare self-hosted runner host (benchmark-tmpl.yml:258, benchmark-multinode-tmpl.yml:338) with no dependency install anywhere in the workflow or runner setup. Every other host-run util is stdlib-only by design. If any runner host's system Python lacks these packages, every benchmark on that host fails at result processing — after the GPU time is spent. See inline comment.
2. 🟡 process_agentic_result.py runs under the isolated aiperf venv, whose explicit requirements (utils/agentic-benchmark/requirements.txt) list neither pydantic nor pyyaml — the import relies on aiperf's transitive deps. Recommend declaring them explicitly. ⚠️ Note: the suggestion block in my inline comment on process_agentic_result.py shows content for requirements.txt — do not click "Apply suggestion" there, as GitHub would insert it into the wrong file; add the two lines to utils/agentic-benchmark/requirements.txt manually.

One behavior change to be aware of (not blocking): _validate_kv_offload_env no longer rejects KV_OFFLOAD_BACKEND=none when offloading is enabled. The source schema indeed allows any non-empty name, and benchmark scripts whitelist backends themselves, so this matches the PR's stated intent.

[claude]

🔴 BLOCKING: These new top-level imports make process_result.py depend on pydantic and (transitively, via matrix_logic/validation.py) PyYAML on the bare self-hosted runner host.

Why it matters: This script runs as python3 utils/process_result.py directly on the runner host (.github/workflows/benchmark-tmpl.yml:258, benchmark-multinode-tmpl.yml:338) with no dependency-install step in the workflow, and utils/runner_setup/setup.sh provisions no Python packages. Every other host-run util (validate_agentic_result, validate_scores.py) is deliberately stdlib-only, and this file's only existing third-party-ish import (aggregate_power) is wrapped in a try/except marked "never block on telemetry". On any runner whose system Python lacks pydantic (Ubuntu doesn't ship it), the "Process result" step will crash with ModuleNotFoundError — after the GPU benchmark time has already been spent — for every single-node and multinode run on that host. The unit tests pass in CI because the GitHub-hosted test env installs dev requirements; they don't exercise the runner-host environment.

Fix: Either (a) verify/ensure every benchmark runner host has pydantic + PyYAML in its system Python (e.g. add a pip install step before "Process result" in benchmark-tmpl.yml/benchmark-multinode-tmpl.yml), or (b) keep process_result.py stdlib-only and validate against a schema-equivalent check as before. If you keep the pydantic import, at minimum confirm on the actual runner fleet before merge.

[claude]

🟡 WARNING: This module runs under $AIPERF_PYTHON — the isolated venv built in benchmarks/benchmark_lib.sh:1329 from utils/agentic-benchmark/requirements.txt + the aiperf submodule — and neither explicitly declares pydantic or pyyaml (which utils/matrix_logic/validation.py imports at module load).

Why it matters: The import only works if the aiperf fork's own dependency tree happens to pull in both packages. If it does today, fine — but this is an implicit transitive dependency: if the pinned aiperf branch (cquil11/aiperf-agentx-v1.0) ever drops or reshuffles either dep, agentic aggregation fails at import with ModuleNotFoundError after the benchmark has completed. Unit tests won't catch it because they don't run inside the aiperf venv.

Fix: Add the deps explicitly so the venv is self-describing:

Suggested change

	from pydantic import BaseModel, ValidationError
	from utils.matrix_logic.validation import (
	ComponentMetadata,
	KVOffloadBackendMetadata,
	)
	numpy>=1.24
	pandas>=2.0.0
	aiohttp>=3.10
	transformers>=4.46
	xlsxwriter>=3.2.1
	tqdm>=4.66
	datasets
	tiktoken
	matplotlib
	pydantic>=2.0
	pyyaml

(applies to utils/agentic-benchmark/requirements.txt — please confirm pydantic/pyyaml are currently present in the aiperf venv before merging)

[claude]

No bugs found, but this touches core result-processing/validation code shared by three ingestion scripts (agentic aggregation, single-node result, changelog matrix), so I'd like a human to sign off before merging.

What was reviewed: the switch from duplicated ad-hoc validation to ComponentMetadata/KVOffloadBackendMetadata Pydantic models in process_agentic_result.py and process_result.py, the new exclude_none output for optional fields, and the KV_OFFLOAD_BACKEND=='none' guard that got dropped in _validate_kv_offload_env (checked GH Actions matrix usage of the changelog output — omitted keys resolve the same as null there, so no downstream breakage from exclude_none).

Extended reasoning...

Overview

This PR consolidates ad-hoc metadata validation in utils/agentic/aggregation/process_agentic_result.py and utils/process_result.py to reuse the existing ComponentMetadata/KVOffloadBackendMetadata Pydantic models from utils/matrix_logic/validation.py, and adds exclude_none/model_dump(exclude_none=True) so optional null fields (e.g. an offload backend with no independent version) are omitted from output rather than rejected or emitted as null. utils/process_changelog.py gets the same exclude_none treatment on its final JSON dump. Tests were updated/added across all three producer files with good coverage of the new accept/reject boundaries.

Security risks

None. No auth, crypto, or permission-sensitive code is touched — this is benchmark result/config parsing and validation.

Level of scrutiny

This is a moderate-risk refactor: the changes themselves are mechanical (delegate to a shared schema instead of duplicating rules), but the code sits in the CI result-ingestion and changelog-matrix pipeline used across the whole benchmarking system, so a subtle behavior change here can silently alter aggregated output for every future run. One such change — the removed backend_name == "none" rejection in _validate_kv_offload_env — was surfaced by the bug-hunting system and its verifiers concluded it wasn't a functional bug (there's no enum restricting the KV offload backend name, so a literal "none" is a benign, if odd, edge case). I independently traced the exclude_none change in process_changelog.py through to how search-space-config is consumed in run-sweep.yml's GitHub Actions matrix (matrix.config.pp, etc.) and confirmed a missing key resolves identically to a null value there, so it shouldn't break the sweep matrix generation.

Other factors

Test coverage is substantial (84 result-processing + 221 matrix/config tests per the PR description, with new tests added for the null-version omission and the schema-rejection message changes). Given the pipeline is production-critical for all benchmark ingestion and this changes validation/output shape in three places at once, I think it's worth a human giving it a final look before merge, even though I found nothing incorrect.