[None][test] Fix the _validate_request fakes in test_forward_error_survival by moraxu · Pull Request #20883 · NVIDIA/TensorRT-LLM

moraxu · 2026-10-23T07:27:13Z

Dev Engineer Review

The test fakes now define kv_cache_retention_config=None and _is_kv_manager_v2=False. These values address missing attributes in the request-validation tests. The changes are limited to test setup.

QA Engineer Review

The modified unit-test file includes four test_validate_request_* cases. The author reports that all 21 tests passed with the change and that pre-commit passed. The tests cover request validation, including model-hook acceptance and rejection, text-only requests, and models without the hook. No matching entry was found in the searched integration test lists. Coverage verdict: sufficient.

Per-File QA Perspective

tests/unittest/executor/test_forward_error_survival.py: Covers executor forward-error survival and request-validation behavior. This unit-test file was not found in the searched test-db/ CI or qa/ manual-QA lists.

Description

The four test_validate_request_* cases in tests/unittest/executor/test_forward_error_survival.py, added in #19465, fail with:

AttributeError: 'types.SimpleNamespace' object has no attribute 'kv_cache_retention_config'

PyExecutor._validate_request reads request.kv_cache_retention_config, then self._is_kv_manager_v2, before it reaches the multimodal hook these cases exercise. The test's fake request is a SimpleNamespace without the first. Its executor shell is built with object.__new__, so __init__, which sets the second, never runs. The file is in l0_cpu.yml, so CPU-Generic-arm-1 fails in every pre-merge run whose tree includes #19465.

This gives the fake request kv_cache_retention_config=None and the shell _is_kv_manager_v2 = False, the value __init__ derives from kv_cache_manager=None. Only the test changes.

Test Coverage

tests/unittest/executor/test_forward_error_survival.py, run against the CI-built aarch64 wheel of [TRTLLM-17144][feat] Load the Muse Spark Open (muse_spark) config #20643. That wheel is based on 6cb5a79980, whose _validate_request and test file match current main. With the file as on main: 4 failed and 17 passed, the same four cases that fail in CI. With this change: 21 passed.
pre-commit passes on the changed file.

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.

…rvival The four test_validate_request_* cases call PyExecutor._validate_request with a SimpleNamespace request and an object.__new__ executor shell. _validate_request reads request.kv_cache_retention_config and self._is_kv_manager_v2 before it reaches the multimodal hook, and neither fake defined them, so all four cases raised AttributeError. Give the request kv_cache_retention_config=None and the shell _is_kv_manager_v2 = False, the value __init__ derives from kv_cache_manager=None. Signed-off-by: Michal Guzek <mguzek@nvidia.com>

coderabbitai · 2026-10-23T07:30:04Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Source mode: Auto
Repository: NVIDIA/TensorRT-LLM
Branch: main
File: .coderabbit.yaml
Commit: 12af626
Layers: Repository YAML
Review profile: CHILL
Plan: Enterprise
Run ID: 7fb55913-826b-4e52-8862-cdba1e0ab468

📥 Commits

Reviewing files that changed from the base of the PR and between 12af626 and bb88175.

📒 Files selected for processing: 1

tests/unittest/executor/test_forward_error_survival.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

The forward error survival test helpers now initialize kv_cache_retention_config on generated requests and _is_kv_manager_v2 on the validating executor.

Changes

Forward Error Survival Test Setup

Layer / File(s)	Summary
Initialize helper defaults `tests/unittest/executor/test_forward_error_survival.py`	`_request` sets `kv_cache_retention_config` to `None`. `_validating_executor` sets `_is_kv_manager_v2` to `False`.

Priority: ⬇️ Low

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

Change: Other

Merge Risk: ⚪ Minimal · up to bb881

