Merged
Conversation
kevalmorabia97
requested review from
h-guo18,
realAsma,
shengliangxu and
yeyu-nvidia
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoved git-lfs from CI, adjusted example dependency lists, relaxed a transformers constraint, added tiktoken to hf extras, updated speculative/transformers plugin rope/cache/dtype handling, refactored PTQ test utilities and tests to use centralized command + explicit calibration dataset, and made speculative-decoding tests generate datasets at runtime. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
kevalmorabia97
force-pushed
the
kmorabia/spec-dec-tests
branch
from
2a82a07 to
c3526bb
Compare
Contributor
|
shengliangxu
reviewed
Apr 6, 2026
h-guo18
reviewed
Apr 6, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pyproject.toml (1)
85-85: Consider adding a version constraint fortiktoken.While several dependencies in the
hfgroup are unpinned (e.g.,nltk,wonderwords), many critical ones have version constraints (e.g.,transformers>=4.56,<5.0,peft>=0.17.0,sentencepiece>=0.2.1). Adding a minimum version constraint fortiktokenwould help ensure compatibility and prevent potential issues with older versions.📌 Example version constraint
- "tiktoken", + "tiktoken>=0.5.0",Note: The specific version should be chosen based on the minimum version required by your codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 85, The dependency entry "tiktoken" in the hf extras of pyproject.toml is unpinned; update the hf extras to include a minimum version constraint for tiktoken (for example "tiktoken>=X.Y.Z" or a range like "tiktoken>=X.Y.Z,<NextMajor") so your code is protected from incompatible older releases; modify the "tiktoken" entry in the hf extras list to the chosen constraint string and run dependency resolution to verify compatibility with existing constraints like transformers and peft.tests/examples/speculative_decoding/test_eagle.py (2)
284-284: Sametrust_remote_code=Truepattern - consider documenting.Same observation as line 234. An inline comment would clarify why this is needed for the Kimi checkpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/examples/speculative_decoding/test_eagle.py` at line 284, The call to AutoConfig.from_pretrained(checkpoint_dir, trust_remote_code=True) needs an inline comment explaining why trust_remote_code=True is required for the Kimi checkpoint; update the invocation site (the AutoConfig.from_pretrained call where checkpoint_dir is used) to add a short comment like “// required for Kimi checkpoint because model code is provided in the checkpoint and must be trusted” (or similar) so readers understand the reason.
234-236: Hardcodedtrust_remote_code=True- acceptable for test code but consider documenting.Per coding guidelines,
trust_remote_code=Trueshould not be hardcoded. However, this is test code (excluded from Bandit checks) testing specific remote models (Kimi, MiniMax) that require remote code execution.Consider adding an inline comment explaining why this is necessary:
Suggested documentation
+ # trust_remote_code=True required for moonshotai/MiniMaxAI models that use custom modeling code cfg = AutoConfig.from_pretrained(model_path, trust_remote_code=True)As per coding guidelines: "Do not hardcode
trust_remote_code=Truewhen loading Hugging Face Transformers models."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/examples/speculative_decoding/test_eagle.py` around lines 234 - 236, The test currently hardcodes trust_remote_code=True when calling AutoConfig.from_pretrained(model_path, trust_remote_code=True) (assigning to cfg) which violates the general guideline; update the test to keep trust_remote_code=True but add a clear inline comment adjacent to the AutoConfig.from_pretrained call explaining that this is test-only, that the tests exercise remote-models (e.g., Kimi, MiniMax) which require remote code execution, and that this file is excluded from Bandit checks—so leave the flag as-is for these specific models and do not change runtime behavior elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelopt/torch/speculative/plugins/transformers.py`:
- Around line 750-753: The code assumes self._base_llm_config has attribute
dtype but standard HF configs use torch_dtype; update the dtype selection logic
used before computing dtypemin so it first checks getattr(self._base_llm_config,
"dtype", None) then getattr(self._base_llm_config, "torch_dtype", None) and only
then falls back to self.eagle_module.layers[0].input_layernorm.weight.dtype;
ensure the variable named dtype is set from that prioritized lookup so
torch.finfo(dtype).min remains valid for dtypemin computation.
In `@pyproject.toml`:
- Line 85: Remove the "tiktoken" dependency entry from pyproject.toml (the
current string "tiktoken") and ensure the dependency remains declared only in
the example-specific requirements files (e.g.,
examples/specdec_bench/requirements.txt and examples/llm_eval/requirements.txt);
update those requirements files if missing and then regenerate any
lock/installed environment artifacts as needed (e.g., update poetry lock or CI
deps) so core package modelopt has no direct tiktoken dependency.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 85: The dependency entry "tiktoken" in the hf extras of pyproject.toml is
unpinned; update the hf extras to include a minimum version constraint for
tiktoken (for example "tiktoken>=X.Y.Z" or a range like
"tiktoken>=X.Y.Z,<NextMajor") so your code is protected from incompatible older
releases; modify the "tiktoken" entry in the hf extras list to the chosen
constraint string and run dependency resolution to verify compatibility with
existing constraints like transformers and peft.
In `@tests/examples/speculative_decoding/test_eagle.py`:
- Line 284: The call to AutoConfig.from_pretrained(checkpoint_dir,
trust_remote_code=True) needs an inline comment explaining why
trust_remote_code=True is required for the Kimi checkpoint; update the
invocation site (the AutoConfig.from_pretrained call where checkpoint_dir is
used) to add a short comment like “// required for Kimi checkpoint because model
code is provided in the checkpoint and must be trusted” (or similar) so readers
understand the reason.
- Around line 234-236: The test currently hardcodes trust_remote_code=True when
calling AutoConfig.from_pretrained(model_path, trust_remote_code=True)
(assigning to cfg) which violates the general guideline; update the test to keep
trust_remote_code=True but add a clear inline comment adjacent to the
AutoConfig.from_pretrained call explaining that this is test-only, that the
tests exercise remote-models (e.g., Kimi, MiniMax) which require remote code
execution, and that this file is excluded from Bandit checks—so leave the flag
as-is for these specific models and do not change runtime behavior elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e1939be-54e5-460a-a4cd-e7b373cc56ad
📒 Files selected for processing (11)
.github/workflows/_example_tests_runner.ymlexamples/llm_eval/requirements.txtexamples/llm_ptq/requirements.txtexamples/speculative_decoding/requirements.txtmodelopt/torch/speculative/plugins/transformers.pypyproject.tomltests/_test_utils/examples/llm_ptq_utils.pytests/examples/llm_ptq/test_llm_ptq.pytests/examples/speculative_decoding/conftest.pytests/examples/speculative_decoding/test_eagle.pytests/gpu/torch/export/test_unified_hf_export_and_check_safetensors.py
💤 Files with no reviewable changes (3)
- examples/llm_ptq/requirements.txt
- examples/llm_eval/requirements.txt
- .github/workflows/_example_tests_runner.yml
h-guo18
reviewed
Apr 6, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1183 +/- ##
==========================================
+ Coverage 74.77% 76.25% +1.47%
==========================================
Files 351 351
Lines 40072 41891 +1819
==========================================
+ Hits 29964 31943 +1979
+ Misses 10108 9948 -160
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
h-guo18
reviewed
Apr 6, 2026
Co-authored-by: h-guo18 <67671475+h-guo18@users.noreply.github.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
kevalmorabia97
force-pushed
the
kmorabia/spec-dec-tests
branch
from
361b4a4 to
0c7f0ed
Compare
h-guo18
approved these changes
Apr 7, 2026
sugunav14
approved these changes
Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Type of change: Test fix
tests/examples/speculative_decoding- previously silently skippedTesting
Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: N/ASummary by CodeRabbit
Chores
Tests
Bug Fixes