[lori-ren]

Dev Engineer Review

model_engine.py adopts standard Ruff formatting and Python 3.10 annotation syntax. The supplied change summary reports no intended behavior changes. Preserve eager annotation evaluation, call signatures, import side effects, and execution and cleanup order.

The configuration changes remove model_engine.py from legacy formatting and file-selection patterns. The author reports that full pre-commit passed and that AST comparison and repeated formatting checks confirmed the intended mechanical changes. The author also reports rerunning the listed tests on H100 after rebasing. These results are author-reported.

QA Engineer Review

No test changes.

Per-File QA Perspective

• .pre-commit-config.yaml: Removes model_engine.py from two hook file patterns. Verify standard hooks select the file as intended.
• legacy-files.txt: Removes model_engine.py from the legacy-file list. Verify legacy handling no longer applies to this file.
• pyproject.toml: Removes the Ruff formatting exclusion for model_engine.py. Verify the configured formatter accepts the file.
• ruff-legacy.toml: Removes model_engine.py from the legacy Ruff include list. Verify formatter scope matches the other configuration changes.
• tensorrt_llm/_torch/pyexecutor/model_engine.py: Updates annotations and formatting. Verify runtime behavior and API compatibility remain unchanged.

Description

model_engine.py still uses the legacy formatting and lint toolchain and older typing aliases. Move it to standard Ruff and Python 3.10 annotation syntax while preserving eager annotation evaluation, call signatures, import side effects, and execution and cleanup order.

Test Coverage

Reran test_pytorch_model_engine.py, test_pytorch_model_engine_warmup.py, engine/test_runners.py, and test_eager_workspace_engine.py on H100 after rebasing onto main at 0b10da8224 (PR head 3c09e8dc36).

Full pre-commit passed inside the container on this rebased head. AST comparison and repeated formatting checks confirmed the intended mechanical changes.

PR Checklist

• Reviewed the applicable coding, test coverage, API, dependency, documentation, and ownership requirements.

[coderabbitai]

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: 5be3334
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 73dbd5c6-f770-4f0d-8882-29016263bdea

Commits

Reviewing files that changed from the base of the PR and between 3c09e8d and ec4edef.

Files selected for processing: 1

• tensorrt_llm/_torch/pyexecutor/model_engine.py

Files skipped from review as they are similar to previous changes: 1

• tensorrt_llm/_torch/pyexecutor/model_engine.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

ModelEngine now uses modern Python type annotations and reformatted expressions. The file is no longer selected by the listed legacy and formatting exclusions. The supplied summaries report no changes to its execution behavior.

Changes

ModelEngine modernization

Layer / File(s)	Summary
Engine setup and runner configuration .pre-commit-config.yaml, legacy-files.txt, pyproject.toml, ruff-legacy.toml, tensorrt_llm/_torch/pyexecutor/model_engine.py	The file was removed from legacy and formatting-exclusion lists. Its initialization, compilation setup, and runner configuration use updated annotations and formatting.
Runtime methods and multimodal handling tensorrt_llm/_torch/pyexecutor/model_engine.py	Metadata access, callable registration, warmup, encoder forwarding, and multimodal handling use updated annotations and formatting. Existing checks and outcomes remain unchanged.
Compilation, refit, and execution lifecycle tensorrt_llm/_torch/pyexecutor/model_engine.py	Sequence-length inference, graph release, compilation, refit, forward execution, and userbuffer initialization are reformatted. Their existing conditions and behavior remain unchanged.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: juney-nvidia, chzblych

Merge Risk: Minimal · up to ec4ed

The typing and formatting changes have no identified merge-blocking risk.

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 with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description check		The description explains the change, states the preservation requirements, lists relevant tests and validation steps, and includes a completed checklist. It is sufficiently complete for this refactori…
Title check		The title identifies the ticket, refactor type, removal from legacy formatting, and modernization of type annotations. It clearly summarizes the main changes.
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.

Full details: Description check

Explanation

The description explains the change, states the preservation requirements, lists relevant tests and validation steps, and includes a completed checklist. It is sufficiently complete for this refactoring pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 38 functions across 1 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

🧪 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.}

[lori-ren]

/bot run

[github-actions]

👎 Promotion blocked, new vulnerability found

Vulnerability report

Component	Vulnerability	Description	Severity
py-filelock	CVE-2026-112906	filelock is a platform-independent file lock for Python. From 4.1.0 until 4.2.1, SoftFileLock versions from 4.1.0 can reuse a stale observation when an attacker pre-dates and removes its own marker, at least two lock-user processes are waiting, coarse or matching modification times preserve the observation, and an honest contender creates a fresh marker on the reused inode, causing that live marker to be removed as stale. This issue is fixed in version 4.2.1.	MEDIUM
py-filelock	CVE-2026-112940	filelock is a platform-independent file lock for Python. From 3.20.4 until 4.2.1, ReadWriteLock let SQLite reopen the database and side files by name; on POSIX systems an attacker with rename access in a non-sticky parent can swap those names to cause overwrite or unlink operations, while a writable adjacent hot journal is additionally required for attacker-chosen content overwrites. This issue is fixed in version 4.2.1.	HIGH

[brnguyen2]

Approving — the comments below are optional touch-ups, not blockers.

Checked the mechanical claim independently: the module AST matches the pre-PR file once annotations and the typing/collections.abc imports are stripped (the one remaining diff is the docstring wrap in get_max_num_sequences), ruff 0.9.4 passes check and format --check, there is no from __future__ import annotations so evaluation stays eager, and scripts/legacy_utils.py check-configs plus ruff-legacy-baseline.json have no leftover reference to the file.

One description fix: the "stacked on #21066, rebase and repeat validation after it merges" paragraph is stale. #21066 is already on the base of this branch and the diff is just these five files. Please drop that paragraph and state whether the H100 test runs listed under Test Coverage were done on the current base.

[lori-ren]

/bot run

[tensorrt-cicd]

PR_Github #81299 [ run ] triggered by Bot. Commit: ec4edef Link to invocation