Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .github/workflows/benchmark-multinode-tmpl.yml
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,14 @@ on:
description: "KV offload backend for agentic scenarios when kv-offloading is not none"
required: false
type: string
router:
description: "Optional router name and version metadata as JSON"
required: false
type: string
kv-transfer:
description: "Optional KV transfer name and version metadata as JSON"
required: false
type: string
total-cpu-dram-gb:
description: "Total CPU DRAM in GB for KV offloading"
required: false
Expand Down Expand Up @@ -155,6 +163,8 @@ env:
DURATION: ${{ inputs.duration }}
KV_OFFLOADING: ${{ inputs.kv-offloading }}
KV_OFFLOAD_BACKEND: ${{ inputs.kv-offload-backend }}
ROUTER_METADATA: ${{ inputs.router }}
KV_TRANSFER_METADATA: ${{ inputs.kv-transfer }}
TOTAL_CPU_DRAM_GB: ${{ inputs.total-cpu-dram-gb }}
PYTHONDONTWRITEBYTECODE: '1'
PYTHONPYCACHEPREFIX: /tmp/inferencex-pycache
Expand Down
10 changes: 10 additions & 0 deletions .github/workflows/benchmark-tmpl.yml
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ on:
description: "KV offload backend for agentic scenarios when kv-offloading is not none"
required: false
type: string
router:
description: "Optional router name and version metadata as JSON"
required: false
type: string
kv-transfer:
description: "Optional KV transfer name and version metadata as JSON"
required: false
type: string
total-cpu-dram-gb:
description: "Configured CPU DRAM capacity in GB for KV offloading"
required: false
Expand Down Expand Up @@ -118,6 +126,8 @@ env:
IS_AGENTIC: ${{ inputs.scenario-type == 'agentic-coding' && '1' || '0' }}
KV_OFFLOADING: ${{ inputs.kv-offloading }}
KV_OFFLOAD_BACKEND: ${{ inputs.kv-offload-backend }}
ROUTER_METADATA: ${{ inputs.router }}
KV_TRANSFER_METADATA: ${{ inputs.kv-transfer }}
TOTAL_CPU_DRAM_GB: ${{ inputs.total-cpu-dram-gb }}
DURATION: ${{ inputs.duration }}
AIPERF_FAILED_REQUEST_THRESHOLD: '0.10'
Expand Down
12 changes: 12 additions & 0 deletions .github/workflows/e2e-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
exp-name: ${{ matrix.config.exp-name }}
conc-list: ${{ toJson(matrix.config.conc) }}
spec-decoding: ${{ matrix.config.spec-decoding }}
Expand Down Expand Up @@ -141,6 +143,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
exp-name: ${{ matrix.config.exp-name }}
conc-list: ${{ toJson(matrix.config.conc) }}
spec-decoding: ${{ matrix.config.spec-decoding }}
Expand Down Expand Up @@ -180,6 +184,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
tp: ${{ matrix.config.tp }}
ep: ${{ matrix.config.ep }}
dp-attn: ${{ matrix.config.dp-attn }}
Expand Down Expand Up @@ -218,6 +224,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
conc-list: ${{ toJson(matrix.config.conc) }}
spec-decoding: ${{ matrix.config.spec-decoding }}
disagg: ${{ matrix.config.disagg }}
Expand Down Expand Up @@ -260,6 +268,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
tp: ${{ matrix.config.tp }}
ep: ${{ matrix.config.ep }}
dp-attn: ${{ matrix.config.dp-attn }}
Expand Down Expand Up @@ -290,6 +300,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
tp: ${{ matrix.config.tp }}
ep: ${{ matrix.config.ep }}
dp-attn: ${{ matrix.config.dp-attn }}
Expand Down
14 changes: 14 additions & 0 deletions .github/workflows/run-sweep.yml
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
tp: ${{ matrix.config.tp }}
ep: ${{ matrix.config.ep }}
dp-attn: ${{ matrix.config.dp-attn }}
Expand Down Expand Up @@ -365,6 +367,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
exp-name: ${{ matrix.config.exp-name }}
conc-list: ${{ toJson(matrix.config.conc) }}
spec-decoding: ${{ matrix.config.spec-decoding }}
Expand Down Expand Up @@ -431,6 +435,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
tp: ${{ matrix.config.tp }}
ep: ${{ matrix.config.ep }}
dp-attn: ${{ matrix.config.dp-attn }}
Expand Down Expand Up @@ -484,6 +490,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
tp: ${{ matrix.config.tp }}
ep: ${{ matrix.config.ep }}
dp-attn: ${{ matrix.config.dp-attn }}
Expand Down Expand Up @@ -528,6 +536,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
conc-list: '[${{ matrix.config.conc }}]'
spec-decoding: ${{ matrix.config.spec-decoding }}
disagg: ${{ matrix.config.disagg }}
Expand Down Expand Up @@ -577,6 +587,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
tp: ${{ matrix.config.tp }}
ep: ${{ matrix.config.ep }}
dp-attn: ${{ matrix.config.dp-attn }}
Expand Down Expand Up @@ -615,6 +627,8 @@ jobs:
model-prefix: ${{ matrix.config.model-prefix }}
framework: ${{ matrix.config.framework }}
precision: ${{ matrix.config.precision }}
router: ${{ matrix.config.router && toJson(matrix.config.router) || '' }}
kv-transfer: ${{ matrix.config['kv-transfer'] && toJson(matrix.config['kv-transfer']) || '' }}
conc-list: ${{ toJson(matrix.config.conc) }}
spec-decoding: ${{ matrix.config.spec-decoding }}
disagg: ${{ matrix.config.disagg }}
Expand Down
17 changes: 17 additions & 0 deletions configs/CONFIGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ entry-name:
runner: string
precision: string
framework: string
# Optional defaults for every search-space entry in this config.
router: { name: string, version: string }
kv-transfer: { name: string, version: string }
scenarios:
fixed-seq-len:
- isl: int
Expand All @@ -20,6 +23,12 @@ entry-name:
- { tp: int, conc-start: int, conc-end: int }
# Optionally, specify 'ep' (expert-parallelism) and 'dp-attn' (data parallel attention)
- { tp: int, ep: int, dp-attn: bool, conc-start: int, conc-end: int }
# Optionally, declare router and KV transfer component metadata.
- tp: int
router: { name: string, version: string }
kv-transfer: { name: string, version: string }
conc-start: int
conc-end: int
- ...
- ...
agentic-coding: # optional
Expand Down Expand Up @@ -52,10 +61,18 @@ The below list describes what each field is:
- Note: the step factor between `conc-start` and `conc-end` is 2, so if `conc-start` is 4 and `conc-end` is 128, all concurrencies `4, 8, 16, 32, ..., 128` will be run.
- (Optional) `ep`: An integer representing the expert parallelism level that the configuration will be served at. Default is 1 (no expert parallelism) when not specified.
- (Optional) `dp-attn`: A boolean representing whether or not to activate data parallel attention for the configuration. Default is false when not specified.
- (Optional) `router`: Router metadata containing exactly non-empty `name` and `version` strings.
- (Optional) `kv-transfer`: KV transfer metadata containing exactly non-empty `name` and `version` strings.
- `agentic-coding`: Agentic trace replay benchmarks using real conversation traces. Each entry must have:
- `trace-source`: Identifier for the trace dataset to use.
- `search-space`: Same structure as `fixed-seq-len` search-space entries.

