Skip to content

feat(config): add router and KV transfer metadata#2096

Open
cquil11 wants to merge 1 commit into
mainfrom
chore/add-agentx-metadata
Open

feat(config): add router and KV transfer metadata#2096
cquil11 wants to merge 1 commit into
mainfrom
chore/add-agentx-metadata

Conversation

@cquil11

@cquil11 cquil11 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Adds optional strict router and KV-transfer metadata to fixed-sequence and agentic configs, carries it through benchmark workflows, and emits it in aggregate results. When present, each object requires exactly name and version.

@cquil11 cquil11 force-pushed the chore/add-agentx-metadata branch from dd8592d to 438b018 Compare July 6, 2026 16:44
@cquil11 cquil11 marked this pull request as ready for review July 6, 2026 16:49
@cquil11 cquil11 requested a review from a team July 6, 2026 16:49
@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

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


Review: LGTM — no blocking issues found

  • Gather context (diff vs base)
  • Review validation / matrix logic changes
  • Review workflow plumbing
  • Review result aggregation changes
  • Run relevant test suites (pytest blocked by sandbox permissions in this job — relying on CI, which runs these suites)
  • Post findings + line count report

Verified during review:

  • All strict (extra='forbid') Pydantic models that receive these dicts were updated — the 4 matrix-entry models, 3 search-space models, and both master-config entry models; ChangelogMatrixEntry composes the updated models, so the changelog path is covered too.
  • The two run-sweep.yml call sites without explicit router:/kv-transfer: lines (sweep-multi-node-8k1k, sweep-single-node-8k1k) inherit them via the *multi-node-inputs/*single-node-inputs YAML anchors — no caller missed.
  • component_metadata(bmk, val) argument order is correct at all 8 call sites (val is the master-config entry, so search-space overrides top-level as documented), and validate_matrix_entry returns the original dict, so hyphenated keys survive into the workflow matrix.
  • Env plumbing is consistent: ROUTER_METADATA/KV_TRANSFER_METADATA follow the same propagation path as the existing required KV_OFFLOADING for both process_result.py (workflow step) and process_agentic_result.py (inside benchmark_lib.sh).

No master config files were modified, so no perf-changelog.yaml entry is required. Line count report posted inline on generate_sweep_configs.py (📈 +18).

return f"kv{kv_offloading}-{kv_offload_backend}"


def component_metadata(benchmark: dict, config: dict) -> dict:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📊 Line Count Report

  • Total Lines: 1230
  • Base Lines: 1212
  • Change: 📈 +18 lines

Comment on lines +117 to +124
def component_metadata(benchmark: dict, config: dict) -> dict:
"""Resolve optional component metadata, preferring search-space overrides."""
metadata = {}
for field in (Fields.ROUTER, Fields.KV_TRANSFER):
value = benchmark.get(field.value, config.get(field.value))
if value is not None:
metadata[field.value] = value
return metadata

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 utils/process_changelog.py:75 (trim_conc) uses tuple(sorted((k, v) for k, v in entry.items() if k != "conc")) as a dict key, which raises TypeError: unhashable type: 'dict' once the new router/kv-transfer fields land on single-node entries as raw dicts. This latently breaks the feature: any master config declaring top-level or search-space router/kv-transfer will crash the sweep setup step for every sweep-enabled PR that touches it. Fix by mirroring _multinode_parallelism_key's treatment via _freeze_matrix_value, or emit component metadata as (name, version) tuples at the source.

Extended reasoning...

The bug

The new component_metadata() helper in utils/matrix_logic/generate_sweep_configs.py:117 resolves router and kv-transfer into raw dicts ({"name": "...", "version": "..."}) and calls entry.update(component_metadata(bmk, val)) on every single-node fixed-seq and single-node agentic entry (diff lines 470, 584, 669, 697, and their generate_test_config_sweep counterparts at 814, 867, 940, 967). The values survive the JSON round-trip through run-sweep.yml's setup step (matrix generator → json.dumpsprocess_changelog.pyjson.loads).

Downstream, utils/process_changelog.py:trim_conc groups single-node entries by every non-conc field:

key = tuple(sorted((k, v) for k, v in entry.items() if k != "conc"))
groups.setdefault(key, []).append(len(out))

Because entry["router"] / entry["kv-transfer"] are dicts, each (k, dict) element inside key is unhashable, and groups.setdefault(key, ...) raises TypeError: unhashable type: 'dict'. The multi-node branch is safe — it short-circuits at line 68 (if entry.get("prefill") is not None) — but every single-node entry falls through to line 75.

Step-by-step proof

  1. A future config declares router: {name: vllm-router, version: 0.1.14} at either the master-config level or in a fixed-seq-len search-space entry (both are documented in configs/CONFIGS.md).
  2. generate_full_sweep builds a single-node entry and calls entry.update(component_metadata(bmk, val)), attaching entry["router"] = {"name": "vllm-router", "version": "0.1.14"}.
  3. The generator prints json.dumps(matrix_values); process_changelog.py re-parses via json.loads, so the dict shape is preserved.
  4. run-sweep.yml:setup passes --trim-conc whenever the PR carries the sweep-enabled label (env TRIM_CONC=true gates the flag at lines ~215-249; the label is the documented default for most PRs).
  5. process_changelog.py:trim_conc:75 builds tuple(sorted((k, v) for k, v in entry.items() if k != "conc")) — the resulting tuple contains a (str, dict) pair.
  6. groups.setdefault(key, ...) hashes key, hits the nested dict, and raises TypeError: unhashable type: 'dict'.

I reproduced this directly against the current tree:

cd utils && python3 -c 'from process_changelog import trim_conc; \
  trim_conc([{"model": "test", "tp": 8, "conc": 4, \
    "router": {"name": "vllm-router", "version": "0.1.14"}}, \
    {"model": "test", "tp": 8, "conc": 8, \
     "router": {"name": "vllm-router", "version": "0.1.14"}}])'

TypeError: unhashable type: 'dict'.

Why existing code doesn't prevent it

The exact hazard is already solved elsewhere in generate_sweep_configs.py. _multinode_parallelism_key(entry) at line 145 pipes every value through _freeze_matrix_value() (lines 133-142) before building the tuple key precisely because dict/list values must be normalized to hashable equivalents. trim_conc was written when all single-node fields were hashable scalars — its docstring at line 56-58 asserts as much — and the new PR violates that invariant without updating the consumer.

Impact

Latent — no master config currently ships router/kv-transfer. But configs/CONFIGS.md (updated in this PR) documents both fields, and validation.py adds them to SingleNodeSearchSpaceEntry, AgenticCodingSearchSpaceEntry, and SingleNodeMasterConfigEntry. The first PR that follows the docs and gets sweep-enabled will crash the setup job, blocking the entire sweep — and it will look like the fault of that downstream PR, not this one.

Fix

Two clean options:

  1. Freeze the values in trim_conc the same way _multinode_parallelism_key does — replace line 75 with something like tuple(sorted((k, _freeze_matrix_value(v)) for k, v in entry.items() if k != "conc")) (or duplicate the small _freeze_matrix_value helper into process_changelog.py).
  2. Emit router / kv-transfer as (name, version) tuples (or JSON strings) at the entry.update sites in generate_sweep_configs.py, and convert back at consumers.

Option 1 is a one-liner and preserves the emitted schema everywhere else. Either way, the fix should ship with this PR before the first router/kv-transfer consumer config lands.

@cquil11 cquil11 added the agentx AgentX benchmarks, recipes, and infrastructure label Jul 9, 2026 — with ChatGPT Codex Connector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agentx AgentX benchmarks, recipes, and infrastructure

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant