Open
Conversation
Signed-off-by: Benjamin Chislett <bchislett@nvidia.com>
Signed-off-by: Benjamin Chislett <bchislett@nvidia.com>
Signed-off-by: Benjamin Chislett <bchislett@nvidia.com>
Signed-off-by: Benjamin Chislett <bchislett@nvidia.com>
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds HumanEval dataset support, a --parallel_drafting CLI flag wired into model/speculative-decoding, and trims turn data in acceptance rate computation before per-turn calculations. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (run.py)
participant Dataset as HumanEval dataset loader
participant Model as Model constructor (vllm)
participant SpecDec as SpeculativeDecoding config
CLI->>Dataset: select dataset "humaneval" and load samples
Dataset-->>CLI: list of Request objects
CLI->>Model: instantiate with parallel_drafting flag
Model->>SpecDec: if specdec present and parallel_drafting true
SpecDec-->>Model: specdec config includes parallel_drafting
Model-->>CLI: model ready (with speculative decoding config)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![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: 1
🧹 Nitpick comments (1)
examples/specdec_bench/specdec_bench/datasets/humaneval.py (1)
24-28: Minor: Inaccurate inline comment.The comment
# list of list of questions.is misleading—self.datais actually alist[Request], not a nested list. Consider updating or removing the comment.📝 Suggested fix
class HumanEval(Dataset): def __init__(self, path, num_samples=164, **kwargs): - self.data: list[Request] = [] # list of list of questions. + self.data: list[Request] = [] self.num_samples = num_samples self._preprocess(path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/specdec_bench/specdec_bench/datasets/humaneval.py` around lines 24 - 28, Update the inaccurate inline comment on the attribute in class HumanEval: the field self.data is declared as a list[Request] (not a nested list), so change or remove the comment "// list of list of questions." to accurately reflect that self.data is a flat list of Request objects; locate the declaration in the __init__ of HumanEval and adjust the comment to something like "list of Request" or remove it entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/specdec_bench/specdec_bench/models/vllm.py`:
- Around line 68-70: The code currently sets specdec["parallel_drafting"] = True
when kwargs.get("parallel_drafting") is truthy but doesn't handle the case where
specdec is None; update the block around kwargs.get("parallel_drafting") to
first ensure specdec is a dict (e.g., if specdec is None, assign an empty dict)
or only perform the item assignment when specdec is not None, so that the
assignment to specdec["parallel_drafting"] cannot raise a TypeError; reference
the specdec variable and the kwargs.get("parallel_drafting") check to locate
where to add the guard/initialization.
---
Nitpick comments:
In `@examples/specdec_bench/specdec_bench/datasets/humaneval.py`:
- Around line 24-28: Update the inaccurate inline comment on the attribute in
class HumanEval: the field self.data is declared as a list[Request] (not a
nested list), so change or remove the comment "// list of list of questions." to
accurately reflect that self.data is a flat list of Request objects; locate the
declaration in the __init__ of HumanEval and adjust the comment to something
like "list of Request" or remove it entirely.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/specdec_bench/run.pyexamples/specdec_bench/specdec_bench/datasets/__init__.pyexamples/specdec_bench/specdec_bench/datasets/humaneval.pyexamples/specdec_bench/specdec_bench/metrics/acceptance_rate.pyexamples/specdec_bench/specdec_bench/models/vllm.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #939 +/- ##
==========================================
- Coverage 70.10% 70.09% -0.02%
==========================================
Files 221 221
Lines 25541 25541
==========================================
- Hits 17905 17902 -3
- Misses 7636 7639 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
yeyu-nvidia
approved these changes
Feb 26, 2026
Signed-off-by: Benjamin Chislett <bchislett@nvidia.com>
| if len(turn) > 1 and turn[0] <= 1: | ||
| turn = turn[1:] # Skip prefill if it is 1 or less, indicating no specdec | ||
| if len(turn) > 1: | ||
| turn = turn[:-1] # Skip final acceptance due to EOS truncating speculation |
Contributor
There was a problem hiding this comment.
Only skip if EOS is present?
Contributor
Author
There was a problem hiding this comment.
I think it's reasonable to skip anyways, since truncation for any reason (EOS, length, stop token) might misrepresent the AR since it is not aligned with the draft size per step
Signed-off-by: Benjamin Chislett <bchislett@nvidia.com>
…Optimizer into bchislett/specbench-tweaks
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
3 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?
Example usage:
Summary by CodeRabbit
New Features
Bug Fixes