Skip to content

[Fix] eplb: reject incompatible recorder modes - #48404

Open
Phoenix3334 wants to merge 5 commits into
sgl-project:mainfrom
Phoenix3334:fix/eplb-recorder-mode-48403
Open

Phoenix3334 wants to merge 5 commits into
sgl-project:mainfrom
Phoenix3334:fix/eplb-recorder-mode-48403

Conversation

@Phoenix3334

@Phoenix3334 Phoenix3334 commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Combining --enable-eplb with per_pass or per_token is accepted at startup but crashes the scheduler on the first rebalance. EPLB requires object dumps, while these recorder modes only support file output.

Reject the combination early with an actionable error. Fixes #48403.

Changes

  • Validate the incompatible modes in handle_eplb_and_dispatch.
  • Add regression coverage for rejected (per_pass, per_token) and allowed (None, stat, stat_approx) modes.
  • Keep the NPU per_pass/per_token file-dump tests without EPLB, while retaining EPLB in the stat/stat_approx cases.

Tests

On 297e54f (v0.5.23):

Check Result
Test-only change (red) 2 failed, 2 passed
Patched -k eplb (green) 2 passed, 5 subtests passed
Full test_server_args.py 322 passed, 1 unrelated test_a5_npu_supports_dcp failure (reproduces without fix)
Non-dummy ServerArgs.resolve_once() Invalid modes rejected; allowed modes resolve

Same failure, reproduced end-to-end: DeepSeek-V4-Flash, 4× B300, TP4/EP4, --eplb-rebalance-num-iterations 1:

Configuration Result
Base + per_pass AssertionError during first rebalance, scheduler exits
Fix + per_pass Startup ValueError before model load
Fix + stat Rebalances successfully; /generate returns HTTP 200

The argument-validation fix was ported from 297e54f to upstream main. Its unit tests and E2E were validated on 297e54f, but have not been independently rerun on main. The NPU recorder-test adjustment is pending NPU CI validation. The regression test calls the real hook directly because the dummy-model path skips that resolution stage.

Accuracy / speed: N/A — argument validation only; valid execution paths are unchanged.


CI States

Latest PR Test (Base): ⏳ Run #42181157401
Latest PR Test (Extra): ❌ Run #42181157223
Latest PR Test (AMD ROCm 10): ⏳ Run #42181157262

Tip

Base CI only covers the core cases, and its workload may not exercise your change. Use /rerun-test to run the tests your change touches, or copy this prompt for an agent:

Find the registered tests this PR affects and run them with /rerun-test, following https://github.com/sgl-project/sglang/blob/main/.agents/skills/rerun-test/SKILL.md

@Phoenix3334 Phoenix3334 changed the title [Fix] eplb: reject detailed recorder modes when EPLB is enabled [Fix] eplb: reject incompatible recorder modes Nov 4, 2026
@Phoenix3334
Phoenix3334 force-pushed the fix/eplb-recorder-mode-48403 branch from da22cce to 9c2f859 Compare November 4, 2026 14:26
@Phoenix3334
Phoenix3334 marked this pull request as ready for review November 4, 2026 14:32

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@BBuf

BBuf commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

/tag-and-rerun-ci

@BBuf

BBuf commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

/rerun-test test/registered/npu/basic_function/parallel_strategy/expert_parallelism/test_npu_expert_distribution_recorder_mode.py

@github-actions github-actions Bot added the run-ci CI: run the baseline test suite on this PR label Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

⛔ test/registered/npu/basic_function/parallel_strategy/expert_parallelism/test_npu_expert_distribution_recorder_mode.py: test/registered/npu/basic_function/parallel_strategy/expert_parallelism/test_npu_expert_distribution_recorder_mode.py is registered for NPU (suite base-b-test-2-npu-a3), not for CUDA or CPU; rerun-test.yml has no NPU job. Rerun it with /rerun-failed-ci, or dispatch the NPU workflow manually.

Copy link
Copy Markdown
Contributor Author

Thanks for triggering the CI rerun. I checked the job logs for the current PR head (156e265b), and wanted to clarify the remaining CI statuses.

Base CI: cancelled jobs, not test assertion failures

  • Base run #41851001393 (attempt 2), especially wait-for-base-b.
  • The base-a CPU/small-GPU jobs passed. During base-b, the waiter initially saw 13 queued jobs; at the next poll, 11 were already marked cancelled, and it emitted base-b jobs failed. This helper treats any completed conclusion other than success/skipped (including cancelled) as failure.
  • Most cancelled GPU jobs never acquired a runner; the two that started on 5090 runners were still installing dependencies when the workflow was cancelled. I found no EPLB test failure in these logs.
  • The workflow has concurrency.cancel-in-progress enabled for PR events, but the specific cancellation trigger is not confirmed. Base CI therefore remains incomplete, not passing.

