[https://nvbugs/6127669][fix] faster test_performance_alignment - #14270
[https://nvbugs/6127669][fix] faster test_performance_alignment#14270tburt-nv wants to merge 2 commits into
Conversation
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
|
/bot run --stage-list "DGX_B200-PyTorch-*" |
📝 WalkthroughWalkthroughThe PR parameterizes benchmark configuration through environment variables. The shell script now accepts ChangesBenchmark Environment Parameterization
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unittest/tools/test_layer_wise_benchmarks.py (1)
185-207: QA test list updates are not needed.This change optimizes unittest performance to avoid timeouts without adding new functionality or altering test behavior beyond parameterization. Per the coding guidelines, QA list updates are unnecessary for unittest-only performance tuning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/tools/test_layer_wise_benchmarks.py` around lines 185 - 207, Revert any changes to QA test lists and ensure this PR only adjusts unittest timing parameters: keep the changes confined to test_performance_alignment (the batch_size/output_mean/profile_dir variables and the check_call invocation) and remove any edits that touched QA lists or test metadata (e.g., any QA_TESTS/qa_list entries or CI test matrix modifications) so only the unittest performance tuning remains in the diff.examples/layer_wise_benchmarks/sample_performance_alignment.sh (1)
9-12: ⚡ Quick winConsider adding validation for the OUTPUT_MEAN constraint.
The comment documents that
OUTPUT_MEANmust be greater thanBATCH_SIZE + 35to ensure the profile window is fully contained in the generation phase. However, there's no validation to enforce this constraint. If a user sets invalid values via environment variables, the script will fail during profiling with unclear errors.🛡️ Proposed validation to fail-fast with a clear error
BATCH_SIZE=${BATCH_SIZE:-32} # Per-request output length. Must be > BATCH_SIZE + 35 so the profile window # captured below (TLLM_PROFILE_START_STOP) is fully contained in the GEN phase. OUTPUT_MEAN=${OUTPUT_MEAN:-256} + +if (( OUTPUT_MEAN <= BATCH_SIZE + 35 )); then + echo "ERROR: OUTPUT_MEAN ($OUTPUT_MEAN) must be > BATCH_SIZE + 35 ($(( BATCH_SIZE + 35 )))" >&2 + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/layer_wise_benchmarks/sample_performance_alignment.sh` around lines 9 - 12, The script sets BATCH_SIZE and OUTPUT_MEAN but lacks validation that OUTPUT_MEAN > BATCH_SIZE + 35 as required for TLLM_PROFILE_START_STOP to be contained in the GEN phase; add a numeric check immediately after these variable assignments that validates OUTPUT_MEAN and BATCH_SIZE are integers and that OUTPUT_MEAN is strictly greater than $BATCH_SIZE + 35, and if not print a clear error mentioning OUTPUT_MEAN and BATCH_SIZE (and the required minimum) and exit non‑zero so the user fails fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@examples/layer_wise_benchmarks/sample_performance_alignment.sh`:
- Around line 9-12: The script sets BATCH_SIZE and OUTPUT_MEAN but lacks
validation that OUTPUT_MEAN > BATCH_SIZE + 35 as required for
TLLM_PROFILE_START_STOP to be contained in the GEN phase; add a numeric check
immediately after these variable assignments that validates OUTPUT_MEAN and
BATCH_SIZE are integers and that OUTPUT_MEAN is strictly greater than
$BATCH_SIZE + 35, and if not print a clear error mentioning OUTPUT_MEAN and
BATCH_SIZE (and the required minimum) and exit non‑zero so the user fails fast.
In `@tests/unittest/tools/test_layer_wise_benchmarks.py`:
- Around line 185-207: Revert any changes to QA test lists and ensure this PR
only adjusts unittest timing parameters: keep the changes confined to
test_performance_alignment (the batch_size/output_mean/profile_dir variables and
the check_call invocation) and remove any edits that touched QA lists or test
metadata (e.g., any QA_TESTS/qa_list entries or CI test matrix modifications) so
only the unittest performance tuning remains in the diff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7670581a-9cf7-4a3b-9df6-df9f1fcf3f9c
📒 Files selected for processing (3)
examples/layer_wise_benchmarks/sample_performance_alignment.shtests/integration/test_lists/waives.txttests/unittest/tools/test_layer_wise_benchmarks.py
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
PR_Github #48949 [ run ] triggered by Bot. Commit: |
|
PR_Github #48949 [ run ] completed with state
|
|
/bot run --stage-list "DGX_B200-PyTorch-*" --disable-fail-fast |
|
PR_Github #48993 [ run ] triggered by Bot. Commit: |
|
PR_Github #48993 [ run ] completed with state
|
Summary by CodeRabbit
Chores
Tests
Description
unittest/tools/test_layer_wise_benchmarks.py::test_performance_alignment[1]is too slow and reaches timeout consistently. This PR speeds it up.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.