Skip to content

[AgentX] vLLM DeepSeek-V4 GB200 disagg test#2282

Closed
wzhao18 wants to merge 24 commits into
mainfrom
wzhao/dsv4-gb200-pd
Closed

[AgentX] vLLM DeepSeek-V4 GB200 disagg test#2282
wzhao18 wants to merge 24 commits into
mainfrom
wzhao/dsv4-gb200-pd

Conversation

@wzhao18

@wzhao18 wzhao18 commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@github-actions

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

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.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 perf-changelog.yaml:4950-4963 — The new perf-changelog.yaml entry lists 4 disagg config-keys (1p1d-dep8-dep8, 1p1d-dep8-dep12, 2p1d-dep8-dep12, 3p1d-dep8-dep16) that are entirely commented out in configs/nvidia-master.yaml, so process_changelog.py's get_config_keys_from_master() will raise ValueError and fail the changelog-validation step in CI. Separately, the entry's pr-link points to PR #2260 instead of this PR (#2282), which will block the reuse-merge tooling's link-validation check, and the description's concurrency lists don't match the actual conc-list values in the corresponding recipe blocks. Either uncomment the four disagg blocks in nvidia-master.yaml (if they are meant to ship) or drop them from the changelog entry, and fix the pr-link and concurrency prose.

    Extended reasoning...

    What's wrong: The changelog entry appended at perf-changelog.yaml:4950-4963 lists five config-keys: dsv4-fp4-gb200-dynamo-vllm-agentic-agg, -1p1d-dep8-dep8, -1p1d-dep8-dep12, -2p1d-dep8-dep12, and -3p1d-dep8-dep16. In configs/nvidia-master.yaml, only the -agentic-agg block is an active (uncommented) top-level key; the other four disagg blocks are entirely commented out (every line, including the key itself, is prefixed with #), so yaml.safe_load/load_config_files never produces them as keys in the parsed master config.

    Where it breaks: utils/process_changelog.py's get_config_keys_from_master() (lines 101-102) does elif key not in master_config: raise ValueError(f\"Config key '{key}' not found in master configs.\") for any literal (non-wildcard) key. main() parses only the added lines of the changelog via get_added_lines() and calls this function for every entry's config_keys (line ~155-158 in the snippet above). Since this changelog entry is itself the addition, all five of its keys get validated, and four of them are missing from master_config — the first missing key raises immediately. This function is invoked by CI: .github/workflows/run-sweep.yml:242 runs process_changelog.py --changelog-file perf-changelog.yaml on PR builds, and per AGENTS.md changelog validation still runs even when the benchmark stages are skipped. So this PR, as written, fails that CI step outright.

    Why existing code doesn't catch it earlier: there's no schema-level cross-check between perf-changelog.yaml and configs/nvidia-master.yaml at authoring time — the mismatch is only caught when process_changelog.py actually resolves the keys against the loaded master config, which only happens in this specific validation script/CI step.

    Step-by-step proof:

    1. CI checks out this PR and runs process_changelog.py --base-ref <main> --head-ref <PR> --changelog-file perf-changelog.yaml.
    2. get_added_lines() extracts the diff-added YAML block, which is exactly the entry at lines 4950-4963, containing all five config-keys.
    3. yaml.safe_load parses that block into one ChangelogEntry with config_keys = [agentic-agg, 1p1d-dep8-dep8, 1p1d-dep8-dep12, 2p1d-dep8-dep12, 3p1d-dep8-dep16].
    4. main() loads master_config from configs/nvidia-master.yaml (and other master configs) via load_config_files, in which the four disagg keys don't exist (they're commented out).
    5. get_config_keys_from_master(entry.config_keys, master_config) iterates the keys; on reaching dsv4-fp4-gb200-dynamo-vllm-agentic-1p1d-dep8-dep8 (not a wildcard, not in master_config), it raises ValueError(\"Config key 'dsv4-fp4-gb200-dynamo-vllm-agentic-1p1d-dep8-dep8' not found in master configs.\").
    6. The workflow step fails, blocking the PR's sweep-gating CI.

    Secondary issues on the same entry: the pr-link is .../pull/2260, but this change is PR #2282. The reuse-merge tooling (prepare_perf_changelog_merge.py's validate_added_pr_link) requires an appended entry's pr-link to equal the merging PR number or be an XXX-style placeholder; #2260 is neither, so canonicalize will raise and block that merge path. Additionally, the description's concurrency lists (e.g. "1P/1D DEP8/DEP8 at [64,128,256,320]") don't match the actual conc-list values in the corresponding (commented-out) recipe blocks ([64, 128, 192, 256], etc., with similar mismatches for the other three disagg configs) — likely leftover text from an earlier draft.

    Fix: Either uncomment the four disagg blocks in nvidia-master.yaml if they're meant to be part of this rollout, or drop those four keys from the changelog entry so only dsv4-fp4-gb200-dynamo-vllm-agentic-agg remains. Update pr-link to .../pull/2282 (or an XXX placeholder), and correct the concurrency lists in the description to match the recipes actually being added.

@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

2 participants