No merge-blocking risk remains for this test-only change.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title names the change: it fixes the _validate_request fakes in test_forward_error_survival. It follows the template with [None][test] and a specific summary. It matches the test-only diff, which …
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The description clearly explains the failure, root cause, fix, affected tests, and test results. It includes the required Description and Test Coverage sections. The PR title follows the required [Non…

Full details: Title check

Explanation

The title names the change: it fixes the _validate_request fakes in test_forward_error_survival. It follows the template with [None][test] and a specific summary. It matches the test-only diff, which adds kv_cache_retention_config=None to the fake request and _is_kv_manager_v2 = False to the executor shell.

Full details: Description check

Explanation

The description clearly explains the failure, root cause, fix, affected tests, and test results. It includes the required Description and Test Coverage sections. The PR title follows the required [None][test] format. The individual checklist items remain unchecked, but this is non-critical because the relevant information is provided.

✨ Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

moraxu · 2026-10-23T07:30:27Z

/bot run --stage-list "CPU-Generic-arm-1"

tensorrt-cicd · 2026-10-23T07:37:00Z

PR_Github #80103 [ run ] triggered by Bot. Commit: bb88175 Link to invocation

tensorrt-cicd · 2026-10-23T13:22:31Z

PR_Github #80103 [ run ] completed with state SUCCESS. Commit: bb88175
/LLM/main/L0_MergeRequest_PR pipeline #65922 (Partly Tested) completed with status: 'SUCCESS'

CI Report

Link to invocation

github-actions · 2026-10-23T16:38:35Z

Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements.

moraxu · 2026-10-23T16:57:50Z

/bot run

tensorrt-cicd · 2026-10-23T17:06:06Z

PR_Github #80217 [ run ] triggered by Bot. Commit: bb88175 Link to invocation

tensorrt-cicd · 2026-10-23T20:53:19Z

PR_Github #80217 [ run ] completed with state SUCCESS. Commit: bb88175
/LLM/main/L0_MergeRequest_PR pipeline #66017 completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

CI Report

Link to invocation

Brings in the main-branch fix for tests/unittest/executor/test_forward_error_survival.py (NVIDIA#20883), which the branch point predates and which failed in this PR's first CI run, and follows the rename of tests/unittest/_torch/executor/ to tests/unittest/_torch/pyexecutor/ (NVIDIA#20669) so that test_forced_chunk_context_limit.py stays in the directory the L0 test lists run. Signed-off-by: berryz <berryz@nvidia.com>

moraxu requested a review from a team as a code owner October 23, 2026 07:27

moraxu requested review from JunyiXu-nv and asfiyab-nvidia October 23, 2026 07:27

github-actions Bot assigned moraxu Oct 23, 2026

coderabbitai Bot mentioned this pull request Oct 23, 2026

[None][infra] Waive 1 failed cases for main in pre-merge 65859 #20895

Merged

yunruis mentioned this pull request Oct 23, 2026

[None][feat] Add the PrimTS native sparse MLA FMHA for DeepSeek-V4 generation and enable it by default on SM100 #20209

Merged

1 task done

Tabrizian approved these changes Oct 23, 2026

View reviewed changes

trtllm-agent added the ci: full pre-merge approved label Oct 23, 2026

Mgluhovskoi mentioned this pull request Oct 23, 2026

[https://nvbugs/6971880][fix] Sync serving schemas on version bumps #20839

Merged

1 task done

moraxu merged commit 461183a into NVIDIA:main Oct 23, 2026
18 of 20 checks passed

trtllm-agent mentioned this pull request Oct 26, 2026

[https://nvbugs/6981910][fix] Unwaive test_forward_error_survival on CPU #21000

Closed

Navigation Menu

Repository navigation

[None][test] Fix the _validate_request fakes in test_forward_error_survival - #20883

[None][test] Fix the _validate_request fakes in test_forward_error_survival#20883
moraxu merged 1 commit into
NVIDIA:mainfrom
moraxu:fix/forward-error-survival-test-fakes

moraxu commented Oct 23, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 23, 2026 •

edited

Loading

Uh oh!

moraxu commented Oct 23, 2026

Uh oh!

tensorrt-cicd commented Oct 23, 2026

Uh oh!

tensorrt-cicd commented Oct 23, 2026

Uh oh!

github-actions Bot commented Oct 23, 2026

Uh oh!

moraxu commented Oct 23, 2026

Uh oh!

tensorrt-cicd commented Oct 23, 2026

Uh oh!

tensorrt-cicd commented Oct 23, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

moraxu commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev Engineer Review

QA Engineer Review

Per-File QA Perspective

Description

Test Coverage

PR Checklist

GitHub Bot Help

Uh oh!

coderabbitai Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

moraxu commented Oct 23, 2026

Uh oh!

tensorrt-cicd commented Oct 23, 2026

Uh oh!

tensorrt-cicd commented Oct 23, 2026

Uh oh!

github-actions Bot commented Oct 23, 2026

Uh oh!

moraxu commented Oct 23, 2026

Uh oh!

tensorrt-cicd commented Oct 23, 2026

Uh oh!

tensorrt-cicd commented Oct 23, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

moraxu commented Oct 23, 2026 •

edited

Loading

coderabbitai Bot commented Oct 23, 2026 •

edited

Loading