[diffusion] Support RL rollout for the Wan pipeline via a per-request scheduler switch#30036
[diffusion] Support RL rollout for the Wan pipeline via a per-request scheduler switch#30036zhihengy wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the WanRolloutScheduler and WanTimestepPreparationStage to support both normal inference (using UniPC) and RL rollout (using Euler SDE) in the Wan video diffusion pipeline. The review feedback highlights critical improvements for concurrent serving environments: first, to prevent race conditions, the shared template scheduler should not be mutated directly, and a request-specific scheduler instance should be used instead; second, to avoid log flooding, the logging guard should be changed from an instance variable to a class-level variable since schedulers are cloned per request.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def forward(self, batch, server_args): | ||
| self.scheduler.prepare_for_batch(batch) | ||
| return super().forward(batch, server_args) |
There was a problem hiding this comment.
Mutating the shared template scheduler (self.scheduler) in WanTimestepPreparationStage.forward can lead to race conditions in concurrent serving environments where multiple requests are processed simultaneously.
Instead, you should first obtain the request-specific scheduler instance using get_or_create_request_scheduler and then call prepare_for_batch on that request-specific instance. This ensures that the state is isolated per request.
| def forward(self, batch, server_args): | |
| self.scheduler.prepare_for_batch(batch) | |
| return super().forward(batch, server_args) | |
| def forward(self, batch, server_args): | |
| from sglang.multimodal_gen.runtime.pipelines_core.diffusion_scheduler_utils import ( | |
| get_or_create_request_scheduler, | |
| ) | |
| scheduler = get_or_create_request_scheduler(batch, self.scheduler) | |
| scheduler.prepare_for_batch(batch) | |
| return super().forward(batch, server_args) |
| class WanRolloutScheduler(SchedulerRLMixin): | ||
| """Use UniPC for normal Wan inference and Euler SDE for RL rollout.""" | ||
|
|
||
| def __init__(self, shift: float | None): | ||
| self.unipc_scheduler = FlowUniPCMultistepScheduler(shift=shift) | ||
| self.euler_scheduler = FlowMatchEulerDiscreteScheduler( | ||
| shift=1.0 if shift is None else shift | ||
| ) | ||
| self._active_scheduler = self.unipc_scheduler | ||
| self._logged_rollout_euler_check = False |
There was a problem hiding this comment.
Since the scheduler is copied/cloned per request (via get_or_create_request_scheduler), instance variables like self._logged_rollout_euler_check will be reset to False for each new request's scheduler instance. This will cause the info log to be printed once per request, potentially flooding the logs in high-throughput environments.
Using a class-level variable instead of an instance variable ensures that the log is printed only once globally.
| class WanRolloutScheduler(SchedulerRLMixin): | |
| """Use UniPC for normal Wan inference and Euler SDE for RL rollout.""" | |
| def __init__(self, shift: float | None): | |
| self.unipc_scheduler = FlowUniPCMultistepScheduler(shift=shift) | |
| self.euler_scheduler = FlowMatchEulerDiscreteScheduler( | |
| shift=1.0 if shift is None else shift | |
| ) | |
| self._active_scheduler = self.unipc_scheduler | |
| self._logged_rollout_euler_check = False | |
| class WanRolloutScheduler(SchedulerRLMixin): | |
| """Use UniPC for normal Wan inference and Euler SDE for RL rollout.""" | |
| _logged_rollout_euler_check = False | |
| def __init__(self, shift: float | None): | |
| self.unipc_scheduler = FlowUniPCMultistepScheduler(shift=shift) | |
| self.euler_scheduler = FlowMatchEulerDiscreteScheduler( | |
| shift=1.0 if shift is None else shift | |
| ) | |
| self._active_scheduler = self.unipc_scheduler |
| if not self._logged_rollout_euler_check: | ||
| logger.info( | ||
| "Wan rollout using FlowMatchEulerDiscreteScheduler " | ||
| "(timesteps dtype=%s, sigmas dtype=%s, max_abs_diff=%.6g)", | ||
| timesteps.dtype, | ||
| sigmas.dtype, | ||
| max_abs_diff, | ||
| ) | ||
| self._logged_rollout_euler_check = True |
There was a problem hiding this comment.
Update the check to use the class-level _logged_rollout_euler_check variable to ensure the log is printed only once globally.
| if not self._logged_rollout_euler_check: | |
| logger.info( | |
| "Wan rollout using FlowMatchEulerDiscreteScheduler " | |
| "(timesteps dtype=%s, sigmas dtype=%s, max_abs_diff=%.6g)", | |
| timesteps.dtype, | |
| sigmas.dtype, | |
| max_abs_diff, | |
| ) | |
| self._logged_rollout_euler_check = True | |
| if not WanRolloutScheduler._logged_rollout_euler_check: | |
| logger.info( | |
| "Wan rollout using FlowMatchEulerDiscreteScheduler " | |
| "(timesteps dtype=%s, sigmas dtype=%s, max_abs_diff=%.6g)", | |
| timesteps.dtype, | |
| sigmas.dtype, | |
| max_abs_diff, | |
| ) | |
| WanRolloutScheduler._logged_rollout_euler_check = True |
fee9210 to
6845f55
Compare
… scheduler switch Serving and RL rollout need different schedulers on one engine: Wan serves with UniPC, while the rollout SDE/log-prob path requires a first-order flow-match Euler scheduler. Add a generic RolloutSchedulerSwitch (post_training) that holds both and dispatches per request on batch.rollout, plus a RolloutTimestepPreparationStage that resolves the switch before timestep preparation. WanPipeline binds UniPC/Euler; other pipelines whose serving scheduler is not first-order (e.g. Wan I2V, also UniPC) can bind the same switch.
6845f55 to
e33ecb2
Compare
Resolve the scheduler via get_or_create_request_scheduler before switching so the switch follows the request scheduler under any isolation policy, and make the rollout sanity-check log flag class-level so per-request clones do not re-log.
|
/tag-run-ci-label |
…mestep stage mickqian flagged that unconditionally wiring RolloutTimestepPreparationStage (and wrapping the scheduler in RolloutSchedulerSwitch) into WanPipeline is confusing for a pure-serving engine. Rework to the same dormant pattern the codebase already uses for RolloutDenoisingMixin on the standard DenoisingStage: - TimestepPreparationStage takes an optional rollout_scheduler and binds it to batch.scheduler only for rollout=True requests; downstream stages already read batch.scheduler, so this is the single switch point. - WanPipeline keeps a plain FlowUniPCMultistepScheduler module (identical to main) and passes the flow-match Euler rollout scheduler to the standard stage; RolloutSchedulerSwitch and its delegation shims are deleted. - The rollout timesteps/sigmas convention check moves into the stage and only runs when the rollout scheduler was bound. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The rollout binding is exercised end-to-end by downstream GRPO training; keep the PR surface minimal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The isolate=True rationale applied to the old scheduler-held flag; the stage is not cloned per request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… post_training Keep the core stage lean, mirroring RolloutDenoisingMixin on DenoisingStage: scheduler selection, the timesteps/sigmas convention check, and the once-per-process log flag now live in post_training/rollout_timestep_mixin.py. The stage keeps only the rollout_scheduler parameter and two call sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
With the scheduler module now a bare UniPC (no longer the SchedulerRLMixin wrapper), the mixin's isinstance check and RL calls on self.scheduler rejected every Wan rollout request. Resolve the scheduler from batch.scheduler (bound by the timestep stage) with the stage module as fallback for pipelines that never bind one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| self.add_standard_timestep_preparation_stage() | ||
| # Serving keeps UniPC; requests with rollout=True bind a first-order | ||
| # flow-match Euler scheduler, which the RL SDE/log-prob path requires. | ||
| self.add_stage( |
There was a problem hiding this comment.
could we make sure only one scheduler is initialized and passed at runtime?
Motivation
mainalready ships the diffusion RL rollout infrastructure (POST /rollout/generate,SchedulerRLMixin,RolloutDenoisingMixin), but Wan is not wired into it:WanPipelineuses a bareFlowUniPCMultistepScheduler, so rollout requests cannot run SDE sampling with log-probs. Serving and rollout need different schedulers on one engine — Wan serves with UniPC while the rollout SDE/log-prob path requires first-order flow-match Euler — and the choice must be per request, since one engine serves both rollout (rollout=True) and eval (rollout=False) requests within a training run.Modifications
Same dormant pattern as
RolloutDenoisingMixinon the standardDenoisingStage: rollout logic lives underpost_training/and only activates when a request setsrollout=True; a pure-serving engine assembles and runs exactly as onmain.post_training/rollout_timestep_mixin.py(new) —RolloutTimestepPreparationMixin: per-request scheduler selection onbatch.rollout, plus a sanity check of the flow-match Euler conventiontimesteps == sigmas[:-1] * num_train_timestepsthat the SDE/log-prob math relies on (fails loudly on a mismatched scheduler; logs the active rollout scheduler once per process).pipelines_core/stages/timestep_preparation.py—TimestepPreparationStagemixes in the above and takes an optionalrollout_scheduler(defaultNone; existing pipelines unchanged). Downstream stages readbatch.scheduler, so this stage is the single binding point.pipelines/wan_pipeline.py— binding only: the scheduler module stays a bare UniPC (identical tomain); the pipeline passes aFlowMatchEulerDiscreteScheduler(same launch-timeflow_shift) as the stage'srollout_scheduler.Other pipelines whose serving scheduler is not first-order Euler (Wan I2V, SANA-class) can adopt the same one-line binding.
Accuracy Tests
rollout=Falsebinds UniPC,rollout=Truebinds flow-match Euler; pipelines without arollout_schedulerare unaffected; the convention check rejects a mismatched scheduler. Fullmultimodal_gen/test/unitsuite unchanged before/after.log_probagreement at the bf16 noise floor (~1e-5).Checklist
🤖 Generated with Claude Code
CI States
Latest PR Test (Base): ❌ Run #28832550262
Latest PR Test (Extra): ❌ Run #28832550138