Skip to content

[rl] Revert generator initialization race condition fix (#3809)#3871

Open
wwwjn wants to merge 1 commit into
mainfrom
revert-init-fix
Open

[rl] Revert generator initialization race condition fix (#3809)#3871
wwwjn wants to merge 1 commit into
mainfrom
revert-init-fix

Conversation

@wwwjn

@wwwjn wwwjn commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Reverts the initialization reordering from #3809, which spawned the trainer first and moved TorchStore init + policy_version read before generator spawn. Restores the original ordering: spawn trainer and generators together, then init TorchStore and read policy_version.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jul 6, 2026
@wwwjn wwwjn requested a review from tianyu-l July 6, 2026 21:57
Comment on lines +288 to +293
# The trainer runs the model eagerly (CUDA graph capture is generator-only),
# so its HybridEP MoE dispatch uses the blocking path. Force
# non_blocking_capacity_factor=None on any HybridEP dispatcher: a recipe may set
# it on the shared model_spec for the generator (the static, host-sync-free
# non-blocking path that a CUDA graph can capture), and that value must not
# switch the trainer to the non-blocking path.

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.

I didn't get the "must not" part. Non-blocking should work with and without cudagraph, so shouldn't force it here?

If you want different version between trainer and generator. We can do similar things as how we separate DeepEP v2 for trainer and generator.

@wwwjn wwwjn Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Non-blocking should work with and without cudagraph
The "must not" is not accurate, basically I want to allow different capacity factor (either None or a float) which needs to be configurable in trainer and generator. This override is too strict for trainer.

I already tried override to have separate HybridEP for trainer and generator, but the difficulty here is the trainer and generator might want different capacity factor number, but override doesn't support passing the capacity factor as a parameter, plus trainer and generator will receive the same model_spec / same config tree when calling apply_override.

  • For DeepEP, we set cudagraph=false in model_spec (align with trainer), and override generator to be "True" (always fixed).
  • For hybridEP, say if we want capacity_factor = None in trainer, but want capacity_factor to be configurable in generator, it's impossible to do that with current override + unified model.

Basically there's only one capacity_factor field in shared model_spec config, which is not expressive enough for both trainer and generator, and we have to fix one side using prior knowledge (eg, set trainer always to be None), and using the only capacity_factor field for another side

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Solution:

  • Make override configurable: List[string | tuple(string, {"", ""})]

    • Probably check nested overrided config
  • Converter: quantization, LoRA, batchInvariantFlex

Reverts the initialization reordering from #3809, which spawned the
trainer first and moved TorchStore init + policy_version read before
generator spawn. Restores the original ordering: spawn trainer and
generators together, then init TorchStore and read policy_version.
@wwwjn wwwjn force-pushed the revert-init-fix branch from 5011f26 to 04fa22b Compare July 10, 2026 01:34
@wwwjn wwwjn changed the title [rl] revert #3809 and add HybridEP config example [rl] Revert generator initialization race condition fix (#3809) Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-no-td ciflow/rl ciflow/8gpu CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants