Skip to content

Improve performance of request_metadata logic#2378

Merged
tdene merged 9 commits into
NVIDIA:mainfrom
tdene:tde/fix_sampling_tensorize
Dec 9, 2025
Merged

Improve performance of request_metadata logic#2378
tdene merged 9 commits into
NVIDIA:mainfrom
tdene:tde/fix_sampling_tensorize

Conversation

@tdene

@tdene tdene commented Nov 24, 2025

Copy link
Copy Markdown
Contributor

What does this PR do ?

The current dynamic sampling code relies on code that surrounds the request_metadata tensor in the dynamic context.
Profiling, and A/B timeit testing, shows that this request_metadata code is poorly optimized.

This PR will address 3 issues found by @mathemakitten

  1. Copying & dtype-casting GPU tensors takes miliseconds, even with non_blocking=True.
  2. Bucketizing a tensor via torch.unique is orders of magnitude slower than doing it via list comprehension.
    3. In order to fix 1) and 2), the "hashed sampling parameters" tracked metadata must be resurrected.

BENCHMARK INFO:

  • The "golden" benchmark prior to this PR returns an average value of 81.77, with expected noise of +/- 2%
  • The "golden" benchmark after the commit, Split request_metadata 2D tensor into 1D slices, returns an average value of 81.41, with expected noise of +/- 2%. This particular change had a risk of degrading performance, but it does not seem to do so.
  • The "golden" benchmark after the commit, Avoid casting/copying request metadata, returns an average value of 84.87, with expected noise of +/- 2%. This particular change was intended to improve performance, and it does.
  • The "golden" benchmark after the commit, Replace tensorized bucketization with for loop, returns an average value of 85.95, with expected noise of +/- 2%. This particular change was intended to improve performance, and it does.

Contribution process

flowchart LR
    A[Pre-checks] --> B[PR Tests]
    subgraph Code Review/Approval
        C1[Expert Review] --> C2[Final Review]
    end
    B --> C1
    C2 --> D[Merge]
Loading

Pre-checks

  • I want this PR in a versioned release and have added the appropriate Milestone (e.g., Core 0.8)
  • 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

Code review

The following process is enforced via the CODEOWNERS file for changes into megatron/core. For changes outside of megatron/core, it is up to the PR author whether or not to tag the Final Reviewer team.

For MRs into `main` branch

(Step 1): Add PR label Expert Review

(Step 2): Collect the expert reviewers reviews

  1. Attach the Expert Review label when your PR is ready for review.
  2. GitHub auto-assigns expert reviewers based on your changes. They will get notified and pick up your PR soon.

⚠️ Only proceed to the next step once all reviewers have approved, merge-conflict are resolved and the CI is passing.
Final Review might get declined if these requirements are not fulfilled.

(Step 3): Final Review

  1. Add Final Review label
  2. GitHub auto-assigns final reviewers based on your changes. They will get notified and pick up your PR soon.

(Optional Step 4): Cherry-pick into release branch

If this PR also needs to be merged into core_r* release branches, after this PR has been merged, select Cherry-pick to open a new PR into the release branch.

For MRs into `dev` branch The proposed review process for `dev` branch is under active discussion.

MRs are mergable after one approval by either eharper@nvidia.com or zijiey@nvidia.com.

Merging your PR

Any member of core-adlr and core-nemo will be able to merge your PR.

@copy-pr-bot

copy-pr-bot Bot commented Nov 24, 2025

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@mathemakitten mathemakitten self-requested a review November 24, 2025 18:14
@tdene tdene force-pushed the tde/fix_sampling_tensorize branch from fb37c1f to 54e69b7 Compare November 24, 2025 21:50
@tdene tdene requested a review from lmcafee-nvidia November 25, 2025 12:13
@tdene tdene force-pushed the tde/fix_sampling_tensorize branch 3 times, most recently from b4d68aa to 786b9f0 Compare November 30, 2025 00:00
@copy-pr-bot

copy-pr-bot Bot commented Nov 30, 2025

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.

@tdene tdene force-pushed the tde/fix_sampling_tensorize branch 3 times, most recently from 78fa9c8 to d4d4f6f Compare December 2, 2025 14:43
@tdene tdene marked this pull request as ready for review December 2, 2025 15:51
@tdene tdene requested review from a team as code owners December 2, 2025 15:51
@tdene tdene added the Expert Review [deprecated] Apply this label to indicate that your PR is ready for expert review. label Dec 2, 2025
@tdene tdene force-pushed the tde/fix_sampling_tensorize branch 2 times, most recently from f27823f to c7124fe Compare December 4, 2025 15:38
@ko3n1g ko3n1g added this to the Core 0.16 milestone Dec 4, 2025
@tdene tdene force-pushed the tde/fix_sampling_tensorize branch from c7124fe to bffbb99 Compare December 4, 2025 17:46
@tdene tdene force-pushed the tde/fix_sampling_tensorize branch from bffbb99 to 219f5fd Compare December 4, 2025 19:52
@tdene

tdene commented Dec 5, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test 5b2bb81

Comment thread megatron/core/inference/contexts/dynamic_context.py
Comment thread megatron/core/inference/contexts/dynamic_context.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Final Review PR is in the "final review" stage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants