[None][chore] re-enable benchmark test in post merge#12035
[None][chore] re-enable benchmark test in post merge#12035zhenhuaw-me wants to merge 4 commits intoNVIDIA:mainfrom
Conversation
|
/bot run |
Also remove duplicate tests. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
📝 WalkthroughWalkthroughThis PR consolidates multiple benchmark test variants in the visual_gen test suite into unified functions. The online benchmark test now accepts an additional Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (3)
tests/integration/defs/visual_gen/test_visual_gen_benchmark.py (3)
304-312: Same redundant assertion pattern.Same issue as
test_online_benchmark: thecheck=Trueparameter makes the returncode assertion redundant.🔧 Proposed fix
result = subprocess.run( cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, check=True, ) - assert result.returncode == 0 assert "Benchmark Result (VisualGen)" in result.stdout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/defs/visual_gen/test_visual_gen_benchmark.py` around lines 304 - 312, The subprocess.run invocation sets check=True, so the subsequent assertion assert result.returncode == 0 is redundant; remove the redundant assert or change check to False if you intend to assert returncode manually—specifically update the subprocess.run call in the test (the call that assigns result from subprocess.run(..., check=True)) and delete the following assert result.returncode == 0 to avoid duplicate checks.
240-248: Redundant assertion aftercheck=True.When
subprocess.run()is called withcheck=True, it raisesCalledProcessErroron non-zero exit codes. The assertion on line 248 is therefore redundant and will never fail (if the returncode were non-zero, the exception would have already been raised).🔧 Proposed fix
result = subprocess.run( cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, check=True, ) - assert result.returncode == 0 assert "Benchmark Result (VisualGen)" in result.stdout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/defs/visual_gen/test_visual_gen_benchmark.py` around lines 240 - 248, The assertion checking result.returncode is redundant because subprocess.run is called with check=True; either remove the assert result.returncode == 0 or change subprocess.run(..., check=False) and then assert the return code and/or inspect stderr; locate the subprocess.run call in this test and remove the final assert (or switch check to False if you intend to assert manually) so the test behavior is consistent with how errors are handled.
36-36: Consider importing the module instead of the function directly.As per the coding guidelines, prefer importing the module rather than individual functions:
-from tensorrt_llm._utils import get_free_port +from tensorrt_llm import _utilsThen use
_utils.get_free_port()at line 101.As per coding guidelines: "Import the module, not individual classes or functions (e.g., use
from package.subpackage import foothenfoo.SomeClass()instead offrom package.subpackage.foo import SomeClass)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/defs/visual_gen/test_visual_gen_benchmark.py` at line 36, Replace the direct function import with a module import so callers use the module namespace; change the import of get_free_port to import tensorrt_llm._utils as _utils and update the call site(s) (e.g., where get_free_port is invoked around line 101) to use _utils.get_free_port(). Ensure any other references to get_free_port are updated to the module-qualified name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/defs/visual_gen/test_visual_gen_benchmark.py`:
- Around line 304-312: The subprocess.run invocation sets check=True, so the
subsequent assertion assert result.returncode == 0 is redundant; remove the
redundant assert or change check to False if you intend to assert returncode
manually—specifically update the subprocess.run call in the test (the call that
assigns result from subprocess.run(..., check=True)) and delete the following
assert result.returncode == 0 to avoid duplicate checks.
- Around line 240-248: The assertion checking result.returncode is redundant
because subprocess.run is called with check=True; either remove the assert
result.returncode == 0 or change subprocess.run(..., check=False) and then
assert the return code and/or inspect stderr; locate the subprocess.run call in
this test and remove the final assert (or switch check to False if you intend to
assert manually) so the test behavior is consistent with how errors are handled.
- Line 36: Replace the direct function import with a module import so callers
use the module namespace; change the import of get_free_port to import
tensorrt_llm._utils as _utils and update the call site(s) (e.g., where
get_free_port is invoked around line 101) to use _utils.get_free_port(). Ensure
any other references to get_free_port are updated to the module-qualified name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4cb1d9a9-89a7-4b4d-84d2-ff1fbd9fb030
📒 Files selected for processing (2)
tests/integration/defs/visual_gen/test_visual_gen_benchmark.pytests/integration/test_lists/test-db/l0_dgx_b200.yml
zhenhuaw-me
force-pushed
the
enable-benchmark-test
branch
from
c8ea32e to
9d57ca7
Compare
|
PR_Github #38263 [ run ] triggered by Bot. Commit: |
|
PR_Github #38263 [ run ] completed with state
|
|
/bot run |
|
PR_Github #38274 [ run ] triggered by Bot. Commit: |
|
PR_Github #38274 [ run ] completed with state
|
|
/bot run |
|
PR_Github #38344 [ run ] triggered by Bot. Commit: |
chang-l
left a comment
chang-l
left a comment
There was a problem hiding this comment.
Maybe we should explicitly enable the post-merge stage CI to double-check before merging?
|
PR_Github #38344 [ run ] completed with state
|
|
/bot help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
3 participants
Also remove duplicate tests.
Summary by CodeRabbit
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)
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.