Skip to content

fix(megatron): honor policy.logprob_chunk_size in the training loss path - #2872

Merged
terrykong merged 3 commits into
NVIDIA-NeMo:mainfrom
kaloyan-inherent:kally/fix-logprob-chunk-loss-path
Jun 23, 2026
Merged

fix(megatron): honor policy.logprob_chunk_size in the training loss path#2872
terrykong merged 3 commits into
NVIDIA-NeMo:mainfrom
kaloyan-inherent:kally/fix-logprob-chunk-loss-path

Conversation

@kaloyan-inherent

Copy link
Copy Markdown
Contributor

What does this PR do ?

This PR closes #2871 by passing chunk_size for the logprobs calculation.

Issues

Closes #2871

Signed-off-by: Kaloyan <253267049+kaloyan-inherent@users.noreply.github.com>
@kaloyan-inherent
kaloyan-inherent requested review from a team as code owners June 18, 2026 19:35
@copy-pr-bot

copy-pr-bot Bot commented Jun 18, 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.

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

@ZhiyuLi-Nvidia can you review?

@terrykong
terrykong requested a review from ZhiyuLi-Nvidia June 22, 2026 16:54
@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-maintainers Waiting on maintainers to respond label Jun 22, 2026
@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor

Thank you @kaloyan-inherent generally look good to me. Could you just add a equivalence test of logprob function with and without chunk_size?

Comment thread tests/unit/distributed/test_distributed_logprob.py
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-customer Waiting on the original author to respond label Jun 22, 2026
Signed-off-by: Kaloyan <253267049+kaloyan-inherent@users.noreply.github.com>
@kaloyan-inherent

Copy link
Copy Markdown
Contributor Author

Thank you @kaloyan-inherent generally look good to me. Could you just add a equivalence test of logprob function with and without chunk_size?

thanks for the review @ZhiyuLi-Nvidia! sounds good -- i added the test

Comment thread nemo_rl/distributed/model_utils.py Outdated
Comment thread tests/unit/distributed/test_distributed_logprob.py
Signed-off-by: Kaloyan <253267049+kaloyan-inherent@users.noreply.github.com>
@ZhiyuLi-Nvidia ZhiyuLi-Nvidia added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label Jun 22, 2026
@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor

/ok to test c7d05e8

@terrykong
terrykong enabled auto-merge (squash) June 23, 2026 15:59

@ZhiyuLi-Nvidia ZhiyuLi-Nvidia left a comment

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.

Thank you @kaloyan-inherent. LGTM!

@terrykong
terrykong merged commit f67e9e6 into NVIDIA-NeMo:main Jun 23, 2026
76 of 77 checks passed
ashors1 pushed a commit that referenced this pull request Jun 27, 2026
…ath (#2872)

Signed-off-by: Kaloyan <253267049+kaloyan-inherent@users.noreply.github.com>
Signed-off-by: Anna Shors <ashors@nvidia.com>
pthombre added a commit that referenced this pull request Aug 31, 2026
…2872

The rebased Gym PR #2872 tree pins openai==2.44.0 while
sglang==0.5.12.post1 pins openai==2.6.1; no environment installs both
extras together, so declare the conflict and relock. uv.lock is the
final state validated by 'uv lock --check' in the container preflights.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
pthombre added a commit that referenced this pull request Aug 31, 2026
Bump the Gym pin from 84f2b29e to 8e983c57: latest PR #2872 head
(beaa94c8, adds the staging-consolidation refactor 313754e2, swe-agents
orjson reads, and a main merge) with the strict-strip fix 84f2b29e
cherry-picked on top, since the PR still lacks it and dropping it would
reintroduce the runaway-generation regression validated in E5.

The refactor removed nemo_gym.token_id_capture.staging.attribution;
resolve_terminal is now re-exported from the staging package with an
unchanged call signature, so only the import moves. Gym's pyproject.toml
is byte-identical across the bump, so uv.lock is untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
pthombre added a commit that referenced this pull request Sep 1, 2026
Bump the live Gym worktree from Stack #2777 top 577ded84 to the
rebased-on-main PR #2872 head bc17ce17dc9744e16dae94d0fff4749b4811838c
to rerun the fullperf capture experiment against the rebased stack.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pthombre added a commit that referenced this pull request Sep 1, 2026
Gym PR #2872 (rebased on main) pins openai==2.44.0 while
sglang==0.5.12.post1 pins openai==2.6.1. Declare nemo-gym and sglang
extras as conflicting -- no environment installs both together -- so
uv forks openai (2.6.1 for sglang, 2.44.0 for gym) instead of failing
resolution. Relocked with uv 0.11.28 to match the perf container.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pthombre added a commit that referenced this pull request Sep 1, 2026
Bump the Gym pin from 84f2b29e to 8e983c57: latest PR #2872 head
(beaa94c8, adds the staging-consolidation refactor 313754e2, swe-agents
orjson reads, and a main merge) with the strict-strip fix 84f2b29e
cherry-picked on top, since the PR still lacks it and dropping it would
reintroduce the runaway-generation regression validated in E5.

The refactor removed nemo_gym.token_id_capture.staging.attribution;
resolve_terminal is now re-exported from the staging package with an
unchanged call signature, so only the import moves. Gym's pyproject.toml
is byte-identical across the bump, so uv.lock is untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
pthombre added a commit that referenced this pull request Sep 1, 2026
Bump the live Gym worktree from Stack #2777 top 577ded84 to the
rebased-on-main PR #2872 head bc17ce17dc9744e16dae94d0fff4749b4811838c
to rerun the fullperf capture experiment against the rebased stack.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
pthombre added a commit that referenced this pull request Sep 1, 2026
Gym PR #2872 (rebased on main) pins openai==2.44.0 while
sglang==0.5.12.post1 pins openai==2.6.1. Declare nemo-gym and sglang
extras as conflicting -- no environment installs both together -- so
uv forks openai (2.6.1 for sglang, 2.44.0 for gym) instead of failing
resolution. Relocked with uv 0.11.28 to match the perf container.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
pthombre added a commit that referenced this pull request Sep 1, 2026
Bump the Gym pin from 84f2b29e to 8e983c57: latest PR #2872 head
(beaa94c8, adds the staging-consolidation refactor 313754e2, swe-agents
orjson reads, and a main merge) with the strict-strip fix 84f2b29e
cherry-picked on top, since the PR still lacks it and dropping it would
reintroduce the runaway-generation regression validated in E5.

The refactor removed nemo_gym.token_id_capture.staging.attribution;
resolve_terminal is now re-exported from the staging package with an
unchanged call signature, so only the import moves. Gym's pyproject.toml
is byte-identical across the bump, so uv.lock is untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
pthombre added a commit that referenced this pull request Sep 1, 2026
The previous pin d8a11dba existed only in the local submodule clone (never
pushed to NVIDIA-NeMo/Gym), which broke every CI job at submodule checkout
('not our ref') and the submodule fast-forward check. Re-pin to 7dea0e786,
the live head of NVIDIA-NeMo/Gym#2872, and relock for its pyproject delta
(uv 0.11.28, lockfile revision 3).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
pthombre added a commit that referenced this pull request Sep 2, 2026
Pin 7b842eab: verify_and_linearize replaced with the metadata-only
implementation this branch's finalizer now calls (StagedCallBaseSnapshot,
ExtrasCommitment, public route codec + span decision table). The SHA is
pushed to NVIDIA-NeMo/Gym#2872, so CI submodule checkout resolves it.

Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) community-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] policy.logprob_chunk_size is ignored by the training loss path which leads to increased memory

4 participants