Skip to content

Conversation

@yeyu-nvidia
Copy link
Contributor

@yeyu-nvidia yeyu-nvidia commented Jan 13, 2026

What does this PR do?

Code cleanup

Overview:
Remove RMSNorm which is identical to LlamaRMSNorm from transformers.

Usage

# Add a code snippet demonstrating how to use this

Testing

Before your PR is "Ready for review"

  • Make sure you read and follow Contributor guidelines and your commits are signed.
  • Is this change backward compatible?: Yes/No
  • Did you write any new necessary tests?: Yes/No
  • Did you add or update any necessary documentation?: Yes/No
  • Did you update Changelog?: Yes/No

Additional Information

Summary by CodeRabbit

  • Refactor
    • Updated the normalization implementation in the Eagle speculative module.

✏️ Tip: You can customize this high-level summary in your review settings.

@yeyu-nvidia yeyu-nvidia requested a review from a team as a code owner January 13, 2026 17:20
@yeyu-nvidia yeyu-nvidia requested a review from h-guo18 January 13, 2026 17:20
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 13, 2026

📝 Walkthrough

Walkthrough

Removed RMSNorm class from Eagle utilities module and replaced its usage with LlamaRMSNorm in the transformers plugin. The substitution maintains the same constructor signature while switching the normalization implementation source.

Changes

Cohort / File(s) Summary
Eagle utilities removal
modelopt/torch/speculative/eagle/utils.py
Removed RMSNorm class definition (19 lines) and torch.nn import; retained make_causal_mask and expand_mask functions
Transformers plugin substitution
modelopt/torch/speculative/plugins/transformers.py
Replaced RMSNorm import from Eagle utilities with LlamaRMSNorm usage in EagleModule initialization when use_last_layernorm is enabled

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: removing a duplicated RMSNorm class and replacing it with LlamaRMSNorm from transformers library.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@yeyu-nvidia yeyu-nvidia removed the request for review from h-guo18 January 13, 2026 17:20
@yeyu-nvidia yeyu-nvidia enabled auto-merge (squash) January 13, 2026 17:22
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
modelopt/torch/speculative/plugins/transformers.py (1)

221-222: Use keyword argument for consistency with existing code.

Lines 263-266 already use LlamaRMSNorm with the keyword argument style (eps=config.rms_norm_eps). Using the same style here improves consistency and makes the parameter's purpose clearer.

Suggested change
         if config.use_last_layernorm:
-            self.norm = LlamaRMSNorm(config.hidden_size, config.rms_norm_eps)
+            self.norm = LlamaRMSNorm(config.hidden_size, eps=config.rms_norm_eps)
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e0d365 and 6f61827.

📒 Files selected for processing (2)
  • modelopt/torch/speculative/eagle/utils.py
  • modelopt/torch/speculative/plugins/transformers.py
💤 Files with no reviewable changes (1)
  • modelopt/torch/speculative/eagle/utils.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: linux
  • GitHub Check: wait-checks / wait
  • GitHub Check: wait-checks / wait
  • GitHub Check: build-docs
  • GitHub Check: code-quality
🔇 Additional comments (1)
modelopt/torch/speculative/plugins/transformers.py (1)

52-52: LGTM!

The import cleanup correctly removes the now-unused RMSNorm from the local utils module.

@codecov
Copy link

codecov bot commented Jan 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.61%. Comparing base (5e0d365) to head (6f61827).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #774      +/-   ##
==========================================
- Coverage   74.62%   74.61%   -0.02%     
==========================================
  Files         192      192              
  Lines       18989    18977      -12     
==========================================
- Hits        14171    14159      -12     
  Misses       4818     4818              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yeyu-nvidia yeyu-nvidia merged commit 90fa48c into main Jan 13, 2026
36 checks passed
@yeyu-nvidia yeyu-nvidia deleted the yeyu/remove_duplicate_RMSNorm branch January 13, 2026 19:26
kevalmorabia97 pushed a commit that referenced this pull request Jan 13, 2026
## What does this PR do?
Code cleanup

**Overview:** 
Remove RMSNorm which is identical to LlamaRMSNorm from transformers.

## Usage
<!-- You can potentially add a usage example below. -->

```python
# Add a code snippet demonstrating how to use this
```

## Testing
<!-- Mention how have you tested your change if applicable. -->

## Before your PR is "*Ready for review*"
<!-- If you haven't finished some of the above items you can still open
`Draft` PR. -->

- **Make sure you read and follow [Contributor
guidelines](https://github.com/NVIDIA/Model-Optimizer/blob/main/CONTRIBUTING.md)**
and your commits are signed.
- **Is this change backward compatible?**: Yes/No <!--- If No, explain
why. -->
- **Did you write any new necessary tests?**: Yes/No
- **Did you add or update any necessary documentation?**: Yes/No
- **Did you update
[Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?**:
Yes/No <!--- Only for new features, API changes, critical bug fixes or
bw breaking changes. -->

## Additional Information
<!-- E.g. related issue. -->


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Refactor**
* Updated the normalization implementation in the Eagle speculative
module.

<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Ye Yu <yeyu@nvidia.com>
jingyu-ml pushed a commit that referenced this pull request Jan 14, 2026
## What does this PR do?
Code cleanup

**Overview:**
Remove RMSNorm which is identical to LlamaRMSNorm from transformers.

## Usage
<!-- You can potentially add a usage example below. -->

```python
# Add a code snippet demonstrating how to use this
```

## Testing
<!-- Mention how have you tested your change if applicable. -->

## Before your PR is "*Ready for review*"
<!-- If you haven't finished some of the above items you can still open
`Draft` PR. -->

- **Make sure you read and follow [Contributor
guidelines](https://github.com/NVIDIA/Model-Optimizer/blob/main/CONTRIBUTING.md)**
and your commits are signed.
- **Is this change backward compatible?**: Yes/No <!--- If No, explain
why. -->
- **Did you write any new necessary tests?**: Yes/No
- **Did you add or update any necessary documentation?**: Yes/No
- **Did you update
[Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?**:
Yes/No <!--- Only for new features, API changes, critical bug fixes or
bw breaking changes. -->

## Additional Information
<!-- E.g. related issue. -->

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Refactor**
* Updated the normalization implementation in the Eagle speculative
module.

<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Ye Yu <yeyu@nvidia.com>
Signed-off-by: Jingyu Xin <jingyux@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants