Skip to content

[Fix] diffusion: Fix the load duration missing from subclassed server tests - #40657

Merged
mickqian merged 15 commits into
mainfrom
cursor/fix-diffusion-load-time-regression-1b8e
Oct 15, 2026
Merged

mickqian merged 15 commits into
mainfrom
cursor/fix-diffusion-load-time-regression-1b8e

Conversation

@michaelzhang-ai

@michaelzhang-ai michaelzhang-ai commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

The AMD nightly nightly-1-gpu-zimage-turbo has failed on all three ROCm variants since the 2026-09-21 run (job), and was still failing on 2026-10-12:

PerformanceValidationError: [performance] zimage_image_t2i: Load duration missing or invalid: None

Image generation succeeds; the failure is in recording the result. #39206 added a load_time_ms=None parameter to DiffusionServerBase._validate_and_record, and a check in _record_performance_result that rejects a missing load duration, but only updated the base class's own call site. Three tests override test_diffusion_generation and call _validate_and_record(case, perf_record) directly, so they record None even though the server measured its load time:

  • test/registered/amd/test_zimage_turbo.py
  • test/registered/xpu/test_xpu_zimage_turbo.py
  • test/registered/xpu/test_xpu_flux2_dev.py

Modifications

_validate_and_record now takes the ServerContext and reads ctx.load_time_ms itself, as _validate_realtime_performance already does. The three overrides, the base-class call site and test_performance_failure_policy.py pass the context. Since ctx is required, an override still using the old call fails with a TypeError instead of silently recording None.

NVIDIA behaviour is unchanged: the base class used to pass diffusion_server.load_time_ms and now passes diffusion_server.

Verification

  • The NVIDIA diffusion suites (1/2/4-GPU, 5090, diffusion-coverage-check) and multimodal-gen-unit-test passed on this branch at e9616a3 (run).
  • The AMD zimage nightly, dispatched on this branch at c1e4114 with continue_on_error=false (run), passes on rocm10, rocm724 and rocm720. Each job records the server's load time (52.3–58.6 s) instead of failing on None.

An earlier revision also added a guard test that drove the three overrides with a stub server. It has been removed: with ctx required, it no longer catches anything the signature doesn't.

Notes for reviewers

Two XPU issues are left alone because this regression did not cause them:


CI States

Latest PR Test (Base): 🚫 Run #38519229876
Latest PR Test (Extra): ❌ Run #38519229934
Latest PR Test (AMD ROCm 10): ❌ Run #38519229845

cursoragent and others added 2 commits September 22, 2026 00:52
`_validate_and_record` took the load duration as an optional argument that
defaulted to None, and only the base class `_test_diffusion_request` passed
it. The AMD and XPU nightlies override `test_diffusion_generation` and call
`_validate_and_record` themselves, so their summaries carried no load time and
`_record_performance_result` rejected every case with "Load duration missing
or invalid: None" even though the server had measured one.

Read the duration off the `ServerContext` instead, as
`_validate_realtime_performance` already does. Every caller holds the context,
so the measurement can no longer be dropped on the way to the summary.

Co-authored-by: Michael <michaelzhang-ai@users.noreply.github.com>
The three registered diffusion tests that override `test_diffusion_generation`
drive the recording path themselves, so nothing caught them drifting from the
base class contract until the AMD nightly failed. Run each override against a
stub server and assert the context's load time reaches the recorded result.

Co-authored-by: Michael <michaelzhang-ai@users.noreply.github.com>
@github-actions github-actions Bot added the diffusion SGLang Diffusion label Sep 22, 2026
The list of overrides was hand-maintained, which is the same "added X without
updating Y" gap that let the original drift through. Scan the registered tree
for tests that call `_validate_and_record` themselves and require the list to
match.

Co-authored-by: Michael <michaelzhang-ai@users.noreply.github.com>
@michaelzhang-ai
michaelzhang-ai marked this pull request as ready for review September 22, 2026 01:45
@michaelzhang-ai michaelzhang-ai added the run-ci CI: run the baseline test suite on this PR label Sep 22, 2026 — with Cursor
cursoragent and others added 3 commits September 22, 2026 03:04
The completeness check only walked test/registered, so an override added
under python/sglang/multimodal_gen/test — where the diffusion server suites
also subclass DiffusionServerBase — escaped the check meant to catch exactly
that. Scan both trees, and assert the base class's own call site is still
found so a wrong scan root fails loudly instead of matching nothing.

