Conversation
Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
|
Caution Review failedThe head commit changed during the review from 7270b5362d151e79b542b8f9773c1b10eb7a556d to 243d56c. 📝 WalkthroughWalkthroughThe protocol initializer now performs ChangesProtocol restoration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Code Review
This pull request wraps the buffer_flags copy operation in _initialize_protocol with torch.inference_mode() to handle cases where the workspace is created under inference mode but restored outside of it. Additionally, a unit test test_protocol_restore_resets_inference_flags has been added to verify this behavior. There are no review comments to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
flashinfer/comm/trtllm_mnnvl_ar.py (1)
178-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared buffer-flags layout to avoid duplication.
The
[0, 2, self.buffer_size_bytes, 0, *num_bytes_to_clear, 0]construction is now duplicated between__init__and_initialize_protocol. Since_initialize_protocolwas touched here, a small helper (e.g.,_default_buffer_flags_values()) would keep both call sites in sync if the layout ever changes.Also applies to: 249-258
🤖 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 `@flashinfer/comm/trtllm_mnnvl_ar.py` around lines 178 - 183, Extract the duplicated buffer-flags layout into a shared helper such as _default_buffer_flags_values(), then use it in both __init__ and _initialize_protocol when constructing self.buffer_flags. Preserve the existing values, dtype, and CUDA device configuration while ensuring both call sites remain synchronized.
🤖 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 `@flashinfer/comm/trtllm_mnnvl_ar.py`:
- Around line 178-183: Extract the duplicated buffer-flags layout into a shared
helper such as _default_buffer_flags_values(), then use it in both __init__ and
_initialize_protocol when constructing self.buffer_flags. Preserve the existing
values, dtype, and CUDA device configuration while ensuring both call sites
remain synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be85c314-97e3-4635-b1b5-6a9da4af8b44
📒 Files selected for processing (2)
flashinfer/comm/trtllm_mnnvl_ar.pytests/comm/test_trtllm_allreduce_checkpoint.py
7270b53 to
243d56c
Compare
The checkpoint test was added by PR flashinfer-ai#3950 but never wired into any CI test script. Add it to task_test_multi_node_comm_kernels.sh so it runs on GB300/GB200 multi-node jobs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/bot run tests/gemm |
|
[FAILED] Pipeline #58040314: 12/20 passed |
The checkpoint test was added by PR #3950 but never wired into any CI test script. Add it to task_test_multi_node_comm_kernels.sh so it runs on GB300/GB200 multi-node jobs. <!-- .github/pull_request_template.md --> ## 📌 Description <!-- What does this PR do? Briefly describe the changes and why they’re needed. --> Add test_trtllm_allreduce_checkpoint.py to task_test_multi_node_comm_kernels.sh ## 🔍 Related Issues <!-- Link any related issues here --> #3966 ## 🚀 Pull Request Checklist Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete. ### ✅ Pre-commit Checks - [ ] I have installed `pre-commit` by running `pip install pre-commit` (or used your preferred method). - [ ] I have installed the hooks with `pre-commit install`. - [ ] I have run the hooks manually with `pre-commit run --all-files` and fixed any reported issues. > If you are unsure about how to set up `pre-commit`, see [the pre-commit documentation](https://pre-commit.com/). ## 🧪 Tests - [ ] Tests have been added or updated as needed. - [ ] All tests are passing (`unittest`, etc.). ## Reviewer Notes <!-- Optional: anything you'd like reviewers to focus on, concerns, etc. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded multi-node communication kernel test coverage to include additional all-reduce and all-to-all scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Description
vLLM can create the FlashInfer MNNVL all-reduce fusion workspace while running graph warmup under
torch.inference_mode(). That makesbuffer_flagsan inference tensor. A later checkpoint restore can run outside inference mode and calls_initialize_protocol(), whose in-placecopy_then fails with:This was reproduced during Dynamo Snapshot + GMS restore of GLM-5.2 with TEP8 after vLLM selected the MNNVL FlashInfer all-reduce backend. The restore failed after remapping the stable workspace VA, while reinitializing the protocol flags.
Run the protocol flag reset inside a narrowly scoped
torch.inference_mode()context. This is the mutation site that requires inference mode, so the change:The regression test creates
buffer_flagsunder inference mode, invokes protocol initialization from normal mode, verifies the expected flag contents, and verifies the caller remains outside inference mode afterward.Related Issues
No associated issue. Related checkpointable MNNVL all-reduce work: #3745.
Checklist
pre-commit run --all-filesand all hooks pass.Tests
Passed:
The focused regression test also passed independently.
Reviewer Notes
The earlier workaround allocated
buffer_flagsoutside inference mode. This version instead fixes the operation that has the constraint: mutating a potentially inference-created tensor during protocol restore. It avoids a separate allocation helper and avoids changing allocation semantics.Summary by CodeRabbit
Bug Fixes
Tests