Skip to content

Restore DeepSeek-V4 tool calling for AgentX evals #2295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cquil11 wants to merge 1 commit into
base: main
Choose a base branch
from
+12 −0
Open

Restore DeepSeek-V4 tool calling for AgentX evals #2295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions benchmarks/single_node/agentic/dsv4_fp4_b200_vllm.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,12 +332,14 @@
--gpu-memory-utilization 0.92
--numa-bind
"${CUMEM_ARGS[@]}"
--no-enable-flashinfer-autotune
--tokenizer-mode deepseek_v4
--reasoning-parser deepseek_v4
--tool-call-parser deepseek_v4
--enable-auto-tool-choice
--attention-config '{"backend":"FLASHINFER_MLA_SPARSE_DSV4","use_prefill_query_quantization":true,"use_fp4_indexer_cache":true}'
--no-disable-hybrid-kv-cache-manager
--disable-uvicorn-access-log

Check warning on line 342 in benchmarks/single_node/agentic/dsv4_fp4_b200_vllm.sh

View check run for this annotation

Claude / Claude Code Review

PR title/description missing required Chinese translation 

PR title and description are English-only, but AGENTS.md mandates bilingual PR titles/descriptions (format: `<English title> / <中文标题>` plus a Chinese section in the body).
Comment on lines 335 to 342

@claude claude Bot Jul 21, 2026

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.

🟡 PR title and description are English-only, but AGENTS.md mandates bilingual PR titles/descriptions (format: <English title> / <中文标题> plus a Chinese section in the body).

Extended reasoning...