NPU: rerun command unsupported; NPU workflow passed, but not the targeted file

  • The requested /rerun-test test/registered/npu/basic_function/parallel_strategy/expert_parallelism/test_npu_expert_distribution_recorder_mode.py was rejected by the bot because rerun-test.yml does not have an NPU job. The test belongs to the base-b-test-2-npu-a3 suite.
  • The NPU workflow #41851001065 (attempt 2) subsequently passed, including base-b-test-2-npu-a3 / run (0) (3/3 selected tests passed).
  • However, the selected test list in that job does not include test_npu_expert_distribution_recorder_mode.py. So the workflow success should not be interpreted as a targeted pass for that file. If that specific coverage is required, it needs an NPU-capable dispatch.

The Extra / Extra AMD failures are at the optional-tier label gate (Require opt-in tier label), before running their test suites. Lint passed.

Current assessment: no failure in the checked logs is attributable to the two-file EPLB validation change. Base still needs an uninterrupted run, and the requested recorder-specific NPU test has not been confirmed executed. Could a maintainer help confirm the Base cancellation trigger / appropriate NPU test dispatch if needed? No code changes have been made in response to these CI statuses.

@BBuf

BBuf commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

[BBuf Review Agent] @Phoenix3334 The per_pass/per_token NPU cases still inherit --enable-eplb, so the new check in python/sglang/srt/arg_groups/parallel_hook.py:762 rejects both during startup. Could you omit that flag for those two cases and keep their file-dump checks?

Copy link
Copy Markdown
Contributor Author

Thanks for catching this. Fixed in 74b1dabc: the inherited NPU recorder tests now omit --enable-eplb for per_pass/per_token, while stat/stat_approx still run with EPLB. The existing start/stop/dump flow and .pt file assertions are unchanged. I've also updated the PR description. The NPU-specific change is pending CI validation.

@github-actions

This comment has been minimized.

@BBuf

BBuf commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

[BBuf Review Agent] @Phoenix3334 Checked 74b1dabc, this fixes the startup issue for both detailed recorder modes and keeps the inherited file-dump checks intact.

@BBuf BBuf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The NPU test adjustment fixes the earlier finding.

Copy link
Copy Markdown
Contributor Author

CI follow-up: upstream lint fix and main sync (Nov 5)

For the record, the earlier Lint failure on 74b1dabc was caused by an unrelated import on upstream main, not by this EPLB change. The repository-wide check-indirect-imports hook flagged ForwardMode being imported via forward_batch_info in test_kv_translate_ownership.py.

This separates the fixed upstream-wide lint issue from the EPLB-specific validation and any remaining CI failures on #48404.

@BBuf

BBuf commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

[BBuf Review Agent] @Phoenix3334 Confirmed a7c016f9 includes the upstream ForwardMode import fix, so the earlier Lint failure was unrelated to this PR. The EPLB validation and NPU recorder adjustment are unchanged.

@BBuf

BBuf commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

/rerun-failed-ci

1 similar comment
@BBuf

BBuf commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

/rerun-failed-ci

Phoenix3334 commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor Author

@BBuf Thanks for the reruns and review. I checked the latest CI for head a7c016f9 to help avoid repeated reruns of the same non-test failures:

  • Lint: passed after syncing the upstream ForwardMode import fix.
  • NPU: passed. Importantly, the previously requested test_npu_expert_distribution_recorder_mode.py actually ran this time: all 4 recorder-mode tests passed (stat, stat_approx, per_pass, per_token).
  • Extra / Extra AMD / MLU: their failures are at the opt-in label gate, before tests execute. The missing labels are run-ci-nv-extra, run-ci-amd-extra, and run-ci-mlu respectively. This PR currently has run-ci and npu; rerunning these optional tiers without opting in will produce the same gate failures. These are not EPLB test regressions.
  • Base / Xeon: Base still has two GPU jobs queued, and Xeon remains pending. The Base waiter reported a timeout and proceeded without its stage gate; that does not establish that every GPU job finished.

I haven't found a new EPLB-related failure requiring a code change; I'd be happy to investigate if a concrete test failure appears. Thanks!

This branch has not been deployed

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

Labels

npu run-ci CI: run the baseline test suite on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] EPLB crashes with per_pass/per_token recorder modes

2 participants