Skip to content

[https://nvbugs/6337231][fix] Unskip and fix iter-stats unit tests' fake-self predicate#15927

Merged
yuxianq merged 1 commit into
NVIDIA:mainfrom
YihuiLu512:dev-fix-08-J-N6337231
Jul 6, 2026
Merged

[https://nvbugs/6337231][fix] Unskip and fix iter-stats unit tests' fake-self predicate#15927
yuxianq merged 1 commit into
NVIDIA:mainfrom
YihuiLu512:dev-fix-08-J-N6337231

Conversation

@YihuiLu512

@YihuiLu512 YihuiLu512 commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

The dummy-request predicate was promoted to a staticmethod by #14922 but the
tests' MagicMock fake self resolves _is_stats_dummy_request to an
auto-generated (always-truthy) child Mock, so every request was filtered as
dummy and all per-request aggregates asserted 0. Bind the real predicate onto
the fake self and remove the skip markers added by #15368.

Also fixes https://nvbugs/6337233 (same root cause).

Summary by CodeRabbit

  • Bug Fixes

    • Improved coverage for request statistics handling so aggregate counters are validated more reliably.
    • Fixed test setup to better reflect real request classification, helping prevent false zero counts in stats-related scenarios.
  • Tests

    • Removed unnecessary skips from several stats-related test cases, increasing test coverage.

Description

Test Coverage

PR Checklist

Please review the following before submitting your PR:

  • PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

  • PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

  • Test cases are provided for new code paths (see test instructions)

  • If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title.

  • Any new dependencies have been scanned for license and vulnerabilities

  • CODEOWNERS updated if ownership changes

  • Documentation updated as needed

  • Update tava architecture diagram if there is a significant design change in PR.

  • The reviewers assigned automatically/manually are appropriate for the PR.

  • Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

…ake-self predicate

The dummy-request predicate was promoted to a staticmethod by NVIDIA#14922 but the
tests' MagicMock fake self resolves `_is_stats_dummy_request` to an
auto-generated (always-truthy) child Mock, so every request was filtered as
dummy and all per-request aggregates asserted 0. Bind the real predicate onto
the fake self and remove the skip markers added by NVIDIA#15368.

Also fixes https://nvbugs/6337233 (same root cause).

Signed-off-by: Yihui Lu <269394165+YihuiLu512@users.noreply.github.com>
@YihuiLu512

Copy link
Copy Markdown
Collaborator Author

/bot run

@YihuiLu512 YihuiLu512 requested a review from yuxianq July 3, 2026 14:19
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: efff4d59-6992-474f-b15c-90f52989c5ce

📥 Commits

Reviewing files that changed from the base of the PR and between edf63e8 and dfc1456.

📒 Files selected for processing (1)
  • tests/unittest/_torch/executor/test_iter_stats_populate.py

📝 Walkthrough

Walkthrough

Removed pytest skip decorators and constants from several stat-aggregation tests, and fixed the _build_fake_self test helper to bind the real PyExecutor._is_stats_dummy_request method instead of an unbound MagicMock, preventing incorrect dummy-request classification in tests.

Changes

Test fix for dummy request stats

Layer / File(s) Summary
Bind real dummy-request predicate and remove skip decorators
tests/unittest/_torch/executor/test_iter_stats_populate.py
Removes the pytest import, NVBUG skip constants, and their decorator usages; updates _build_fake_self to import PyExecutor and assign the real _is_stats_dummy_request method to the fake executor object.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the PR's main fix and follows the required ticket-and-type format.
Description check ✅ Passed The description explains the bug and fix clearly, though the Test Coverage section is not filled in.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@YihuiLu512 YihuiLu512 removed the request for review from yuxianq July 3, 2026 14:23
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #57476 [ run ] triggered by Bot. Commit: dfc1456 Link to invocation

@YihuiLu512 YihuiLu512 requested a review from QiJune July 3, 2026 14:26
@YihuiLu512

Copy link
Copy Markdown
Collaborator Author

@tedzhouhk
Please check whether this revision aligns with the original PR's intent (#14922).

@YihuiLu512 YihuiLu512 enabled auto-merge (squash) July 3, 2026 14:31
@YihuiLu512 YihuiLu512 disabled auto-merge July 3, 2026 14:32
@tedzhouhk

Copy link
Copy Markdown
Contributor

@tedzhouhk Please check whether this revision aligns with the original PR's intent (#14922).

lgtm, thx for fixing (sorry I can't approve due to permission reason

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #57476 [ run ] completed with state SUCCESS. Commit: dfc1456
/LLM/main/L0_MergeRequest_PR pipeline #46211 completed with status: 'SUCCESS'

CI Report

Link to invocation

@YihuiLu512

Copy link
Copy Markdown
Collaborator Author

PR_Github #57476 [ run ] completed with state SUCCESS. Commit: dfc1456 /LLM/main/L0_MergeRequest_PR pipeline #46211 completed with status: 'SUCCESS'

CI Report

Link to invocation

All 9 previously skipped test cases are now covered again and have passed.

@yuxianq yuxianq merged commit 96b73dd into NVIDIA:main Jul 6, 2026
12 checks passed
BrianLi23 pushed a commit to BrianLi23/TensorRT-LLM that referenced this pull request Jul 9, 2026
…ake-self predicate (NVIDIA#15927)

Signed-off-by: Yihui Lu <269394165+YihuiLu512@users.noreply.github.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.

4 participants