[TRTLLM-14719][infra] Add spec-dec acceptance-length regression baselines and remove PARD CnnDailymail coverage - #17474
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe changes add YAML-backed acceptance-length baselines, compute acceptance length from iteration statistics, and validate speculative-decoding accuracy tests across multiple draft-model and MTP configurations. ChangesAcceptance Length Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AccuracyTest
participant IterationPerformanceStats
participant AcceptanceLengthCalculator
participant BaselineValidator
participant AcceptanceLengthYAML
AccuracyTest->>IterationPerformanceStats: enable and retrieve iteration statistics
AccuracyTest->>AcceptanceLengthCalculator: compute average acceptance length
AcceptanceLengthCalculator-->>AccuracyTest: acceptance length
AccuracyTest->>BaselineValidator: validate observed value
BaselineValidator->>AcceptanceLengthYAML: load or persist baseline
BaselineValidator-->>AccuracyTest: pass or assertion failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/integration/defs/accuracy/accuracy_core.py (1)
147-179: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd focused coverage for the environment-controlled paths.
Add non-GPU tests for skip mode, population mode, a missing key, a null
min_al, and a regression. The supplied integration callers exercise the normal threshold path only. Population is a stated PR feature and currently has no focused coverage.As per path instructions, integration-test changes require a test coverage review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/defs/accuracy/accuracy_core.py` around lines 147 - 179, Add focused non-GPU tests covering the acceptance-length helper’s environment-controlled branches: skip mode, population mode, missing baseline key, null min_al, and the normal regression/threshold path. Exercise the existing accuracy helper and its environment variables, isolate filesystem/YAML writes for population mode, and verify each path’s expected return or exception without requiring a GPU.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/defs/accuracy/references/acceptance_length.yaml`:
- Line 1: Add the standard NVIDIA copyright header at the beginning of the
acceptance_length.yaml content, before the YAML mapping, using 2026 as the
latest meaningful modification year.
In `@tests/integration/defs/accuracy/test_llm_api_pytorch.py`:
- Around line 100-113: Update _compute_acceptance_length in
tests/integration/defs/accuracy/test_llm_api_pytorch.py to annotate llm as LLM.
In tests/integration/defs/accuracy/accuracy_core.py, define a typed
AcceptanceLengthBaseline entry and annotate baselines as dict[str,
AcceptanceLengthBaseline], keeping the outer dictionary mutable for
populate-mode updates. No test changes are needed.
---
Nitpick comments:
In `@tests/integration/defs/accuracy/accuracy_core.py`:
- Around line 147-179: Add focused non-GPU tests covering the acceptance-length
helper’s environment-controlled branches: skip mode, population mode, missing
baseline key, null min_al, and the normal regression/threshold path. Exercise
the existing accuracy helper and its environment variables, isolate
filesystem/YAML writes for population mode, and verify each path’s expected
return or exception without requiring a GPU.
🪄 Autofix
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: Enterprise
Run ID: 5f1ddb10-0eef-40a5-beb6-ba6720c29345
📒 Files selected for processing (3)
tests/integration/defs/accuracy/accuracy_core.pytests/integration/defs/accuracy/references/acceptance_length.yamltests/integration/defs/accuracy/test_llm_api_pytorch.py
mikeiovine
left a comment
mikeiovine
left a comment
There was a problem hiding this comment.
Looks great, thanks @allisonlim-nv.
I am slightly worried that ALs will fluctuate a bit due to kernel differences on different hardware - I think we discussed it before, where did we land on that? Are the differences too small to matter?
d170c78 to
25b5030
Compare
Signed-off-by: Allison Lim <allim@nvidia.com>
Signed-off-by: Allison Lim <allim@nvidia.com>
Signed-off-by: Allison Lim <allim@nvidia.com>
Signed-off-by: Allison Lim <allim@nvidia.com>
Fix docstring formatting in _compute_acceptance_length function. Signed-off-by: allisonlim-nv <allim@nvidia.com> Signed-off-by: Allison Lim <allim@nvidia.com>
Reformatted docstring for clarity and structure. Signed-off-by: allisonlim-nv <allim@nvidia.com> Signed-off-by: Allison Lim <allim@nvidia.com>
Signed-off-by: Allison Lim <allim@nvidia.com>
25b5030 to
0ceaa97
Compare
|
The current references were collected on B200s. I’ll run the one-GPU baselines on Hopper (H100/H200) and compare the AL deltas to validate whether the 5% tolerance is sufficient. |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Solid idea — AL regressions currently pass CI silently, and this closes that. Two things to sort out before it lands.
The baselines may be measuring only the tail of each run. PyExecutor keeps iteration stats in a buffer capped by max_stats_len (default 1000, tensorrt_llm/llmapi/llm_args.py:5483); older entries are dropped, and nothing drains the buffer during task.evaluate() — get_stats() is pull-only. For test_ngram (max_bs=16, 1319 GSM8K requests × up to 256 output tokens) the run is far more than 1000 iterations, so the recorded AL covers only the final ~1000 — i.e. the last handful of requests at low concurrency. That both explains why the ngram/suffix baselines look low relative to max_draft_len=4, and makes the number sensitive to how many iterations a run happens to take, which is exactly what a kernel change alters. Please pass max_stats_len=-1 on the AL tests (or otherwise cover the whole run) and re-collect the baselines.
Baseline provenance. Each min_al is one run × 0.95 with no measured run-to-run spread. Since batch composition varies with scheduling, please note in the PR (or a comment in the YAML) how many runs backed each number, so the next person hitting a failure can tell a regression from noise.
Description vs diff: CnnDailymail is also removed from TestLlama3_1_8BInstruct::test_eagle3, not just test_pard — the title/description only mentions PARD. Either state it or restore it; dropping a summarization eval from EAGLE3 coverage is a separate decision from the AL work.
TRTLLM_POPULATE_ACCEPTANCE_LENGTH is a new developer-facing knob with no documentation outside the helper docstring. A short "how to regenerate baselines" note next to references/acceptance_length.yaml would save the next person a code read.
| and stat['specDecodingStats']['numDraftTokens'] > 0 | ||
| ] | ||
| assert spec_iters, "No iterations with speculative decoding stats" | ||
| return sum(stat['acceptanceLength'] |
There was a problem hiding this comment.
acceptanceLength is already a per-request average within one iteration (py_executor.py:2100: (accepted + requests_with_draft) / requests_with_draft). Averaging it over iterations gives a 1-request tail iteration the same weight as a 200-request steady-state one, which adds variance the 5% margin has to absorb.
The request-weighted aggregate uses fields already in the dict:
accepted = sum(s['numAcceptedTokens'] for s in spec_iters)
reqs = sum(s['numRequestsWithDraftTokens'] for s in spec_iters)
return (accepted + reqs) / reqs| entry["ref_al"] = al_value | ||
| entry["min_al"] = al_value * 0.95 | ||
|
|
||
| with open(yaml_path, "w", encoding="utf-8") as f: |
There was a problem hiding this comment.
Rewriting the checked-in YAML from inside a test is racy: under pytest -n (or several AL tests in one session) two processes read the same snapshot and the later write drops the other's entry. yaml.safe_dump also erases any comments and normalizes the file (the ? -style key on line 3 of the YAML comes from this).
Safer: in populate mode write a per-test file under a temp/output dir (e.g. $TRTLLM_AL_OUT/<sanitized_key>.yaml) and merge them into the reference YAML with a small script, so the CI run never mutates the source tree.
|
|
||
|
|
||
| def assert_acceptance_length(test_key: str, al_value: float) -> None: | ||
| """Assert acceptance length meets the registered minimum. |
There was a problem hiding this comment.
The docstring doesn't mention TRTLLM_POPULATE_ACCEPTANCE_LENGTH, which is the flag that makes this function write files, and the documented Raises: KeyError is misleading in populate mode — populating still requires the key to pre-exist, so bootstrapping a new test needs a manual YAML stub first. Worth documenting both, plus the 0.95 factor's origin.
| @@ -0,0 +1,39 @@ | |||
| TestLlama3_1_8BInstruct::test_eagle3[eagle3_one_model_True-overlap_scheduler_True-sampler_async_worker_False]: | |||
There was a problem hiding this comment.
The keys duplicate pytest ids by hand, so any parametrize change silently breaks the mapping (KeyError at best, and for the value-gated tests like test_eagle3/test_bfloat16 the gate tuple can quietly stop matching, dropping the check with no failure). Deriving the key from request.node.nodeid.split("::", 1)[1] (or request.node.name plus the class name) keeps test and reference in sync automatically.
4 participants
Description
Tracks TRTLLM-14719 and nvbug/6037653.
Add acceptance-length (AL) regression coverage to end-to-end speculative-decoding accuracy tests. Tests collect speculative-decoding statistics and fail only when AL drops below 95% of its recorded reference, allowing expected variation from new kernels while detecting regressions.
Coverage includes all speculative-decoding algorithms in the suite. EAGLE3, MTP, and DFlash are covered across multiple model families, including autoregressive transformers, DeepSeek, GPT-OSS, and Nemotron. Remove the CnnDailymail evaluation from
test_pardand 'eagle3'. Use GSM8k as the single sataset for acceptance-length validation to keep regression signal consistent and avoid additional CI overhead.No API, dependency, or documentation changes.
Test Coverage
DEP4_MTP_ON, 4× B200).pre-commitchecks passed.Dev Engineer Review
test_pardandeagle3.QA Engineer Review
tests/integration/defs/accuracy/accuracy_core.pyandtests/integration/defs/accuracy/test_llm_api_pytorch.py.assert_acceptance_length._compute_acceptance_length.tests/integration/defs/accuracy/references/acceptance_length.yaml.tests/integration/test_lists/changes are reported. Test-list coverage for the modified tests requires verification.