Skip to content

fix: Fix DAPO crash due to discard generation weights#2582

Open
guyueh1 wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
guyueh1:fix_dapo
Open

fix: Fix DAPO crash due to discard generation weights#2582
guyueh1 wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
guyueh1:fix_dapo

Conversation

@guyueh1

@guyueh1 guyueh1 commented May 27, 2026

Copy link
Copy Markdown
Contributor

What does this PR do ?

Fix an issue introduced by #2495 : when using dynamic sampling, at end of generation phase, we cannot simply discard generation weights and mark it stale, because if the batch is decided to be incomplete for training, we directly jump to next step and start generation again, in that case marking it stale will trigger a refit which will crash.

This is just a temporal patch, but it does not enable the intended optimization of #2495 for DAPO; I will propose a proper fix in another PR.

Issues

List issues that this PR closes (syntax):

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

  • ...

Signed-off-by: Guyue Huang <guyueh@nvidia.com>
@guyueh1
guyueh1 requested a review from a team as a code owner May 27, 2026 00:49
@copy-pr-bot

copy-pr-bot Bot commented May 27, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@guyueh1 guyueh1 self-assigned this May 27, 2026
@guyueh1 guyueh1 added the CI:L2 Run doctests, unit tests, functional tests, and convergence tests label May 27, 2026
@guyueh1

guyueh1 commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 824462d

Signed-off-by: Guyue Huang <guyueh@nvidia.com>
@guyueh1

guyueh1 commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 232d44f

@sharonyu-115

sharonyu-115 commented May 29, 2026

Copy link
Copy Markdown
Contributor

Hi @guyueh1, I ran into the problem to be fixed by this PR when testing #2224. Then I found #2587 and #2614 seem to address the same problem, and the latter two are quite similar. Looks like the fixes need to be consolidated?

@jQizhang fyi.

@sharonyu-115 sharonyu-115 mentioned this pull request May 29, 2026
4 tasks
@guyueh1

guyueh1 commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

Hi @guyueh1, I ran into the problem to be fixed by this PR when testing #2224. Then I found #2587 and #2614 seem to address the same problem, and the latter two are quite similar. Looks like the fixes need to be consolidated?

@jQizhang fyi.

@sharonyu-115
they fix the same problem but in different ways, using this PR, dapo will basically behave the same as before the breaking commit; using 2614 or 2587, it fixes the breaking commit but will slightly regress the perf (more invocation of refit) but saves cpu memory. The best fix is to not sleep the generation engine at all if the dynamic sampling has not finished a complete batch, but temporally both this or that can work. I'ok with whichever is merged.

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

Labels

CI:L2 Run doctests, unit tests, functional tests, and convergence tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants