[Chore] Separate out NCCL utilities from vllm.utils #27197
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively separates the NCCL utility functions from vllm.utils into a new vllm.utils.nccl module, which improves code organization and modularity. The changes are clean and follow the refactoring goal. I have one suggestion to improve error handling in the new nccl.py module.
vllm/utils/nccl.py
Outdated
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Using a broad except Exception: pass can hide important errors. If an unexpected issue occurs while searching for the nvidia.nccl package's include paths, it will be silently ignored. This could lead to downstream compilation failures that are difficult to debug. It's better to at least log the exception to aid in troubleshooting.
| except Exception: | |
| pass | |
| except Exception as e: | |
| logger.debug("Failed to find nccl include path from nvidia.nccl package: %s", e) |
dongbo910220
force-pushed
the
feature/refactor-nccl-utils
branch
from
aad29b5 to
d6273aa
Compare
Move NCCL library discovery functions to vllm/utils/nccl.py: - find_nccl_library: NCCL/RCCL library resolver - find_nccl_include_paths: NCCL header path resolver This reduces vllm/utils/__init__.py complexity and improves modularity for distributed computing utilities. Contributes to vllm-project#26900 Signed-off-by: dongbo910220 <1275604947@qq.com>
dongbo910220
force-pushed
the
feature/refactor-nccl-utils
branch
from
d6273aa to
14ee2bf
Compare
…ils.nccl - Replace legacy imports in pynccl_* files with vllm.utils.nccl. - Drop re-exports from vllm.utils.__init__ (no back-compat). Signed-off-by: dongbo910220 <1275604947@qq.com>
dongbo910220
force-pushed
the
feature/refactor-nccl-utils
branch
from
57b1314 to
97f234a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively refactors NCCL-related utility functions out of vllm.utils into a new, dedicated module, vllm.utils.nccl. This improves modularity and code organization. The changes are well-executed: imports are correctly updated, and the new module includes improved docstrings and more informative logging for previously silent errors. The overall code quality is excellent, and I have no high or critical severity concerns with this change.
github-actions
bot
added
the
ready
|
Hi @DarkLight1337 , The failing test
The validation logic that rejects Note: The test with |
Signed-off-by: dongbo910220 <1275604947@qq.com>
Signed-off-by: dongbo910220 <1275604947@qq.com>
Signed-off-by: dongbo910220 <1275604947@qq.com> Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: dongbo910220 <1275604947@qq.com> Signed-off-by: 0xrushi <6279035+0xrushi@users.noreply.github.com>
Signed-off-by: dongbo910220 <1275604947@qq.com> Signed-off-by: 0xrushi <6279035+0xrushi@users.noreply.github.com>
Signed-off-by: dongbo910220 <1275604947@qq.com>
3 participants
Part of #26900
This PR continues the effort to clean up
vllm.utilsby extracting NCCL-related helper functions into a new, dedicated modulevllm/utils/nccl.py.The following functions have been moved:
vllm.utils.find_nccl_library -> vllm.utils.nccl.find_nccl_libraryvllm.utils.find_nccl_include_paths -> vllm.utils.nccl.find_nccl_include_pathsCC: @DarkLight1337
Test Plan
Test Result