Reject NCCL runtimes older than the build - #712
Conversation
|
|
||
| if built_nccl_version is not None: | ||
| runtime_nccl_version = get_nccl_runtime_version(loaded_nccl_so) | ||
| assert runtime_nccl_version >= built_nccl_version, \ |
There was a problem hiding this comment.
🔵 suggestion: 当 /proc/self/maps 中没有任何 libnccl 映射时 loaded_nccl_so 为 None，此时若 built_nccl_version 已设置，会执行 get_nccl_runtime_version(None)，即 ctypes.CDLL(None) 打开主程序句柄，随后对 ncclGetVersion 的解析可能产生难以理解的 AttributeError。建议在版本检查前先断言 loaded_nccl_so is not None 并给出明确提示（旧代码在 filecmp.cmp(None, ...) 处同样会以 TypeError 失败，属既有边缘情况，非本 MR 引入的回归）。
🤖 v5
There was a problem hiding this comment.
Fixed in 841e4e3. check_nccl_so() now fails explicitly with "No NCCL runtime is loaded in the current process" before querying the version or comparing paths. Added a GPU-free regression test.
|
|
||
|
|
||
| def get_nccl_runtime_version(library_path: str) -> int: | ||
| library = ctypes.CDLL(library_path) |
There was a problem hiding this comment.
🔵 suggestion: get_nccl_runtime_version 通过 ctypes.CDLL(library_path) 重新 dlopen 已映射的库路径，由于该库已在进程中加载，这只是增加引用计数，行为正确；但若该 so 不导出 ncclGetVersion（极旧或被裁剪的构建），library.ncclGetVersion 会抛出 AttributeError 而非 RuntimeError，且错误信息不含库路径。建议用 try/except 包装符号解析并转成带路径的可操作错误。
🤖 v5
There was a problem hiding this comment.
Fixed in 841e4e3. A missing ncclGetVersion symbol is now translated to a RuntimeError that includes the library path and missing symbol. Added regression coverage.
| for name in persistent_env_names: | ||
| code += f"persistent_envs['{name}'] = '{os.environ[name]}'\n" if name in os.environ else '' | ||
| nccl_header = os.path.join(find_pkgs.find_nccl_root(), 'include', 'nccl.h') | ||
| built_nccl_version = nccl_utils.read_nccl_header_version(nccl_header) |
There was a problem hiding this comment.
🔵 suggestion: generate_default_envs 现在要求构建时 <nccl_root>/include/nccl.h 必须存在且包含三个版本宏，否则 read_nccl_header_version 抛异常导致构建失败。这是合理的强约束（构建本就依赖 NCCL 头文件），但属于行为变化，建议在发布说明或 README 中提及，便于自定义 EP_NCCL_ROOT_DIR 的用户排查。
🤖 v5
There was a problem hiding this comment.
No README change planned: every supported build already compiles NCCL-dependent CUDA sources with the discovered NCCL include directory, so a root without include/nccl.h was not a usable build configuration before this change. The new parser now fails earlier with the exact missing macro or header path, which is more actionable without expanding the PR documentation scope.
🤖 ds-review-bot Code Reviewv6未发现会破坏现有行为或阻碍目标功能的明确问题。构建版本记录、运行时版本校验以及延迟加载扩展的实现整体一致。 v4v5该 MR 实现完整且与描述一致，共 7 个变更文件、3 个提交：(1) setup.py 在构建时解析 nccl.h 的 NCCL_MAJOR/MINOR/PATCH，把 built_nccl_version 写入生成的 deep_ep/envs.py wheel 元数据；(2) deep_ep/init.py 在导入 deep_ep._C 之前，通过 ctypes 对 /proc/self/maps 中已加载的 libnccl 调用 ncclGetVersion，若运行时版本低于构建版本则以带版本号的可操作 AssertionError 拒绝导入，等于或更新的版本放行；(3) deep_ep/utils/init.py 将兼容性导出 EventHandle 改为模块级 getattr 惰性导入并缓存到 globals()，确保 NCCL 守卫运行前不会加载 _C，且保持 from deep_ep.utils import EventHandle 的向后兼容。源码检出（无生成的 envs.py）保持原有行为，EP_SUPPRESS_NCCL_CHECK=1 仍可跳过全部检查。新增 14 个无 GPU 单元测试覆盖头文件解析、运行时查询、构建元数据生成、导入守卫四种行为场景及惰性导出；tests/test_nccl_import_guard.py 通过委托 real_open 的 open 钩子伪造 /proc/self/maps，实现干净且验证了拒绝发生在 init_jit 之前。经核对：版本编码 major10000+minor100+patch 与 NCCL >= 2.9 的 NCCL_VERSION_CODE 一致（<2.9 的旧编码返回值更小，仍会被正确拒绝，仅错误消息中的运行时版本号会失真，可忽略）；tests/utils/test_utils_init.py 确认导入 utils 包不会把 deep_ep._C 带入 sys.modules。总体质量良好，建议合并；下列均为非阻塞性建议。 Files reviewed: 7 |
2 participants
Summary
deep_ep._CEventHandleexport so_Cis not loaded before the NCCL guardWhy
When another package downgrades NCCL, the existing loaded-vs-installed binary comparison moves both sides to the downgraded library. The check therefore cannot detect that
_Cwas built against a newer NCCL Device API, and users see an opaque undefined-symbol error.Generated wheel metadata now carries
built_nccl_version. At import, DeepEP callsncclGetVersionon the mapped runtime and allows equal or newer versions. Source checkouts without generated metadata keep the existing behavior, andEP_SUPPRESS_NCCL_CHECK=1still bypasses the checks.Testing
PYTHONPYCACHEPREFIX=/tmp/deepep-pycache python3 -m pytest tests/utils/test_nccl.py tests/utils/test_utils_init.py tests/test_nccl_import_guard.py -qPYTHONPYCACHEPREFIX=/tmp/deepep-pycache python3 -m compileall -q deep_ep testsResult: 14 tests passed. These are GPU-free unit tests; no CUDA/NCCL integration run was performed.
Fixes #710