AGENTS.md explicitly states: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format: <English title> / <中文标题>. In the PR/issue body, follow the English content with its Chinese translation (e.g. a ## 中文说明 section...). This applies to every PR and every issue."

PR #2295's title is "Restore DeepSeek-V4 tool calling for AgentX evals" — no / <中文标题> suffix. The body consists of ## What changed, ## Why, and ## Validation sections, all in English only, with no accompanying ## 中文说明 (or similar) Chinese section.

This is not a stylistic nitpick — it's a repository-specific, explicitly documented mandatory convention, and it's one this repo actively follows: recent merged commits on the same recipe lineage use the bilingual title format, e.g. 01e9e73 [LMCache] Add LMCache arm to official DSV4 FP4 B200 vLLM AgentX sweep / 将 LMCache 分支并入官方 DSV4 FP4 B200 vLLM AgentX 扫描 (#2231) and 9ecc56d [LMCache] Add LMCache arm on tuned DSV4 FP4 B300 vLLM AgentX recipe / 在调优后的 DSV4 FP4 B300 vLLM AgentX 配方上新增 LMCache 分支 (#2232). PR #2295 (this PR, a direct follow-up to #2231) breaks that pattern.

Step-by-step proof:

  1. Open AGENTS.md, line 7 — the bilingual title/body requirement is stated as mandatory ("must be bilingual"), with an explicit title format and a body-structure example.
  2. Compare against this PR's actual title: Restore DeepSeek-V4 tool calling for AgentX evals. Applying the required format, it should read something like Restore DeepSeek-V4 tool calling for AgentX evals / 恢复 DeepSeek-V4 AgentX 评测的工具调用能力.
  3. Compare against this PR's body: sections are ## What changed, ## Why, ## Validation — no ## 中文说明 (or equivalent) section follows them, as the convention requires.
  4. Cross-check against sibling/parent PRs in the same lineage ([LMCache] Add LMCache arm to official DSV4 FP4 B200 vLLM AgentX sweep / 将 LMCache 分支并入官方 DSV4 FP4 B200 vLLM AgentX 扫描 #2231, [LMCache] Add LMCache arm on tuned DSV4 FP4 B300 vLLM AgentX recipe / 在调优后的 DSV4 FP4 B300 vLLM AgentX 配方上新增 LMCache 分支 #2232) merged just before this one — both follow the bilingual format, confirming this isn't a dead-letter rule but one actively enforced in this exact recipe family.

Fix is trivial: retitle to Restore DeepSeek-V4 tool calling for AgentX evals / 恢复 DeepSeek-V4 AgentX 评测的工具调用能力 (or similar) and append a ## 中文说明 section summarizing the What changed / Why / Validation content in Chinese.

This has no runtime or functional impact — it's PR metadata, not code — so it doesn't block correctness, but it is a genuine, actionable deviation from a mandatory, explicitly documented repo convention that this PR's own lineage otherwise follows consistently.

--compilation-config '{"cudagraph_mode":"FULL_DECODE_ONLY","mode":0}'
--max-num-seqs "$MAX_NUM_SEQS"
--max-cudagraph-capture-size "$MAX_NUM_SEQS"
Expand Down
10 changes: 10 additions & 0 deletions perf-changelog.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5003,3 +5003,13 @@
- "LMCache MP server (lmcache_driven transfer mode) + LMCacheMPConnector per PR #2153; L1 pool derated to 75% of TOTAL_CPU_DRAM_GB; --enable-cumem-allocator dropped on the lmcache arm only (cuMem/VMM allocations cannot be CUDA-IPC-exported to the LMCache server)"
- "LMCache TP8 points mirror the vllm-simple ladder (conc [8, 12, 16]); LMCache DEP8 points mirror the Mooncake ladder (conc [12, 20, 28, 36, 44, 52, 60, 68, 76]) for a like-for-like backend comparison; the PR #2231 bring-up sweep validated the recipe across DEP8 conc 8-80 (peak ~25.7k total tok/s/GPU at conc 72, 96-98% cache hit)"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2231

- config-keys:
- dsv4-fp4-b200-vllm-agentic
scenario-type:
- agentic-coding
description:
- "Eval-only follow-up to PR #2231: restore the official DeepSeek-V4 vLLM tool-calling frontend flags --tool-call-parser deepseek_v4 and --enable-auto-tool-choice"
- "Re-run the dedicated AgentX SWE-bench eval to verify tool definitions are accepted instead of failing with 'tool parsing is disabled by frontend configuration'"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2231

@claude claude Bot Jul 21, 2026

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.

🔴 BLOCKING: Appended entry's pr-link points to #2231, but this PR is #2295.

Why it matters: The PR CI gate passes (it runs validate_perf_changelog.py without a PR number), but the merge path does not. utils/prepare_perf_changelog_merge.py (invoked via utils/merge_with_reuse.sh) calls compare_entries(..., pr_number) → validate_added_pr_link(), which requires every entry appended by a PR to use that PR's own link or an XXX placeholder (utils/validate_perf_changelog.py:144-160). It will raise ChangelogValidationError: new PR entry must use 'https://github.com/SemiAnalysisAI/InferenceX/pull/2295' or an XXX placeholder; found '.../pull/2231' and block the merge. utils/recover_failed_ingest.py:326 likewise matches the ingest entry against the merging PR's link.

Fix: Point pr-link at this PR — the description lines already reference #2231, so no context is lost:

Suggested change
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2231
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2295

evals-only: true

Check failure on line 5015 in perf-changelog.yaml

View check run for this annotation

Claude / Claude Code Review

perf-changelog pr-link points to #2231 instead of this PR 

The newly appended perf-changelog.yaml entry sets `pr-link` to `.../pull/2231` instead of this PR's own number (#2295). At merge time `utils/merge_with_reuse.sh` runs `prepare_perf_changelog_merge.py canonicalize --pr-number 2295`, which rejects any newly-appended entry whose `pr-link` isn't the current PR's own link or an `XXX` placeholder — so this will raise `ChangelogValidationError` and abort the standard merge tooling, even though it slips past CI. Fix: use an `XXX` placeholder for `pr-lin
Comment on lines +5006 to +5015

@claude claude Bot Jul 21, 2026

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.

🔴 The newly appended perf-changelog.yaml entry sets pr-link to .../pull/2231 instead of this PR's own number (#2295). At merge time utils/merge_with_reuse.sh runs prepare_perf_changelog_merge.py canonicalize --pr-number 2295, which rejects any newly-appended entry whose pr-link isn't the current PR's own link or an XXX placeholder — so this will raise ChangelogValidationError and abort the standard merge tooling, even though it slips past CI. Fix: use an XXX placeholder for pr-link (auto-canonicalized at merge) and keep the #2231 reference in the description text only.

Extended reasoning...

The bug: The perf-changelog entry appended by this PR (lines 5006-5015) sets:

pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2231

but this PR is #2295, not #2231. utils/validate_perf_changelog.py enforces (via validate_added_pr_link, lines 144-160) that any newly appended entry's pr-link must equal https://github.com/SemiAnalysisAI/InferenceX/pull/<pr_number> for the PR that added it, or be one of the PR_LINK_PLACEHOLDERS ("XXX" / .../pull/XXX). Pointing a brand-new entry at a different, already-merged PR's number satisfies neither condition.

Why CI didn't catch it: The check-changelog step in run-sweep.yml invokes validate_perf_changelog.py with only --base-ref/--head-ref — no --pr-number flag exists on that script at all. With pr_number=None, validate_added_pr_link falls back to the looser branch (line 146-151) that just checks the link is some well-formed .../pull/<digits> URL via CANONICAL_PR_LINK.fullmatch. .../pull/2231 passes that regex trivially, so CI is green, and the PR description's own validation command (which mirrors exactly what CI runs) also doesn't exercise the stricter check.

Why merge would fail: utils/merge_with_reuse.sh unconditionally runs (around lines 181-186):

prepare_perf_changelog_merge.py canonicalize --base-ref origin/main --pr-number "$PR"

This calls canonicalize_appended_links() -> compare_entries(base, head, pr_number=2295) -> validate_added_pr_link(link, 2295). Now pr_number is not None, so the strict branch runs: expected = ".../pull/2295", and since .../pull/2231" is neither expected nor in PR_LINK_PLACEHOLDERS, it raises:

ChangelogValidationError: new PR entry must use '.../pull/2295' or an XXX placeholder; found '.../pull/2231'

prepare_perf_changelog_merge.py's main() catches this and returns 1, and this invocation in merge_with_reuse.sh isn't guarded with if ! (unlike the dry-run canonicalize call earlier in the script), so under set -euo pipefail the merge script aborts outright.

Step-by-step proof:

  1. Base perf-changelog.yaml has N entries; this PR's diff appends entry N+1 with pr-link: .../pull/2231.
  2. Reviewer/CI runs validate_perf_changelog.py --base-ref origin/main --head-ref HEAD (no --pr-number) → pr_number=None → only regex-checks the link is well-formed → passes. PR merges "cleanly" from CI's perspective.
  3. A maintainer later runs utils/merge_with_reuse.sh 2295 to actually integrate the PR.
  4. That script calls prepare_perf_changelog_merge.py canonicalize --pr-number 2295.
  5. compare_entries walks the appended entries, calls validate_added_pr_link(".../pull/2231", 2295).
  6. expected = ".../pull/2295"; .../pull/2231" is not in PR_LINK_PLACEHOLDERS and != expected → ChangelogValidationError raised → main() returns 1 → merge_with_reuse.sh aborts under set -e.

The fix is a one-line change: set pr-link: XXX (or .../pull/XXX) instead of hardcoding .../pull/2231; the canonicalize step will substitute the real PR link (2295) automatically at merge time. The existing description text already references "#2231" for context, which is fine to keep — it's the structured pr-link field that must not point at a different PR.

Loading