[https://nvbugs/6435097][fix] Remove the single stale nvbugs/6427411 waiver line for… - #16266
[https://nvbugs/6435097][fix] Remove the single stale nvbugs/6427411 waiver line for…#16266trtllm-agent wants to merge 1 commit into
nvbugs/6427411 waiver line for…#16266Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThe integration waiver list removes the Changes
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
03137a5 to
261fd97
Compare
261fd97 to
384414e
Compare
mzweilz
left a comment
There was a problem hiding this comment.
LGTM if the unwaived test passes.
fredricz-20070104
left a comment
There was a problem hiding this comment.
Review summary - Approve
Reviewed the full diff; no blocking or major issues found.
Minor, non-blocking notes:
tests/integration/test_lists/waives.txt: Sibling waiver for the same bug id 6427411 left in place
Automated review by NVCortex Lite, run by @fredricz-20070104.
384414e to
5878223
Compare
The TorchSampler IndexError described in nvbug 6435097 was introduced by PR NVIDIA#15920 ("Move greedy stop checks to host") and cleanly reverted by PR NVIDIA#16163 (commit 9a8ec05). PR NVIDIA#16163 is already present at HEAD, so the underlying code bug is fixed. The waiver at nvbugs/6427411 for test_ctx_pp_gen_tp_asymmetric[GSM8K-gen_tp=1-ctx_pp=2] was added by PRs NVIDIA#16103 / NVIDIA#16105 / NVIDIA#16127 before the revert landed and was not cleaned up afterward. Verified locally on this HEAD (e523b43): pytest tests/integration/defs/accuracy/test_disaggregated_serving.py::\ TestLlama3_1_8BInstruct::test_ctx_pp_gen_tp_asymmetric[GSM8K-gen_tp=1-ctx_pp=2] -> PASSED, GSM8K accuracy = 73.465 (threshold 70.997). Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
5878223 to
f0380c6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Summary
use_host_stop_criteriafast path that under-populated per-step/seq_slot/beam token buffers on non-last PP ranks, causing IndexError; PR [TRTLLM-14155][fix] Revert host-side greedy stop checks from #15920 #16163 (commit 9a8ec05) reverted PR [None][perf] Move greedy stop checks to host #15920 and is present at HEAD, so the code is already fixed. The waiver line for this test was left behind by cleanup PRs [https://nvbugs/6427411][chore] Waive failed tests in Pre-merge #16103/[https://nvbugs/6427411][chore] Waive a failed test in Pre-merge #16105/[https://nvbugs/6427411][chore] Waive remaining PP sampler regressions #16127.nvbugs/6427411waiver line fortest_ctx_pp_gen_tp_asymmetric[GSM8K-gen_tp=1-ctx_pp=2]inwaives.txt; do not touch other test entries or other bug IDs.Test plan
Links
Dev Engineer Review
nvbugs/6427411waiver andSKIPmarker forTestLlama3_1_8BInstruct::test_ctx_pp_gen_tp_asymmetric[GSM8K-gen_tp=1-ctx_pp=2].waives.txtentry.QA Engineer Review
test-db/orqa/files were modified.tests/integration/test_lists/waives.txt.