`router` and `kv-transfer` may be omitted independently. When either field is
present, both `name` and `version` are required and additional keys are rejected.
Top-level declarations apply to every scenario and search-space entry in that
master config. A search-space declaration overrides the corresponding top-level
value for that point.

Agentic duration is not a master YAML field. Matrix generation defaults agentic
jobs to 3600 seconds; reusable workflow callers may override the `duration`
input.
Expand Down
26 changes: 26 additions & 0 deletions utils/agentic/aggregation/process_agentic_result.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,24 @@ 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."""
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 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


def _validate_kv_offload_env() -> tuple[str, str]:
kv_offloading = required_env("KV_OFFLOADING")
kv_offload_backend = os.environ.get("KV_OFFLOAD_BACKEND", "")
Expand Down Expand Up @@ -139,6 +157,14 @@ def build_agg(
}
agg.update(multinode_fields)

for env_name, result_field in (
("ROUTER_METADATA", "router"),
("KV_TRANSFER_METADATA", "kv_transfer"),
):
metadata = optional_component_metadata(env_name)
if metadata is not None:
agg[result_field] = metadata

metadata = aggregate.get("metadata")
if isinstance(metadata, dict):
dataset = metadata.get("dataset")
Expand Down
23 changes: 23 additions & 0 deletions utils/agentic/aggregation/test_process_agentic_result.py
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,29 @@ def test_processor_preserves_dataset_provenance(tmp_path: Path):
}


def test_processor_emits_component_metadata_when_present(tmp_path: Path):
result_dir = _write_fixture(tmp_path)
agg = _run_processor(
result_dir,
tmp_path / "out",
env_overrides={
"ROUTER_METADATA": json.dumps({"name": "vllm-router", "version": "0.1.14"}),
"KV_TRANSFER_METADATA": json.dumps({"name": "mooncake", "version": "0.3.11.post1"}),
},
)

assert agg["router"] == {"name": "vllm-router", "version": "0.1.14"}
assert agg["kv_transfer"] == {"name": "mooncake", "version": "0.3.11.post1"}


def test_processor_omits_component_metadata_when_absent(tmp_path: Path):
result_dir = _write_fixture(tmp_path)
agg = _run_processor(result_dir, tmp_path / "out")

assert "router" not in agg
assert "kv_transfer" not in agg


def test_processor_latency_units_are_seconds(tmp_path: Path):
"""aiperf reports ms; legacy schema is seconds. Verify conversion."""
result_dir = _write_fixture(tmp_path)
Expand Down
18 changes: 18 additions & 0 deletions utils/matrix_logic/generate_sweep_configs.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,16 @@
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

"""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

