[Test] agentx - #2373
Conversation
|
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 关于重新运行失败任务的文档 |
63f5a4f to
17a62fb
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| compilation-config: '{"cudagraph_mode":"FULL_DECODE_ONLY","mode":0}' | ||
| max-cudagraph-capture-size: 128 | ||
| stream-interval: 10 | ||
| language-model-only: true |
There was a problem hiding this comment.
🔴 This new Kimi-K3 multi-node recipe sets vllm_config.aggregated.language-model-only: true, but Kimi-K3 is a text-only checkpoint with no vision tower — the repo's own validated single-node K3 recipe (kimik3_fp4_b300_vllm.sh:28) explicitly documents NO --language-model-only (text-only checkpoint) as a deliberate difference from the multimodal Kimi-K2.5 template. This looks like a copy-paste leftover from a VLM recipe and should be removed to match the validated config; at best it's a no-op, at worst it breaks vLLM weight loading at startup.
Extended reasoning...
agg-gb300-tp8-agentic.yaml:107 sets language-model-only: true inside vllm_config.aggregated. This flag exists in vLLM to instruct the engine to load only the language_model.*-prefixed submodule of a multimodal checkpoint, discarding the vision tower/encoder weights. It only makes sense — and only has matching weight-name prefixes to act on — for checkpoints that actually ship a vision component.
Kimi-K3 is not such a checkpoint. The repo's own validated single-node recipe for this exact model, benchmarks/single_node/speedbench/kimik3_fp4_b300_vllm.sh:28, explicitly calls out NO --language-model-only (text-only checkpoint) as one of the deliberate differences from the Kimi-K2.5 template (kimik2.5_fp4_b300_vllm.sh:24, which documents --language-model-only (text-only benchmark, no vision) and passes it at line 275 specifically to strip K2.5's vision tower). This PR's recipe header even states it was "Ported from the validated srt-slurm recipe," so the presence of this flag directly contradicts its own stated provenance.
A repo-wide grep confirms the pattern: language-model-only: true appears only in genuine VLM recipes (the minimax-m3 variants, which are multimodal) plus this new K3 file. The deepseek-v4 agentic recipe this file says it follows the layout of does not use the flag either. That makes it very likely a copy-paste artifact carried over from a vision-model template (e.g. minimax-m3 or kimi-k2.5) during recipe authoring, rather than an intentional setting for K3.
Concrete walkthrough of the failure path: (1) The launcher script copies this YAML into the srt-slurm recipe tree and calls srtctl apply -f agg-gb300-tp8-agentic.yaml. (2) srtctl translates vllm_config.aggregated keys into vLLM CLI/engine args, so language-model-only: true becomes --language-model-only (or the equivalent engine kwarg) passed to the vLLM server process for the Kimi-K3 checkpoint. (3) Because the K3 checkpoint stores all its weights at the top level with no language_model.* prefix and no vision submodule to strip, the flag either (a) causes vLLM's multimodal-config loading path to look for a submodule that does not exist, producing a weight-name-mismatch or empty-load error during startup, aborting the sweep before any benchmark runs, or (b) is silently ignored as a no-op, in which case the recipe still diverges from the exact single-node K3 configuration the header claims to port, undermining result comparability.
Fix: delete the language-model-only: true line from the vllm_config.aggregated block, matching the documented, validated single-node K3 config.
Why existing code doesn't catch this: the flag is just a container/engine CLI argument threaded through YAML config by srtctl; there is no schema or model-architecture validation in this repo (or apparently in srtctl) that cross-checks flags like language-model-only against whether the target checkpoint actually has a vision component, so a copy-paste from a VLM template silently passes review until vLLM either errors or diverges at runtime.
| - kimik3-fp4-gb300-dynamo-vllm-agentic-agg | ||
| description: | ||
| - "Add Kimi-K3 GB300 Dynamo-vLLM AgentX aggregate TP8 baseline (concurrency 16): one TP8 aggregate worker across two GB300 4-GPU nodes, upstream srt-slurm v1.0.36, random session-affinity routing." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/TBD |
There was a problem hiding this comment.
🔴 The new perf-changelog.yaml entry (for kimik3-fp4-gb300-dynamo-vllm-agentic-agg) uses pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/TBD instead of the actual PR number 2373. This isn't just a broken link — utils/validate_perf_changelog.py only accepts a real .../pull/<number> link or the exact placeholder string XXX, so TBD will fail the changelog validation gate. Fix by changing the link to .../pull/2373 (or .../pull/XXX if the PR number isn't final yet).
Extended reasoning...
The bug: perf-changelog.yaml:5124 adds a new entry for kimik3-fp4-gb300-dynamo-vllm-agentic-agg with pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/TBD. Every other entry in the file (e.g. pull/2349, pull/2341, pull/2336) uses a concrete PR number, and PR metadata confirms this change is PR #2373 — so the correct value is pull/2373.
Why this fails validation, not just cosmetics: utils/validate_perf_changelog.py defines:
CANONICAL_PR_LINK = re.compile(r"https://github\.com/SemiAnalysisAI/InferenceX/pull/\d+")
PR_LINK_PLACEHOLDERS = {"XXX", "https://github.com/SemiAnalysisAI/InferenceX/pull/XXX"}and in validate_added_pr_link():
if pr_number is None:
if not CANONICAL_PR_LINK.fullmatch(link):
raise ChangelogValidationError(...)
return
expected = f"https://github.com/SemiAnalysisAI/InferenceX/pull/{pr_number}"
if link not in PR_LINK_PLACEHOLDERS and link != expected:
raise ChangelogValidationError(...)TBD is not a recognized placeholder (only the literal XXX is), and it doesn't match \d+ either. So both code paths reject it.
Step-by-step proof:
- On this PR's own CI run,
pr_number = 2373is passed intovalidate_added_pr_link. link = "https://github.com/SemiAnalysisAI/InferenceX/pull/TBD".expected = "https://github.com/SemiAnalysisAI/InferenceX/pull/2373".- Check:
link not in PR_LINK_PLACEHOLDERS→ True (TBD isn't XXX).link != expected→ True (TBD != 2373). - Both conditions true →
raise ChangelogValidationError(f"new PR entry must use {expected!r} or an XXX placeholder; found {link!r}"). - Independently, if this entry ever reached a main-branch validation run (
pr_number=None),CANONICAL_PR_LINK.fullmatch(link)also fails sinceTBDdoesn't match\d+, raising a different but equally fatalChangelogValidationError.
Why nothing else catches this: there's no tooling in this repo that auto-substitutes TBD with the real PR number at merge time — the historical entries were all filled in manually with concrete numbers before merge, and the validator only accepts an exact \d+ or the literal string XXX as an escape hatch for pre-merge state.
Fix: change line 5124 to pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2373, matching the actual PR number, or use the accepted .../pull/XXX placeholder if the number isn't final at commit time.
| UCX_RCACHE_MAX_UNRELEASED: "1024" | ||
| UCX_TLS: "cuda_copy,cuda_ipc,tcp" | ||
| NCCL_P2P_LEVEL: NVL | ||
|
|
There was a problem hiding this comment.
🔴 This new recipe sets IS_MULTINODE: "false" even though it spans 2 nodes with a TP8 aggregate worker — the identical-topology deepseek-v4 sibling (agg-gb300-tp8-mtp-agentic.yaml) sets it to "true" for this exact reason, with a comment explaining that aggregate agentic workers must stay on the multinode result schema. This causes process_agentic_result.py::_gpu_shape() to emit the wrong result schema (single-node pp/dcp_size/pcp_size fields + is_multinode: false instead of the prefill_num_workers/decode_num_workers=0 fields every other agentic-aggregate recipe reports), which also conflicts with configs/nvidia-master.yaml declaring multinode: true for this same config and breaks artifact-reuse/ingestion keying.
Extended reasoning...
The bug: benchmarks/multi_node/srt-slurm-recipes/vllm/kimi-k3/agentic/agg-gb300-tp8-agentic.yaml declares resources.agg_nodes: 2, gpus_per_node: 4, gpus_per_agg: 8 — a single TP8 aggregate worker physically spanning two 4-GPU GB300 nodes. Despite that, benchmark.env.IS_MULTINODE is set to "false" (line 129 in the new file; flagged near line 92 in the reported diff hunk).
Why this is wrong: IS_MULTINODE is not a physical-topology flag in this codebase — it is the schema selector consumed by utils/agentic/aggregation/process_agentic_result.py::_gpu_shape(). I verified this directly:
def _gpu_shape():\n is_multinode = env_bool("IS_MULTINODE")\n ...\n if not is_multinode:\n # single-node branch: emits {pp, dcp_size, pcp_size}, num_gpus = tp*pp*pcp_size\n ...\n # multinode branch: reads PREFILL_NUM_WORKERS/DECODE_NUM_WORKERS etc.,\n # emits num_prefill_gpu/num_decode_gpu fields instead\n```
The directly analogous deepseek-v4 recipe with the *identical* topology (`agg-gb300-tp8-mtp-agentic.yaml`: `gpus_per_node: 4`, `agg_nodes: 2`, `gpus_per_agg: 8`) sets `IS_MULTINODE: "true"` with an explicit comment: *'Keep aggregate workers in the multinode result schema so ingestion uses the zero decode-worker count instead of duplicating TP into P and D.'* Every one of the 8 deepseek-v4 agentic recipes sets this to `"true"` for aggregate agentic workers — including `agg-gb300-tp4-mtp-agentic.yaml`, which is single-node (`agg_nodes: 1`) yet still sets `IS_MULTINODE: "true"`, confirming the flag tracks result-schema convention for agentic-aggregate configs, not raw node count. This kimi-k3 recipe's own header comment says it 'follows the deepseek-v4 agentic layout,' but it diverges on exactly this setting.
**Step-by-step proof of impact:**
1. The job runs with `IS_MULTINODE=false`, `TP=8`, and no `PP_SIZE`/`DCP_SIZE`/`PCP_SIZE` set (so they default to 1 via `env_int`).
2. `_gpu_shape()` takes the `if not is_multinode:` branch: it computes `num_gpus = tp * pp * pcp_size = 8 * 1 * 1 = 8` (numerically correct — the headline GPU count and throughput-per-GPU are unaffected) and emits `fields = {pp: 1, dcp_size: 1, pcp_size: 1}`.
3. `is_multinode: false` and these `pp`/`dcp_size`/`pcp_size` fields get written into the result JSON, instead of the `prefill_num_workers`/`num_prefill_gpu`/`decode_num_workers=0`/`num_decode_gpu=0` fields that every sibling agentic-aggregate result (deepseek-v4 tp4/tp8 variants) carries.
4. `benchmark_lib.sh` separately writes this same `is_multinode=false` into `meta_env.json`, and `utils/validate_reusable_sweep_artifacts.py` keys reusable sweep artifacts as `'single'` vs `'multi'` based on this flag — so this result is bucketed/keyed differently from every other agentic-aggregate config.
5. Meanwhile `configs/nvidia-master.yaml`'s new `kimik3-fp4-gb300-dynamo-vllm-agentic-agg` entry declares `multinode: true`, so the sweep generator expects a multinode-schema result. The emitted artifact (`is_multinode: false`) won't match what ingestion expects for this config, producing a result-schema / reuse-key mismatch even though the throughput numbers themselves are correct.
**Why nothing else catches this:** the single-node branch computes `num_gpus` successfully (no crash), and both branches happen to reduce to 8 GPUs for this exact configuration, so there's no visible failure in the benchmark run itself — only a downstream metadata/schema inconsistency during result ingestion and artifact-reuse matching.
**Fix:** change `IS_MULTINODE: "false"` to `IS_MULTINODE: "true"` on this line, matching the deepseek-v4 sibling recipe and the `multinode: true` declaration already present in `configs/nvidia-master.yaml` for this config.
</details>
<!-- bhfn:1:normal -->
17a62fb to
96e4aa1
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30326164080 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30328311160 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30330678288 |
1 participant
just to test the infra functional