Co-authored-by: Michael <michaelzhang-ai@users.noreply.github.com>
parents[4] silently resolves to the wrong directory if this file is ever
moved, turning a relocation into a confusing red in an unrelated PR. Walk up
until an ancestor actually contains both scanned trees instead.

Co-authored-by: Michael <michaelzhang-ai@users.noreply.github.com>
test/registered/unit/README.md says tests here are CPU-only and register
with register_cpu_ci, and this one needs no GPU either — but base-a-test-cpu
installs python[dev], which omits the diffusion extra it imports.

Co-authored-by: Michael <michaelzhang-ai@users.noreply.github.com>
@cursor

cursor Bot commented Sep 22, 2026

Copy link
Copy Markdown

Superseding my two earlier comments — both name runs that later pushes made stale. Do not rerun 35677085181 or 35684596604.

Current head is dc90df43. Its pr-test.yml run is 35692466008; once it finishes, the action needed is a failed-jobs-only rerun of that run (/rerun-failed-ci, or gh run rerun 35692466008 --failed --repo sgl-project/sglang). It is still finishing only because GPU runners are contended — the remaining jobs fail-fast the moment a runner picks them up.

This change is green

  • Lint — success on dc90df43.
  • call-multimodal-gen-tests / multimodal-gen-unit-test — success, on this head and the previous one. That is the suite carrying the new guard, and it executed for real rather than being collected-and-skipped: all three overrides driven, each recording load=44977.73ms.
  • multimodal-gen-test-1-5090, bcg-diffusion, compute-diffusion-partitions and all ten base-a-test-cpu shards — success.
  • On the earlier head a3f865da, where the 1-GPU diffusion suite got to complete before fail-fast, diffusion-coverage-check reported 45 of 45 cases executed, 45 passed, 0 failed. Those all drive _validate_and_record through the base class against a real server.

Everything red is one root cause

Every other failed job in the run emits this in its own log:

Fail-fast: skipping — root cause job(s): call-multimodal-gen-tests / multimodal-gen-test-1-b200

I checked all twenty individually. wait-for-base-b is a cascade of a cascade, and diffusion-coverage-check fails only because the 2-GPU suite was cut before running.

That root cause is flaky on main, not caused by this PR

Worth flagging to the diffusion owners, since it makes the b200 lane a coin flip for any PR touching multimodal_gen.

The last scheduled main run's b200 job (106570862025) failed the same nvfp4 cases and went green only on its fourth retry:

attempt 1: 2 failed, 3 passed
attempt 2: 2 failed, 3 deselected
attempt 3: 1 failed, 1 passed, 3 deselected
attempt 4: 1 passed, 4 deselected

On that main run, flux1_modelopt_nvfp4_t2i measured e2e=1612.05ms against expected_e2e_ms=836.71 with a 1045.89ms limit — main is ~1.9x over its own threshold on that case. My runs measured 1424–1925ms for the same test, i.e. the same range. qwen_image_2512_modelopt_nvfp4_t2i is similar, measuring 18.5–37.9s against a 9650.06ms baseline.

So the outcome is decided by which retry happens to draw a fast enough sample within the 7-attempt budget. main drew one; these runs did not. It is also not reachable from this diff: validate_e2e reads summary.e2e_ms from collect_metrics(perf_record), which this PR does not touch, and on the base-class path the change is behaviourally identical — it previously passed diffusion_server.load_time_ms as an argument and now reads ctx.load_time_ms off the same object.

Those baselines look due a re-seed, but that is a separate change in a lane this PR does not own, and I have deliberately not touched them — widening a perf gate that happens to be blocking my own PR would be the wrong fix for the wrong reason.

Co-authored-by: quitenode <quitenode@users.noreply.github.com>

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

PR 40657 passes pre-review: the diffusion test helper reads the load duration from the server context, NVIDIA still records the same load_time_ms, and every checklist row passed. LGTM.

@yichiche

Copy link
Copy Markdown
Collaborator

@yichiche

Copy link
Copy Markdown
Collaborator

@michaelzhang-ai heads-up — this PR now conflicts with main (mergeStateStatus: DIRTY) as of fb746818, so CI cannot run to completion.

Could you merge or rebase main into cursor/fix-diffusion-load-time-regression-1b8e? Happy to help if the conflict lands in AMD/ROCm code.

# Conflicts:
#	python/sglang/multimodal_gen/test/unit/test_pi05_modelopt_fp8_cuda.py
@michaelzhang-ai

Copy link
Copy Markdown
Collaborator Author

@yichiche Both are addressed in 49f3f65.

The imageio failure came from test_pi05_modelopt_fp8_cuda.py, which imports sglang.multimodal_gen but was registered on the base-b / 1-gpu-small lane, which doesn't install the diffusion extra. It is unrelated to this change. #44055 fixed it on main by moving that test's registration to the kernel-unit lane, which collided with fb74681 moving the same file into the diffusion unit suite on this branch. I resolved the merge by keeping main's version, so the PR diff is back to its original six files.

CI is re-running on the new head.

Making ctx a required argument already turns a stale override call into a TypeError, so the guard no longer catches anything the signature does not.
@michaelzhang-ai michaelzhang-ai changed the title [diffusion] Fix the load duration missing from subclassed server tests [AMD][diffusion] Fix the load duration missing from subclassed server tests Oct 13, 2026
@michaelzhang-ai

Copy link
Copy Markdown
Collaborator Author

@yichiche Two updates since your approval:

  • I dropped the guard test (test/registered/unit/diffusion/test_diffusion_server_load_time_recording.py). With ctx now a required argument, an override using the old call fails with a TypeError, so the test no longer caught anything extra. The PR is down to the five call-site and signature changes.
  • I dispatched the AMD zimage nightly on this branch at c1e4114 with continue_on_error=false: run 38473414064. zimage_image_t2i passes on rocm10, rocm724 and rocm720, and each job records the load time (52.3–58.6 s) instead of failing on None.

The NVIDIA base-b jobs are being rerun after GitHub API 502s in the fail-fast step; no tests had run in those jobs.

@michaelzhang-ai michaelzhang-ai changed the title [AMD][diffusion] Fix the load duration missing from subclassed server tests [Fix] diffusion: Fix the load duration missing from subclassed server tests Oct 14, 2026
@michaelzhang-ai

michaelzhang-ai commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

@yichiche @HaiShaw @BBuf @mickqian Could you merge this when you get a chance? Thanks!

On ef3a301, every diffusion suite in PR Test that ran passed, including multimodal-gen-unit-test (which covers the one test this PR touches) and diffusion-coverage-check. The AMD zimage nightly also passes on this branch (run). The only diffusion job without a result is multimodal-gen-test-1-gpu (4), which skipped itself behind the same wait-for-base-b error.

The remaining red NVIDIA checks are base-b jobs that never ran a test. wait-for-base-b keeps failing with HttpError: Server Error when it fetches this run's job list, so every rerun ends the same way. The AMD failures (kandinsky6, ipc_a2a, the 2-GPU server startups, KDA, test_score_api) don't touch the diffusion test harness, and the ones I compared also fail on main's scheduled AMD runs.

@mickqian
mickqian disabled auto-merge October 15, 2026 05:38
@mickqian
mickqian merged commit 676c038 into main Oct 15, 2026
495 of 657 checks passed
@mickqian
mickqian deleted the cursor/fix-diffusion-load-time-regression-1b8e branch October 15, 2026 05:38
arathi-hlab added a commit to arathi-hlab/sglang that referenced this pull request Oct 15, 2026
…ject#40657

sgl-project#40657 restored the (ctx, case, perf_record) signature and updated the
XPU call sites on main; the merge kept the load_time_ms kwarg form, which
no longer matches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diffusion SGLang Diffusion run-ci CI: run the baseline test suite on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants