Repository navigation
Conversation
|
/bot run --disable-fail-fast |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Install LMBenchmark in the test fixture requirements-dev.txt, tests/integration/defs/kv_cache/test_prefix_aware_scheduling.py |
The shared development requirements no longer include LMBenchmark. The test fixture installs the pinned package before resolving its script. The missing-package message and QPS-sweep docstring were updated. |
Estimated code review effort: 2 (Simple) | ~8 minutes
Change: Bug fix
Suggested reviewers: xinhe-nv
Merge Risk: 
1959a
-
Transient network failures can still prevent the affected CI tests from starting. This is a low merge risk but merits follow-up.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the fix: reverting the git pin introduced by commit 8f04d3a. Description check The description explains the release issue and the intended revert. It includes the required sections and checklist. The Test Coverage section is empty, but this is a minor omission for this dependenc… Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… 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 release issue and the intended revert. It includes the required sections and checklist. The Test Coverage section is empty, but this is a minor omission for this dependency-only change.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. 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 2 functions across 1 files.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
|
PR_Github #80957 [ run ] triggered by Bot. Commit: |
|
PR_Github #80957 [ run ] completed with state
|
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com> (cherry picked from commit c55d5b1) Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
| global LMBENCHMARK_SCRIPT | ||
|
|
||
| llm_venv.run_cmd(["-m", "pip", "install", _LMBENCHMARK_INSTALL_SPEC], timeout=600) |
There was a problem hiding this comment.
@tburt-nv Could we add retries around this install to tolerate transient network failures during Git checkout or dependency installation? The existing timeout already handles stalled installs; a small retry would help with temporary failures that exit early.
|
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. |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
This restores the pre-#20232 LMBenchmark handling exactly, and leaves the unrelated grace_s change from that merge in place. The reason checks out: setup.py:160 reads requirements-dev.txt into the devel extra, so the benchmark @ git+... line ended up as a direct-URL dependency in the wheel metadata.
The git install now runs at fixture time, so these l0 tests depend on GitHub being reachable from CI again, same as before #20232. That is fine for now. If the owners want the pin back in a requirements file later, it needs to go in one that setup.py doesn't read.
|
|
||
| This is a correctness stress test; request latency is reported but is | ||
| not a pass/fail threshold. | ||
| NOTE: This test is intended for post-merge / nightly CI due to its |
There was a problem hiding this comment.
This revert brings back the "intended for post-merge / nightly CI" note, but test_multi_round_qa_shared_prefix[*] is listed in pre-merge l0_h100.yml (lines 235-242). Consider keeping the newer "correctness stress test; latency is not a pass/fail threshold" wording so the docstring doesn't contradict the test lists.
|
PR_Github #81943 [ run ] triggered by Bot. Commit: |
|
PR_Github #81943 [ run ] completed with state
|
|
PR_Github #82028 [ run ] triggered by Bot. Commit: |
|
PR_Github #82028 [ run ] completed with state |
|
Resolved two open comments, and merged #21502 instead. |
Description
As part of mass-merge PR #20232, commit 8f04d3a introduced a direct URL reference to the TRT-LLM wheel's dependencies, which makes it unreleaseable. This PR reverts that change so that we can release the wheel again.
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-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin 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.Dev Engineer Review
requirements-dev.txtno longer pinsLMBenchmarkthrough a Git URL. The integration-test fixture installs the pinned revision before resolving its script.PR_Github #80957succeeded, while/LLM/main/L0_MergeRequest_PR #66679failed. The supplied context does not include failure details or a resolution.QA Engineer Review
1959aee.Per-File QA Perspective
requirements-dev.txt: Removing the Git dependency changes the development dependency set. Verify that release workflows do not require this dependency outside the integration-test fixture.tests/integration/defs/kv_cache/test_prefix_aware_scheduling.py: The fixture installs the pinnedLMBenchmarkrevision before resolving its script. The smoke test is listed intest-db/l0_b200.yml,test-db/l0_h100.yml, andqa/llm_function_core.txt; the H100 list also includes parameterized cases. The B300 smoke case is waived inwaives.txt.