Check failure on line 124 in utils/matrix_logic/generate_sweep_configs.py

View check run for this annotation

Claude / Claude Code Review

trim_conc crashes on router/kv-transfer dict values in single-node entries

`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_ke
Comment on lines +117 to +124

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.



def chunk_multinode_agentic_concurrencies(conc_values: list[int]) -> list[list[int]]:
"""Bound sequential agentic profiles sharing one server allocation."""
size = MAX_MULTINODE_AGENTIC_CONCURRENCIES_PER_ALLOCATION
Expand Down Expand Up @@ -457,6 +467,7 @@
Fields.DISAGG.value: disagg,
Fields.RUN_EVAL.value: False, # Default, may be overridden by mark_eval_entries
}
entry.update(component_metadata(bmk, val))

validate_matrix_entry(entry, is_multinode)
matrix_values.append(entry)
Expand Down Expand Up @@ -570,6 +581,7 @@
if dp_attn is not None:
entry[Fields.DP_ATTN.value] = dp_attn

entry.update(component_metadata(bmk, val))
validate_matrix_entry(entry, is_multinode)
matrix_values.append(entry)

Expand Down Expand Up @@ -654,6 +666,7 @@
Fields.DISAGG.value: disagg,
Fields.SCENARIO_TYPE.value: "agentic-coding",
}
entry.update(component_metadata(bmk, val))
validate_agentic_matrix_entry(entry)
matrix_values.append(entry)
else:
Expand Down Expand Up @@ -681,6 +694,7 @@
}
if kv_offload_backend is not None:
entry[Fields.KV_OFFLOAD_BACKEND.value] = kv_offload_backend
entry.update(component_metadata(bmk, val))
validate_agentic_matrix_entry(entry)
matrix_values.append(entry)

Expand Down Expand Up @@ -797,6 +811,7 @@
Fields.DISAGG.value: disagg,
Fields.RUN_EVAL.value: False,
}
entry.update(component_metadata(bmk, val))
matrix_values.append(validate_matrix_entry(entry, is_multinode=True))
else:
# Single-node config
Expand Down Expand Up @@ -849,6 +864,7 @@
Fields.DISAGG.value: disagg,
Fields.RUN_EVAL.value: False,
}
entry.update(component_metadata(bmk, val))
matrix_values.append(validate_matrix_entry(entry, is_multinode=False))

# ---- Agentic-coding scenarios ----
Expand Down Expand Up @@ -921,6 +937,7 @@
Fields.DISAGG.value: disagg,
Fields.SCENARIO_TYPE.value: "agentic-coding",
}
entry.update(component_metadata(bmk, val))
matrix_values.append(validate_agentic_matrix_entry(entry))
else:
for conc in conc_values:
Expand All @@ -947,6 +964,7 @@
}
if kv_offload_backend is not None:
entry[Fields.KV_OFFLOAD_BACKEND.value] = kv_offload_backend
entry.update(component_metadata(bmk, val))
matrix_values.append(validate_agentic_matrix_entry(entry))

return matrix_values
Expand Down
Loading