[chore] bump kimi-k2.5 int4 b200/b300 vllm images to v0.25.0#2256
[chore] bump kimi-k2.5 int4 b200/b300 vllm images to v0.25.0#2256xinli-sw wants to merge 2 commits into
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 关于重新运行失败任务的文档 |
1093e5d to
6c94e1d
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
configs/nvidia-master.yaml:3126-3132— The PR title and description don't follow AGENTS.md's mandatory bilingual convention (<English title> / <中文标题>, plus a body with a Chinese translation). Sibling PRs in the git log (e.g. #2224, #2244) follow this format; this one is English-only with an empty body. Not blocking merge, but should be brought into compliance.Extended reasoning...
AGENTS.md (top of file, line 7) states in a blockquote-emphasized rule that overrides default behavior: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English." The required title format is
<English title> / <中文标题>, and the body should follow the English content with a Chinese translation (e.g. a## 中文说明section). The rule explicitly states "This applies to every PR and every issue."PR #2256's title is
[chore] bump kimi-k2.5 int4 b200/b300 vllm images to v0.25.0— purely English, with no/ <中文标题>suffix — and the PR body is empty (no description, no## 中文说明section at all).This is easy to verify by comparing against sibling PRs merged into the same repo just before this one: commit
4bbb2acis titled[AgentX] vLLM DeepSeek-V4 B200 aggregate / vLLM DeepSeek-V4 B200 聚合配置 (#2224), and commit1e44aa8is titledSimplify reusable sweep ingestion with direct source dispatch / 通过直接分发源运行简化复用扫描摄取 (#2244). Both follow the mandated bilingual title format; PR #2256 does not. This confirms the bilingual title/description requirement is an actively-enforced convention in this repo, not a dead rule.Step-by-step proof:
- Open AGENTS.md line 7 — the bilingual title/description rule, marked as applying to every PR.
- Open PR #2256's metadata — title field contains only English text, no
/ <中文>segment; body field is empty. - Compare against #2224 and #2244 titles in the git log — both contain the bilingual
/ <中文>suffix, showing the convention is followed elsewhere in the same repo history. - Conclusion: PR #2256 violates the explicit, repo-mandated bilingual title/description rule.
Why this doesn't block merge: the change itself (bumping two vLLM image tags from v0.24.0 to v0.25.0, and removing a now-stale NOTE comment about B300 lacking a dedicated recipe) is functionally correct and low-risk — nothing in the actual YAML diff is affected by this issue. Unlike the perf-changelog check, this bilingual title/description rule isn't wired into a blocking CI gate in this repo, so it's a metadata/policy compliance gap rather than a functional defect. The fix is trivial: the author (or a maintainer editing the PR) should update the title to
[chore] bump kimi-k2.5 int4 b200/b300 vllm images to v0.25.0 / [chore] 将 kimi-k2.5 int4 b200/b300 vllm 镜像升级到 v0.25.0and add a short bilingual description to the PR body. -
🔴
configs/nvidia-master.yaml:3129— This PR bumps the vLLM image tag forkimik2.5-int4-b200-vllmandkimik2.5-int4-b300-vllm(v0.24.0 → v0.25.0) but does not append the requiredperf-changelog.yamlentry. Per AGENTS.md's 'Updating Docker images' section, this is required and is what triggers the benchmark sweep for the changed configs — please add an entry (e.g. mirroring the prior bump's entry) before merging.Extended reasoning...
What's missing:
configs/nvidia-master.yamlis the only file changed in this PR — it bumps theimage:tag for bothkimik2.5-int4-b200-vllmandkimik2.5-int4-b300-vllmfromvllm/vllm-openai:v0.24.0tovllm/vllm-openai:v0.25.0. There is no corresponding addition toperf-changelog.yaml.Why this matters (not just style): AGENTS.md's 'Updating Docker images' section is explicit: 'Update the image tag ... and append a
perf-changelog.yamlentry (required - triggers benchmarks).' This isn't a documentation nicety — the changelog is the mechanism that drives which configs actually get swept/validated. The repo's own.github/workflows/claude-pr-review.ymlcodifies this exact check as a blocking review item: if a master config file is edited andperf-changelog.yamlis not edited in the same PR, reviewers should flag that the changelog must also be updated.Proof / verification steps:
git show --staton this PR's commit shows onlyconfigs/nvidia-master.yamlchanged (2 insertions, 6 deletions) —perf-changelog.yamlis untouched.- Tailing
perf-changelog.yamlat HEAD shows the most recent entry references PR #2224 (an unrelated DSv4 AgentX change); there is no entry mentioningkimik2.5-int4-b200-vllm,kimik2.5-int4-b300-vllm, or v0.25.0 anywhere near the tail. - Every previous image bump for these same two config keys (e.g. the v0.21.0 → v0.24.0 bump) has a matching
perf-changelog.yamlentry withconfig-keys, adescriptionline noting the image update, and apr-link. This PR breaks that established pattern.
Impact if merged as-is: Because the changelog entry is what triggers the benchmark sweep for the touched configs, the v0.25.0 image bump may not actually get benchmarked/validated through the normal pipeline, defeating the purpose of the version bump (there's also a risk that whatever mechanism ingests the changelog to select configs for sweeping simply skips these two configs).
Fix: Append a
perf-changelog.yamlentry at the end of the file following the existing format, e.g.:- config-keys: - kimik2.5-int4-b200-vllm - kimik2.5-int4-b300-vllm description: - "Bump vLLM image from v0.24.0 to v0.25.0" pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2256
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29553384191 |
No description provided.