[None][feat] Log running metric estimates during long lm-eval runs - #16752
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPartial scoring is added to lm-eval generation. Environment variables configure scoring intervals and non-streaming in-flight windows, wrappers track completed responses, and multimodal wrappers retain interface parity without applying these features. ChangesPartial scoring and generation control
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LmEvalEvaluator
participant LmEvalWrapper
participant ThreadPool
participant _RunningScoreTracker
participant lm_eval_task
LmEvalEvaluator->>LmEvalWrapper: pass scoring interval and task dictionary
LmEvalWrapper->>ThreadPool: submit bounded generation window
ThreadPool-->>LmEvalWrapper: return completed response
LmEvalWrapper->>_RunningScoreTracker: update completed response
_RunningScoreTracker->>lm_eval_task: apply filters and process results
lm_eval_task-->>_RunningScoreTracker: return numeric metrics
_RunningScoreTracker-->>LmEvalWrapper: log running aggregates
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/unittest/others/test_lm_eval.py (1)
772-855: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for configuration and wrapper integration.
Coverage summary: the six added tests cover tracker aggregation, isolation, failure disabling, logging cadence, response-list wrapping, and nested groups. They do not verify environment parsing (
positive,0, invalid values) or thatLmEvalWrapper.generate_untilinvokes the tracker for completed outputs. Test-list status: notests/integration/test_lists/file is in scope for these unit tests. Verdict: insufficient.As per path instructions,
tests/**changes require changed-test, test-list, and coverage-verdict 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/unittest/others/test_lm_eval.py` around lines 772 - 855, Add tests covering configuration parsing for positive values, zero, and invalid values, plus wrapper integration verifying LmEvalWrapper.generate_until invokes the running score tracker for completed outputs. Extend the existing tracker test coverage without changing unrelated behavior, and include the required changed-test, test-list, and coverage-verdict review updates for tests/** changes.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 `@tensorrt_llm/evaluate/lm_eval.py`:
- Around line 115-117: Update the metric aggregation key in the evaluation loop
to include instance.task_name alongside the metric and ensemble name, keeping
sums and counts task-specific. Add a regression test covering two tasks that use
the same filter and verify their running estimates remain separate.
- Around line 77-92: The partial-scoring API lacks the requested type
annotations. Update the visible initializer to return None, annotate update()’s
instance parameter with lm_eval.api.instance.Instance or an equivalent protocol
exposing task_name, doc, resps, and filtered_resps, and annotate
partial_scores_every as int | None wherever it is declared in the related
partial-scoring API.
---
Nitpick comments:
In `@tests/unittest/others/test_lm_eval.py`:
- Around line 772-855: Add tests covering configuration parsing for positive
values, zero, and invalid values, plus wrapper integration verifying
LmEvalWrapper.generate_until invokes the running score tracker for completed
outputs. Extend the existing tracker test coverage without changing unrelated
behavior, and include the required changed-test, test-list, and coverage-verdict
review updates for tests/** changes.
🪄 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: Enterprise
Run ID: ebf238ba-7d93-4149-813f-142a953a85e7
📒 Files selected for processing (2)
tensorrt_llm/evaluate/lm_eval.pytests/unittest/others/test_lm_eval.py
|
/bot run |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/others/test_lm_eval.py (1)
951-951: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the scorer update payload.
This passes if
generate_untilcallsupdatewith the wrong request or response text, leaving the scoring contract untested.Proposed test strengthening
- mock_update.assert_called_once() + mock_update.assert_called_once_with(fake_request, "42")As per path instructions, test-code changes require actionable TensorRT-LLM 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/unittest/others/test_lm_eval.py` at line 951, Strengthen the test around generate_until by asserting the arguments passed to the scorer update call, not just that mock_update was called once. Use the expected request and generated response text from the test scenario, and preserve the existing call-count assertion so the scoring contract is verified for both payload values.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.
Nitpick comments:
In `@tests/unittest/others/test_lm_eval.py`:
- Line 951: Strengthen the test around generate_until by asserting the arguments
passed to the scorer update call, not just that mock_update was called once. Use
the expected request and generated response text from the test scenario, and
preserve the existing call-count assertion so the scoring contract is verified
for both payload values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d160a6ee-a966-48e7-ae70-aee0ec9b41d9
📒 Files selected for processing (2)
tensorrt_llm/evaluate/lm_eval.pytests/unittest/others/test_lm_eval.py
👎 Promotion blocked, new vulnerability foundVulnerability report
|
|
/bot run |
|
PR_Github #61091 [ run ] triggered by Bot. Commit: |
|
PR_Github #61091 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61585 [ run ] triggered by Bot. Commit: |
|
PR_Github #61585 [ run ] completed with state
|
00bb7c1 to
d92dca5
Compare
|
/bot run |
|
PR_Github #61612 [ run ] triggered by Bot. Commit: |
|
PR_Github #61612 [ run ] completed with state
|
0ab8788 to
2b69293
Compare
|
/bot run |
|
PR_Github #61622 [ run ] triggered by Bot. Commit: |
|
PR_Github #61622 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61639 [ run ] triggered by Bot. Commit: |
|
PR_Github #61639 [ run ] completed with state
|
2b69293 to
0e83afa
Compare
|
/bot run |
|
PR_Github #61976 [ run ] triggered by Bot. Commit: |
- Include task_name in metric aggregation key so multi-task evals keep per-task running estimates separate - Add type annotations to _RunningScoreTracker (__init__ -> None, instance: Any in update) - Extract env-var parsing into _parse_partial_scores_env() to make it unit-testable - Add tests: task-key separation, env-var parsing (positive/zero/negative/ invalid/unset), and generate_until tracker invocation Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
…rtial-score signal Add TLLM_EVAL_MAX_IN_FLIGHT (default 0/unset = disabled, preserving existing submit-all behavior). When set to W>0, LmEvalWrapper.generate_until submits at most W requests concurrently; as each completes (via per-request waiter threads + FIRST_COMPLETED), it is scored through the existing _RunningScoreTracker and the window is topped up with the next unsubmitted request. Outputs are collected into an index-addressed list so the returned order still matches submission order — downstream scoring is unchanged. Why: on deployments using GUARANTEED_NO_EVICT scheduling the engine admits ~all requests concurrently, so every response completes in a burst at the very end and TLLM_EVAL_PARTIAL_SCORES_EVERY prints nothing until the final minute. Windowing makes responses complete steadily throughout the run, providing early failure signal. TRADEOFF: windowing can reduce end-to-end throughput — waves of W requests may under-fill the scheduler versus all-at-once admission. The payoff is EARLY FAILURE SIGNAL via steady partial scores. Suggested W: 256-512. Gated to the standard non-streaming path only; streaming and the multimodal generate_until override keep the submit-all behavior. A failed request re-raises without deadlocking the window (pool shutdown drains remaining waiters). Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
Assert exact (request, text) payload passed to _RunningScoreTracker.update instead of just checking call count. Addresses CodeRabbit review. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
Port TLLM_EVAL_SPEC_STATS from the Kimi K3 branch: when set to 1, generate_until logs a corpus-aggregate acceptance length (AL, mean of per-request avg_decoded_tokens_per_iter) after the run, for both the text and multimodal wrappers. Silent no-op on non-speculative runs. Unlike the original branch version, acceptance rate (AR) is intentionally not reported and return_perf_metrics is not forced: the request_perf_metrics.speculative_decoding counters AR needs are only maintained by TRTLLMSampler, not the TorchSampler used by one-engine spec-dec, so AR would silently read 0 on the default PyTorch path. AR reporting can return together with a fix populating those counters. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
- Add tests/unittest/others/test_lm_eval.py to l0_a10.yml so the suite actually runs in CI (test-db wires files individually; unlisted files are never collected). - Aggregate the TLLM_EVAL_SPEC_STATS acceptance length as an iteration-weighted mean (total decoded tokens / total decode iterations) using per-request decoding_iter, matching the canonical definition in bench/dataclasses/reporting.py; min/max stay per-request; missing decoding_iter falls back to weight 1. - Harden the TLLM_EVAL_MAX_IN_FLIGHT windowed path: bound waiter threads to min(W, len(requests)); on a failed request, escape via shutdown(wait=False, cancel_futures=True) instead of blocking on every other in-flight waiter (a hung request could previously trap the exception and hang the eval). Tests cover the raise path, the in-flight cap, and submission-order results under out-of-order completion. - Wire the partial scorer into the multimodal generate_until override (scoring the post-processed text lm-eval itself scores) instead of accepting and silently ignoring the arguments. - Document the shallow-copy / live-task caveat on _RunningScoreTracker. - Add end-to-end tests driving lm-eval's real evaluate() loop (real ConfigurableTask via custom_dataset, real filters and aggregation) over a mocked LLM: final score, partial-estimate-vs-final agreement, and windowed-path score parity. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
The three multi-image content_parts tests constructed the wrapper with model_type="gemma3", which never opts into interleaved placeholders (MULTIMODAL_PLACEHOLDER_REGISTRY.get_interleave_placeholders returns False for it), so apply_chat_template never built content_parts and the tests failed with KeyError — silently, since this file was not collected by any CI list. Now that l0_a10.yml collects the file, they must be green: drive the registry flag explicitly (interleave=True/False) so the tests are independent of which models the installed transformers version registers, and rename the two negative-path tests to describe what they actually assert. Full file passes 87/87. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
|
/bot run |
0f5efa8 to
b9fd5a9
Compare
|
PR_Github #62822 [ run ] triggered by Bot. Commit: |
|
PR_Github #62822 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62837 [ run ] triggered by Bot. Commit: |
|
PR_Github #62837 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62870 [ run ] triggered by Bot. Commit: |
b9fd5a9 to
2d4c391
Compare
|
/bot run |
|
PR_Github #62881 [ run ] triggered by Bot. Commit: |
|
PR_Github #62870 [ run ] completed with state |
|
PR_Github #62881 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62896 [ run ] triggered by Bot. Commit: |
|
PR_Github #62896 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62951 [ run ] triggered by Bot. Commit: |
|
PR_Github #62951 [ run ] completed with state |
4 participants
Summary
Adds three opt-in env-var features to
tensorrt_llm/evaluate/lm_eval.pyforobservability during long lm-eval runs:
TLLM_EVAL_PARTIAL_SCORES_EVERY=<N>— rolling metric estimates_RunningScoreTracker: scores each completedgenerate_untilresponse on a throwaway instance copy (using the owning task's filter
ensembles and
process_results) and logs a running aggregate every Nresponses (e.g.
exact_match ~ 72.50 after 500/5000 responses).tracker without affecting the eval.
task_nameso multi-task evals keep per-taskestimates separate.
_parse_partial_scores_env()fortestability.
TLLM_EVAL_MAX_IN_FLIGHT=<W>— submission windowinggenerate_untilsubmits at most W requests concurrently;as each completes (via per-request waiter threads +
FIRST_COMPLETED), itis scored through
_RunningScoreTrackerand the window is topped up withthe next unsubmitted request. Outputs are collected into an index-addressed
list so the returned order still matches submission order — downstream
scoring is unchanged.
generate_untiloverride keep the submit-all behavior.drains remaining waiters).
requests may under-fill the scheduler vs. all-at-once admission. The payoff
is early failure signal via steady partial scores throughout the run instead
of a burst at the very end. Suggested W: 256–512.
TLLM_EVAL_SPEC_STATS=1— speculative-decoding acceptance-length summarygenerate_untilcompletes (both the text and multimodal wrappers),logs a corpus-aggregate acceptance length: AL as the mean of per-request
avg_decoded_tokens_per_iter(target token + accepted draft tokens perdecode step), with min/max/n.
excluded rather than counted as zero. Adds no per-request overhead
(
avg_decoded_tokens_per_iteris always populated; perf metrics are notrequested).
request_perf_metrics.speculative_decodingcounters it needs are onlymaintained by
TRTLLMSampler(update_num_tokens_per_iteration), not bythe
TorchSamplerused by one-engine spec-dec, so AR would silently read0 on the default PyTorch path. AR reporting should return in a follow-up
together with a fix populating those counters.
Motivation
Multi-hour lm-eval runs currently give zero quality signal until the very
end.
TLLM_EVAL_PARTIAL_SCORES_EVERYlets operators see rolling estimatesand kill a bad run early.
TLLM_EVAL_MAX_IN_FLIGHTensures those partialscores arrive steadily throughout the run rather than all at once at the end
(which can happen when the engine admits all requests concurrently under
GUARANTEED_NO_EVICTscheduling).TLLM_EVAL_SPEC_STATSgives accuracyruns on speculative-decoding configs a free acceptance-length readout, so a
misbehaving drafter is visible from the eval log without a separate perf run.
Test plan
pytest tests/unittest/others/test_lm_eval.py -k "running_score"— all unit tests passpytest tests/unittest/others/test_lm_eval.py -k "spec_stats or log_spec"— env gating (unset/1/other values), AL mean/min/max/n aggregation, exclusion of requests without metrics, silence on non-spec runs, andgenerate_untilinvoking/skipping_log_spec_statsTLLM_EVAL_PARTIAL_SCORES_EVERY=100and confirm partial-score log lines appear every 100 responses; confirm final score is unchanged vs a run without the env var setTLLM_EVAL_MAX_IN_FLIGHT=256and confirm steady partial-score output throughout the runDev Engineer Review
generate_untilruns viaTLLM_EVAL_PARTIAL_SCORES_EVERY(unset/≤0 disables)._RunningScoreTrackerthat shallow-copies completed in-flight lm-eval instances for isolated scoring, injects a probe response, applies the owning task’s_filters/ensembles, runsprocess_resultson the probe, aggregates numeric metric estimates, logs rolling aggregates everyNcompleted responses and once at completion, and permanently disables itself on any scoring exception (evaluation continues).LmEvalWrapper.__init__(..., partial_scores_every: Optional[int], partial_scoring_task_dict: Optional[dict])MultimodalLmEvalWrapper.__init__(..., partial_scores_every: Optional[int], partial_scoring_task_dict: Optional[dict])LmEvalEvaluator.evaluateparsesTLLM_EVAL_PARTIAL_SCORES_EVERYand wirespartial_scores_every/partial_scoring_task_dict=self.task_dictinto wrapperlm_kwargsonly when enabled.TLLM_EVAL_MAX_IN_FLIGHT: whenmax_in_flight > 0, it completes responses in a sliding window (thread-pool based) to allow mid-run partial-score updates while preserving output order; otherwise it falls back to submit-all-then-fetch. Streaming and multimodal paths do not apply windowing.process_resultscalled with the “results is a list” convention, task-group flattening, and per-task accumulator separation) and env-var parsing; one test also assertsgenerate_untilinvokes the partial scorer.TLLM_EVAL_MAX_IN_FLIGHT.QA Engineer Review
tests/unittest/others/test_lm_eval.py:test_running_score_tracker_aggregates_meantest_running_score_tracker_does_not_mutate_instancetest_running_score_tracker_unknown_task_disablestest_running_score_tracker_logs_on_intervaltest_running_score_tracker_process_results_list_conventiontest_running_score_tracker_task_groups_flattenedtest_running_score_tracker_separate_keys_per_tasktest_parse_partial_scores_env_positivetest_parse_partial_scores_env_zero_disablestest_parse_partial_scores_env_negative_disablestest_parse_partial_scores_env_invalid_raisestest_parse_partial_scores_env_unset_returns_nonetest_generate_until_invokes_partial_scorertests/integration/test_lists/: no references to these new unit tests found, so CI/manual list coverage is unclear.