Skip to content

fix(vllm): derive expert-parallel world size instead of reading a missing field - #615

Open
nicolasnoble wants to merge 3 commits into
mainfrom
nnoble/mx-440-vllm-sources-always-publish-expert_parallel_size0-so
Open

fix(vllm): derive expert-parallel world size instead of reading a missing field#615
nicolasnoble wants to merge 3 commits into
mainfrom
nnoble/mx-440-vllm-sources-always-publish-expert_parallel_size0-so

Conversation

@nicolasnoble

@nicolasnoble nicolasnoble commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

vLLM's ParallelConfig has no expert_parallel_size attribute. metadata/publish.py read one anyway with a getattr default of 0, so the expert-parallel component of every vLLM mx_source_id has been a constant zero. sglang and TRT-LLM populate it for real, so this is the vLLM path only.

It produces false matches, not failed ones. An MoE deployment with --enable-expert-parallel and one without hash identically, and after #363 both have a worker at worker_rank 0, so the transfer succeeds and delivers the wrong expert weights with nothing raised and no disk fallback.

build_source_identity now computes tp * dp * pcp when enable_expert_parallel is set and 1 otherwise, following vllm/model_executor/layers/fused_moe/config.py. The attribute is prefill_context_parallel_size; the longer name in parallel_state.py is a local parameter, and reading that one would reintroduce the same silent default.

No dp field on SourceIdentity. manifest-azure-dp2.yaml and TEST_PLAN row 5.2 describe that omission as deliberate, DP cores being interchangeable copies, and the corrected product already carries dp. It does leave pcp unhashed, which fits better with the wider topology work.

Adds the first test coverage for build_source_identity, and corrects five places in CI and TEST_PLAN claiming that enabling EP raises the value above zero. None were executable assertions, which is why the ep2 job passed with both sides publishing the same wrong number.

This changes published mx_source_id values for vLLM sources, non-MoE included, without a version change.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected expert-parallel world-size reporting in source identity metadata.
    • Expert-parallel size is now derived from tensor, data, and prefill-context parallelism.
    • Disabled expert parallelism now reports a size of 1 instead of 0.
    • Excluded decode-context parallelism from the calculation.
  • Documentation

    • Updated metadata descriptions, test plans, deployment guidance, and version-bump procedures to reflect the corrected behavior.
  • Tests

    • Added coverage for expert-parallel sizing, source-identity propagation, defaults, and related configuration handling.

…sing field

Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
@copy-pr-bot
copy-pr-bot Bot deployed to automated-release August 12, 2026 19:41 Active
@copy-pr-bot
copy-pr-bot Bot deployed to automated-release August 12, 2026 19:41 Active
@github-actions github-actions Bot added the fix label Aug 12, 2026
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The change derives expert-parallel world size from tp * dp * pcp, updates source identity generation and tests, and aligns workflow, manifest, metadata, test-plan, verification, and fixture documentation.

Changes

Expert-parallel identity calculation

Layer / File(s) Summary
Derive and validate expert-parallel world size
modelexpress_client/python/modelexpress/metadata/publish.py, modelexpress_client/python/tests/test_publish_source_identity.py
build_source_identity now uses the derived expert-parallel size. Tests cover defaults, products, ignored attributes, and preserved identity fields.
Align EP documentation and manifests
.github/workflows/modelexpress-ci-tests.yml, ci/TEST_PLAN.md, ci/k8s/client/vllm/manifest-azure-ep2.yaml, docs/metadata.md
Documentation now describes tp * dp * pcp sizing and the disabled value of 1.
Update verification and version fixtures
CLAUDE.md, modelexpress_server/src/p2p/backend/memory.rs
Verification guidance covers additional fixtures, full source-ID tests, canonicalization checks, and Python development dependencies. The Rust fixture uses version 0.5.0.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Poem

I’m a rabbit counting three,
tp * dp * pcp makes identity.
Tests hop high, docs follow bright,
One when EP sleeps through the night.
Version carrots land just right.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main fix: deriving the vLLM expert-parallel world size instead of reading an unavailable field.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai 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.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CLAUDE.md`:
- Around line 101-117: Update the version-bump procedure in CLAUDE.md to use a
repository-wide text search that includes Markdown, YAML, Helm, and other
relevant files, rather than restricting grep to .rs, .py, and .toml. Instruct
users to classify intentional public-image references separately while updating
stale version fixtures.
- Around line 129-145: Execute the Rust source-identity assertions before
running the Python suite: add cargo test --workspace or a targeted
modelexpress-server source-identity test command, rather than relying on cargo
check --workspace --tests. Preserve the existing cross-check workflow and ensure
the Rust tests actually run.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8218b3f5-f5ec-4335-a4ca-3f6b411b7782

📥 Commits

Reviewing files that changed from the base of the PR and between 89bbb59 and 4e7a06d.

📒 Files selected for processing (8)
  • .github/workflows/modelexpress-ci-tests.yml
  • CLAUDE.md
  • ci/TEST_PLAN.md
  • ci/k8s/client/vllm/manifest-azure-ep2.yaml
  • docs/metadata.md
  • modelexpress_client/python/modelexpress/metadata/publish.py
  • modelexpress_client/python/tests/test_publish_source_identity.py
  • modelexpress_server/src/p2p/backend/memory.rs

Comment thread CLAUDE.md
Comment thread CLAUDE.md
@nicolasnoble
nicolasnoble requested review from zhengluo-nv and removed request for AndyDai-nv August 12, 2026 21:08
…just check

Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
@copy-pr-bot
copy-pr-bot Bot deployed to automated-release August 12, 2026 21:13 Active
@copy-pr-bot
copy-pr-bot Bot deployed to automated-release August 12, 2026 21:13 Active
@nicolasnoble
nicolasnoble enabled auto-merge (squash) August 12, 2026 21:13
Comment thread modelexpress_client/python/modelexpress/metadata/publish.py Outdated
Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
@copy-pr-bot
copy-pr-bot Bot deployed to automated-release August 14, 2026 00:00 Active
@copy-pr-bot
copy-pr-bot Bot deployed to automated-release August 14, 2026 00:00 Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants