Skip to content

fix bug where Gemma4 is not working with recompute_granularity = "full" - #5324

Merged
ericharper merged 8 commits into
NVIDIA:mainfrom
warpuv:fix_gemma4_recompute_granularity_full
Jul 17, 2026
Merged

fix bug where Gemma4 is not working with recompute_granularity = "full"#5324
ericharper merged 8 commits into
NVIDIA:mainfrom
warpuv:fix_gemma4_recompute_granularity_full

Conversation

@warpuv

@warpuv warpuv commented Jun 12, 2026

Copy link
Copy Markdown
Contributor
  • I, the PR author, have personally reviewed every line of this PR.

What does this PR do ?

fix bug where Gemma4 is not working with recompute_granularity = "full"

Issue tracking

Linked issue: #5323

Contribution process

Pre-checks

  • I have added relevant unit tests
  • I have added relevant functional tests
  • I have added proper typing to my code Typing guidelines
  • I have added relevant documentation
  • I have run the autoformatter.sh on my PR

@warpuv
warpuv requested review from a team as code owners June 12, 2026 18:11
@copy-pr-bot

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

@svcnvidia-nemo-ci
svcnvidia-nemo-ci marked this pull request as draft June 12, 2026 18:11
@github-actions

Copy link
Copy Markdown
Contributor

This PR has been automatically converted to draft because all PRs must start as drafts.

When you are ready for review, click Ready for Review to begin the review process. This will:

  1. Add the oncall reviewer (optional reviewer)
  2. Add required review teams based on your changes

See the contribution guide for more details.

@warpuv
warpuv force-pushed the fix_gemma4_recompute_granularity_full branch from a95a7b1 to 3d7f6bd Compare June 12, 2026 18:15
@ericharper
ericharper requested a review from guihong-nv June 12, 2026 18:17
@warpuv
warpuv force-pushed the fix_gemma4_recompute_granularity_full branch 3 times, most recently from 9315f25 to c8fd994 Compare June 12, 2026 18:33
@warpuv
warpuv marked this pull request as ready for review June 12, 2026 19:40
@svcnvidia-nemo-ci
svcnvidia-nemo-ci requested a review from a team June 12, 2026 19:40
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the Final Review PR is in the "final review" stage label Jun 12, 2026
@warpuv
warpuv force-pushed the fix_gemma4_recompute_granularity_full branch from dcda546 to a7f772e Compare June 12, 2026 23:19
@guihong-nv

Copy link
Copy Markdown
Contributor

@warpuv Thanks for the quick fix. I think the checkpoint arg-splitting is the right direction, but I found one correctness issue that should be fixed before merge.

The reconstruction in checkpointed_forward() currently treats rotary_pos_emb_local is None as meaning “this was a non-dual RoPE input”:

rotary_pos_emb = (
    rotary_pos_emb_global
    if rotary_pos_emb_local is None
    else (rotary_pos_emb_local, rotary_pos_emb_global)
)

That changes semantics for a valid asymmetric dual-RoPE tuple like (None, k_pos_emb): it gets collapsed to k_pos_emb, and then SelfAttention will duplicate non-tuples into (k_pos_emb, k_pos_emb), applying query RoPE even though the caller explicitly disabled it.

Could we preserve whether the original input was dual RoPE and rebuild the tuple whenever it was originally tuple/list, even if the local element is None?

I’d also strongly suggest adding a focused regression test for recompute_granularity="full" with tuple rotary_pos_emb, including an asymmetric case like (None, k_pos_emb).

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-customer Waiting on the original author to respond and removed waiting-on-customer Waiting on the original author to respond labels Jun 14, 2026
@warpuv
warpuv force-pushed the fix_gemma4_recompute_granularity_full branch 4 times, most recently from e457ec9 to 41327aa Compare June 17, 2026 14:54
warpuv added 3 commits June 17, 2026 14:59
Signed-off-by: Yury Parfenov <4665475+warpuv@users.noreply.github.com>
… None. Lint error fix with te_checkpoint

Signed-off-by: Yury Parfenov <4665475+warpuv@users.noreply.github.com>
Signed-off-by: Yury Parfenov <4665475+warpuv@users.noreply.github.com>
@warpuv
warpuv force-pushed the fix_gemma4_recompute_granularity_full branch from 41327aa to f56232b Compare June 17, 2026 14:59
@warpuv

warpuv commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Dear @guihong-nv, thank you for your review and valuable suggestions. I fixed the code and added test.

Signed-off-by: Yury Parfenov <4665475+warpuv@users.noreply.github.com>
@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-customer Waiting on the original author to respond label Jun 17, 2026
@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-maintainers Waiting on maintainers to respond label Jul 9, 2026
@guihong-nv

Copy link
Copy Markdown
Contributor

/ok to test 2473b86

@guihong-nv guihong-nv 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.

LGTM

@Connor-XY

Copy link
Copy Markdown
Contributor

/claude review

@claude claude Bot 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.

LGTM

@Connor-XY
Connor-XY self-requested a review July 10, 2026 06:36
@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-customer Waiting on the original author to respond waiting-on-maintainers Waiting on maintainers to respond and removed waiting-on-customer Waiting on the original author to respond labels Jul 10, 2026
@warpuv

warpuv commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@mcore-oncall

@svcnvidia-nemo-ci svcnvidia-nemo-ci added Approved All necessary approvals have been made and removed Final Review PR is in the "final review" stage labels Jul 17, 2026
@ericharper
ericharper added this pull request to the merge queue Jul 17, 2026
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started!

You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/29596792106

@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-maintainers Waiting on maintainers to respond label Jul 17, 2026
Merged via the queue into NVIDIA:main with commit acc7e64 Jul 17, 2026
92 of 94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved All necessary approvals have been made community-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants