Skip to content

Conversation

@lancelly
Copy link
Collaborator

@lancelly lancelly commented Jan 29, 2026

Fix the sorting order in schedule_attention_dp_requests to correctly prioritize non-relaxed requests. The comment states "Prioritize the requests that are not in relax mode", but reverse=True causes relaxed requests (get_relax_value=True)
to be processed first instead. Removing reverse=True ensures non-relaxed requests are scheduled to their target DP rank before slots fill up.

Summary by CodeRabbit

  • Performance Improvements
    • Optimized request scheduling prioritization in attention processing to improve system efficiency.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
@lancelly lancelly requested a review from a team as a code owner January 29, 2026 16:32
@lancelly lancelly requested a review from achartier January 29, 2026 16:32
@lancelly
Copy link
Collaborator Author

/bot run --disable-fail-fast

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 29, 2026

📝 Walkthrough

Walkthrough

A sorting order change in the request scheduling function flips how requests are prioritized based on the get_relax_value key, reversing from descending to ascending order. The functional behavior and control flow remain otherwise unchanged.

Changes

Cohort / File(s) Summary
Request Scheduling Priority
tensorrt_llm/_torch/pyexecutor/executor_request_queue.py
Modified sort order in _schedule_attention_dp_requests from descending to ascending for get_relax_value, altering request prioritization logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description lacks required sections from the template: Test Coverage section is missing, and the PR Checklist is incomplete or absent. Add a Test Coverage section listing relevant tests that safeguard these changes, and complete the PR Checklist with appropriate checkboxes marked as required by the template.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: correcting the sorting order to prioritize non-relaxed requests in attention DP scheduling.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@lancelly
Copy link
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd
Copy link
Collaborator

PR_Github #34151 [ run ] triggered by Bot. Commit: 887da94

@tensorrt-cicd
Copy link
Collaborator

PR_Github #34151 [ run ] completed with state SUCCESS. Commit: 887da94
/LLM/main/L0_MergeRequest_PR pipeline #26348 completed with status: 'FAILURE'

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

@lancelly
Copy link
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd
Copy link
Collaborator

PR_Github #34176 [ run ] triggered by Bot. Commit: 887da94

@tensorrt-cicd
Copy link
Collaborator

PR_Github #34176 [ run ] completed with state SUCCESS. Commit: 887da94
/LLM/main/L0_MergeRequest_PR pipeline #26370 completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

@lancelly lancelly merged commit f2dd0ee into NVIDIA:main Jan 30, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants