feat(config): add router and KV transfer metadata#2096
Conversation
cquil11
force-pushed
the
chore/add-agentx-metadata
branch
from
dd8592d to
438b018
Compare
|
Claude finished @cquil11's task in 7m 4s —— View job Review: LGTM — no blocking issues found
Verified during review:
No master config files were modified, so no |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return f"kv{kv_offloading}-{kv_offload_backend}" | ||
|
|
||
|
|
||
| def component_metadata(benchmark: dict, config: dict) -> dict: |
There was a problem hiding this comment.
📊 Line Count Report
- Total Lines: 1230
- Base Lines: 1212
- Change: 📈 +18 lines
| 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 |
There was a problem hiding this comment.
🔴 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.dumps → process_changelog.py → json.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
- 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 inconfigs/CONFIGS.md). generate_full_sweepbuilds a single-nodeentryand callsentry.update(component_metadata(bmk, val)), attachingentry["router"] = {"name": "vllm-router", "version": "0.1.14"}.- The generator prints
json.dumps(matrix_values);process_changelog.pyre-parses viajson.loads, so the dict shape is preserved. run-sweep.yml:setuppasses--trim-concwhenever the PR carries thesweep-enabledlabel (envTRIM_CONC=truegates the flag at lines ~215-249; the label is the documented default for most PRs).process_changelog.py:trim_conc:75buildstuple(sorted((k, v) for k, v in entry.items() if k != "conc"))— the resulting tuple contains a(str, dict)pair.groups.setdefault(key, ...)hasheskey, hits the nested dict, and raisesTypeError: 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:
- Freeze the values in
trim_concthe same way_multinode_parallelism_keydoes — replace line 75 with something liketuple(sorted((k, _freeze_matrix_value(v)) for k, v in entry.items() if k != "conc"))(or duplicate the small_freeze_matrix_valuehelper intoprocess_changelog.py). - Emit
router/kv-transferas(name, version)tuples (or JSON strings) at theentry.updatesites ingenerate_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
added
the
agentx
ChatGPT Codex Connector
…data # Conflicts: # configs/CONFIGS.md # utils/matrix_logic/generate_sweep_configs.py # utils/matrix_logic/test_generate_sweep_configs.py # utils/matrix_logic/test_validation.py # utils/matrix_logic/validation.py # utils/test_process_result.py
cquil11
requested review from
1am9trash,
Ankur-singh,
billishyahao,
chunfangamd,
jgangani,
kedarpotdar-nv,
seungrokj and
yctseng0211
as code owners
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
1 similar comment
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
1 participant
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.