Ep api design - Build Infra dependencies - #3315
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds submodule pins and Meson patches, build orchestration to optionally build/stage NIXL-EP and NCCL-EP during wheel/editable preparation, runtime import-time probing/loading helpers that raise informative errors, packaging metadata to include staged libs, a Dockerfile for building nvep-enabled images, and .dockerignore/.gitignore updates. ChangesMoE Expert-Parallel Transport Backends
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 adds infrastructure to build and integrate the moe_ep transport backends, NCCL-EP and NIXL-EP, into FlashInfer. It introduces git submodules, build logic in build_backend.py for compilation and patching, and a dedicated Dockerfile. Reviewers suggested improving build portability by checking for various library directory names, enhancing error handling for RPATH modifications, and strengthening dependency probes for git and nvcc. Further feedback recommended safeguarding submodule updates against non-git environments and ensuring import-time warnings account for all backend-specific build flags.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
docker/Dockerfile.flashinfer-nvep (3)
109-109: ⚖️ Poor tradeoffAcknowledge supply-chain risk of pipe-to-shell installation.
Piping
curltoshwithout verification is the officially recommendeduvinstallation method, but it creates a supply-chain attack surface if the install script is compromised.For enhanced security in production environments, consider:
- Downloading the script, verifying its checksum, then executing it.
- Using a pinned release URL instead of the latest version redirect.
- Installing
uvvia a package manager if available for your base image.For a reference Dockerfile intended for development use, the current approach is acceptable.
🤖 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 `@docker/Dockerfile.flashinfer-nvep` at line 109, The RUN line that pipes curl to sh (RUN curl -LsSf https://astral.sh/uv/install.sh | sh) presents a supply-chain risk; replace it by downloading a pinned release script or archive first, verify its checksum/signature, and then execute it (or install uv from a distro package if available). Specifically, change the current RUN step that invokes https://astral.sh/uv/install.sh to a two-step process: fetch a fixed-version URL or release artifact, validate integrity (checksum or signature), and only then run the installer, ensuring the Dockerfile no longer uses an unverified curl | sh pattern.
1-174: ⚡ Quick winConsider adding a non-root USER for production deployments.
The static analysis warning about running as root (DS-0002) is valid. While acceptable for a development/reference image, production deployments should use a non-root user to limit the blast radius of potential container escapes or compromises.
🔒 Adding a non-root user
Add before the final
CMD:# Create non-root user with GPU access RUN groupadd -r flashinfer -g 1000 && \ useradd -r -u 1000 -g flashinfer -G video flashinfer && \ chown -R flashinfer:flashinfer ${FLASHINFER_SRC} ${VENV} USER flashinferNote: GPU access requires the
videogroup or appropriate device permissions. Test with your specific--gpusruntime configuration.If this image is only for development/testing, document that in the header comments and the current approach is acceptable.
🤖 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 `@docker/Dockerfile.flashinfer-nvep` around lines 1 - 174, The image runs as root which triggers DS-0002; before the final CMD add creation of a non-root user/group (e.g. group flashinfer gid 1000 and user flashinfer uid 1000 in group flashinfer, supplementary group video for GPU access), chown the workspace and venv (${FLASHINFER_SRC} and ${VENV}) to that user, and switch to it with USER flashinfer so the container runs unprivileged; update any build steps that require root (apt, installs, UCX build) to remain before this switch and keep CMD ["bash"] unchanged.
47-49: ⚡ Quick winConsider adding checksum verification and documenting URL stability expectations.
The hardcoded DOCA download URL could break if Mellanox reorganizes their downloads or removes old versions. While
--tries=3 --waitretry=5handles transient failures, it won't help if the URL becomes permanently unavailable.💡 Suggested improvements
- Add a comment documenting the expected URL stability and update policy:
# DOCA SDK + GPU Direct Async Kernel-Initiated (GDAKI) headers. # Required for NIXL EP's high-throughput kernels which include # `uct/ib/mlx5/gdaki/gdaki.cuh` from UCX's UCT device API. Without DOCA # installed BEFORE UCX is built, UCX's GDAKI support is omitted and # NIXL EP's HT TUs fail compile. +# NOTE: This URL may become unavailable as Mellanox updates their download +# repository. If builds fail, check https://www.mellanox.com/downloads/DOCA +# for the current download location. ARG DOCA_VERSION=3.2.0-125000-25.10
- Optionally add SHA256 checksum verification:
RUN wget --tries=3 --waitretry=5 --no-verbose \ https://www.mellanox.com/downloads/DOCA/DOCA_v3.2.0/host/doca-host_${DOCA_VERSION}-ubuntu2404_amd64.deb \ -O /tmp/doca-host.deb \ + && echo "EXPECTED_SHA256_HERE /tmp/doca-host.deb" | sha256sum -c - \ && dpkg -i /tmp/doca-host.deb \🤖 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 `@docker/Dockerfile.flashinfer-nvep` around lines 47 - 49, The Dockerfile's RUN that wget's the DOCA package (the command downloading https://.../doca-host_${DOCA_VERSION}-ubuntu2404_amd64.deb into /tmp/doca-host.deb) should include a short comment about URL stability/update policy and perform checksum verification to avoid silently using a tampered/removed file; update the RUN to first download or embed a known SHA256 for the exact DOCA_VERSION and then verify the downloaded /tmp/doca-host.deb with sha256sum (failing the build on mismatch) and remove the downloaded files on failure/success, or alternatively download a vendor-signed checksum and verify it before dpkg install—ensure you reference DOCA_VERSION and /tmp/doca-host.deb in the verification steps and make the build error out if checksums do not match.
🤖 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.
Inline comments:
In `@build_backend.py`:
- Around line 289-292: The subprocess.run call invoking [sys.executable, "-m",
"pip", "install", "--no-deps", *wheels] currently uses check=False and does not
inspect the result, which can silently ignore failed wheel installs; modify this
to detect failures by either using check=True or capturing the CompletedProcess
(result = subprocess.run(..., check=False)) and then checking result.returncode,
and on non-zero return code raise a clear exception or exit with a descriptive
error that includes the failing wheels (the wheels variable) and the return
code/message so runtime wheel install failures are surfaced rather than
swallowed.
- Around line 230-249: _nixl_buildable() currently misses a check for the CUDA
compiler which causes Meson to fail later; add a PATH check for "nvcc" similar
to the check in _nccl_buildable(), returning (False, "nvcc not on PATH (install
CUDA toolkit / add nvcc to PATH)") when shutil.which("nvcc") is falsy so the
function fails fast with a clear actionable message before running
meson/ninja/pkg-config checks.
- Around line 137-139: The staging loop only globs (build /
"examples/device/ep") for "nixl_ep_cpp*.so" but the comment notes Meson may
place the extension in build/ as well; update the logic in build_backend.py to
search both locations (e.g., both build.glob("nixl_ep_cpp*.so") and (build /
"examples/device/ep").glob("nixl_ep_cpp*.so") or iterate over a list of
candidate directories) and copy any found files using the existing
shutil.copy(cand, dst / cand.name) call so the nixl_ep_cpp*.so is reliably
staged from either location. Ensure the loop still handles multiple matches and
does not error if a directory yields no results.
In `@flashinfer/moe_ep/__init__.py`:
- Around line 96-104: The current import-time warning only checks BUILD_NVEP and
misses other per-backend build flags; update the check to consider BUILD_NCCL_EP
and BUILD_NIXL_EP as well (i.e., warn if any of these env vars is "1" and
available_backends() is empty). Modify the conditional around the warning in
__init__.py to test os.environ.get for BUILD_NVEP, BUILD_NCCL_EP, or
BUILD_NIXL_EP (instead of only BUILD_NVEP), and keep the warning text
referencing _pkg_dir and available_backends() unchanged so users still see the
same diagnostic when no backend libraries are found.
---
Nitpick comments:
In `@docker/Dockerfile.flashinfer-nvep`:
- Line 109: The RUN line that pipes curl to sh (RUN curl -LsSf
https://astral.sh/uv/install.sh | sh) presents a supply-chain risk; replace it
by downloading a pinned release script or archive first, verify its
checksum/signature, and then execute it (or install uv from a distro package if
available). Specifically, change the current RUN step that invokes
https://astral.sh/uv/install.sh to a two-step process: fetch a fixed-version URL
or release artifact, validate integrity (checksum or signature), and only then
run the installer, ensuring the Dockerfile no longer uses an unverified curl |
sh pattern.
- Around line 1-174: The image runs as root which triggers DS-0002; before the
final CMD add creation of a non-root user/group (e.g. group flashinfer gid 1000
and user flashinfer uid 1000 in group flashinfer, supplementary group video for
GPU access), chown the workspace and venv (${FLASHINFER_SRC} and ${VENV}) to
that user, and switch to it with USER flashinfer so the container runs
unprivileged; update any build steps that require root (apt, installs, UCX
build) to remain before this switch and keep CMD ["bash"] unchanged.
- Around line 47-49: The Dockerfile's RUN that wget's the DOCA package (the
command downloading https://.../doca-host_${DOCA_VERSION}-ubuntu2404_amd64.deb
into /tmp/doca-host.deb) should include a short comment about URL
stability/update policy and perform checksum verification to avoid silently
using a tampered/removed file; update the RUN to first download or embed a known
SHA256 for the exact DOCA_VERSION and then verify the downloaded
/tmp/doca-host.deb with sha256sum (failing the build on mismatch) and remove the
downloaded files on failure/success, or alternatively download a vendor-signed
checksum and verify it before dpkg install—ensure you reference DOCA_VERSION and
/tmp/doca-host.deb in the verification steps and make the build error out if
checksums do not match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7db5da0-7b55-4bb6-a3e1-370917e76bd1
📥 Commits
Reviewing files that changed from the base of the PR and between 103fcf8 and c28bc150ca53c28f3351c4fba9045bf4d534dada.
📒 Files selected for processing (12)
.dockerignore.gitignore.gitmodules3rdparty/nccl3rdparty/nixl3rdparty_patches/nixl/0001-meson-add-blackwell-arches.patchbuild_backend.pydocker/Dockerfile.flashinfer-nvepflashinfer/moe_ep/__init__.pyflashinfer/moe_ep/nccl_ep/__init__.pyflashinfer/moe_ep/nixl_ep/__init__.pypyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
flashinfer/moe_ep/__init__.py (1)
109-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winImport-time warning should honor
BUILD_NCCL_EPandBUILD_NIXL_EPtoo.The warning only fires for
BUILD_NVEP=1. Users who explicitly opted in viaBUILD_NCCL_EP=1orBUILD_NIXL_EP=1(the per-backend flags introduced inbuild_backend.py) lose this diagnostic when the build silently produced no plugins.Suggested fix
-if os.environ.get("BUILD_NVEP") == "1" and not available_backends(): +_requested_build = any( + os.environ.get(k) == "1" + for k in ("BUILD_NVEP", "BUILD_NCCL_EP", "BUILD_NIXL_EP") +) +if _requested_build and not available_backends(): import warnings warnings.warn( - "BUILD_NVEP=1 was set, but no moe_ep backend libraries were found " + "A BUILD_*_EP=1 flag was set, but no moe_ep backend libraries were found " f"under {_pkg_dir}. Check the build log for meson/make failures.", RuntimeWarning, stacklevel=2, )🤖 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/moe_ep/__init__.py` around lines 109 - 117, The import-time warning currently only checks BUILD_NVEP; change the condition in flashinfer/moe_ep/__init__.py so the warning fires if any per-backend build flag is set (BUILD_NVEP, BUILD_NCCL_EP, BUILD_NIXL_EP) and available_backends() returns false. Update the if expression that wraps warnings.warn to compute a boolean like any(os.environ.get(flag) == "1" for flag in ("BUILD_NVEP","BUILD_NCCL_EP","BUILD_NIXL_EP")) and use that instead of only BUILD_NVEP, keeping the same warning message and referencing _pkg_dir and available_backends() as before.build_backend.py (2)
140-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStaging only covers one of the two documented output locations.
The comment at Line 140 states the torch extension may land in
build/orbuild/examples/device/ep/, but the glob only searches the latter. If Meson placesnixl_ep_cpp*.sounderbuild/(as the comment claims is possible), the build will succeed but the plugin will not be staged intoflashinfer/moe_ep/nixl_ep/_libs/, and the runtime probe inflashinfer/moe_ep/__init__.py::_probe_nixl_epwill report NIXL-EP as unavailable.Suggested fix
- # The torch extension lands either in build/ or build/examples/device/ep/ - for cand in (build / "examples/device/ep").glob("nixl_ep_cpp*.so"): - shutil.copy(cand, dst / cand.name) - print(f"[BUILD_NVEP] staged: {cand.name}") + # The torch extension lands either in build/ or build/examples/device/ep/ + seen: set[str] = set() + for root in (build, build / "examples/device/ep"): + for cand in root.glob("nixl_ep_cpp*.so"): + if cand.name in seen: + continue + shutil.copy(cand, dst / cand.name) + seen.add(cand.name) + print(f"[BUILD_NVEP] staged: {cand.name}")🤖 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 `@build_backend.py` around lines 140 - 143, The staging loop only searches (build / "examples/device/ep") for "nixl_ep_cpp*.so" but the comment says the extension may also be under build/; update the staging logic in build_backend.py (the loop that iterates over build / "examples/device/ep".glob("nixl_ep_cpp*.so") and does shutil.copy to dst and prints "[BUILD_NVEP] staged:") to also search build.glob("nixl_ep_cpp*.so") (or iterate over both candidate directories), copy any matches into dst / cand.name, and avoid duplicate copies so the runtime probe in flashinfer/moe_ep/__init__.py::_probe_nixl_ep can find the plugin.
359-378:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
nvccavailability check to_nixl_buildable().NIXL EP compiles CUDA sources (the patch enables Blackwell gencode arches in
meson.build), sonvccis a hard build-time dep. Currently, all checks pass whilenvccis missing, and the failure surfaces later as an opaque Meson/Ninja error instead of a clear preflight message._nccl_buildable()already does this check at Lines 391–395; mirror it here.Suggested fix
def _nixl_buildable() -> tuple[bool, str]: """Probe for hard NIXL-EP build-time deps. Returns (ok, reason_if_not).""" if not shutil.which("meson"): return False, "meson not on PATH (apt install meson)" if not shutil.which("ninja"): return False, "ninja not on PATH (apt install ninja-build)" + if not shutil.which("nvcc"): + return False, ( + "nvcc not on PATH (install CUDA toolkit and put " + "/usr/local/cuda/bin on $PATH)" + ) pkgconfig = shutil.which("pkg-config")🤖 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 `@build_backend.py` around lines 359 - 378, The _nixl_buildable() probe is missing an nvcc availability check so CUDA sources will fail later; add a check like in _nccl_buildable(): call shutil.which("nvcc") inside _nixl_buildable() and if missing return False with a clear message (e.g. "nvcc not on PATH (install CUDA toolkit / nvcc)") so preflight fails early; place this check alongside the other tool checks (meson, ninja, pkg-config) before invoking pkg-config for ucx/libibverbs.
🧹 Nitpick comments (1)
docker/Dockerfile.flashinfer-nvep (1)
183-185: ⚡ Quick winSmoke probes don't fail the build when no backends were produced.
available_backends()returns[]if both EP plugins are missing, butprint('moe_ep backends:', [])still exits 0 — so a silently failed best-effort build (e.g., NIXL skipped on missing dep) would produce an image whoseflashinfer-nveptag promises both backends but ships neither. Consider asserting in the probe.Suggested fix
-RUN python -c "from flashinfer.moe_ep import available_backends; print('moe_ep backends:', available_backends())" +RUN python -c "from flashinfer.moe_ep import available_backends; \ + b = available_backends(); print('moe_ep backends:', b); \ + assert b, 'no moe_ep backends built — check the build log'"🤖 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 `@docker/Dockerfile.flashinfer-nvep` around lines 183 - 185, The smoke probe currently prints available_backends() but doesn't fail the build if it returns an empty list; change the RUN that invokes python -c "from flashinfer.moe_ep import available_backends; print(...)" to check that available_backends() is non-empty and exit non-zero (raise SystemExit or call sys.exit(1)) when it is empty so the Docker build fails; keep the existing nccl_ep probe (python -c "import nccl_ep; from nccl.core.communicator import Communicator; ...") as-is but ensure the mojo EP check uses available_backends() to assert presence of at least one backend before succeeding.
🤖 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.
Inline comments:
In `@docker/Dockerfile.flashinfer-nvep`:
- Around line 101-106: The comment block stating "DOCA gpunetio — NOT installed
here" is stale and contradicts the earlier apt install of doca-sdk-gpunetio,
libdoca-sdk-gpunetio-dev and libdoca-sdk-verbs-dev (installed from the DOCA host
deb); update or remove that comment to reflect the current state: either delete
the block or rephrase it to acknowledge that doca-sdk-gpunetio and related
packages are installed (referencing the apt install of doca-sdk-gpunetio,
libdoca-sdk-gpunetio-dev, libdoca-sdk-verbs-dev and the DOCA host deb
installation), so maintainers are not misled.
In `@flashinfer/moe_ep/nccl_ep/__init__.py`:
- Around line 71-101: The code currently calls ctypes.CDLL directly in two
places (when nccl_so is found and when loading libnccl_ep.so in
_load_libnccl_ep) which can raise OSError and leak raw errors; wrap both
ctypes.CDLL calls in try/except OSError as e and raise MoEEpNotBuiltError with
an actionable message (include the offending path, e.g. str(nccl_so) or the
_libs_dir / "libnccl_ep.so" value, and suggested install/build steps) using
"raise ... from e" to preserve the original error; update
_preload_libnccl/_load_libnccl_ep call sites accordingly so the module
consistently surfaces MoEEpNotBuiltError instead of raw OSError.
In `@flashinfer/moe_ep/nixl_ep/__init__.py`:
- Around line 74-111: The preload path may miss required base libs and load
failures aren't translated to MoEEpNotBuiltError; update _preload_libnixl to
validate every name in _NIXL_BASE_LIBS (either by trying ctypes.CDLL(libname,
RTLD_GLOBAL) when nixl_lib_dir is None, or by checking nixl_lib_dir / libname
exists and loading it), and if any lib is missing or a ctypes.CDLL raise OSError
capture that exception and raise MoEEpNotBuiltError with a clear message
(include the original exception). Also wrap the final ctypes.CDLL(...) in
_load_nixl_ep_cpp in a try/except OSError and re-raise MoEEpNotBuiltError
(including the underlying error) so extension load failures are reported as
MoEEpNotBuiltError; refer to functions _preload_libnixl, _load_nixl_ep_cpp,
symbol _NIXL_BASE_LIBS, variable nixl_lib_dir and _libs_dir when making the
changes.
---
Duplicate comments:
In `@build_backend.py`:
- Around line 140-143: The staging loop only searches (build /
"examples/device/ep") for "nixl_ep_cpp*.so" but the comment says the extension
may also be under build/; update the staging logic in build_backend.py (the loop
that iterates over build / "examples/device/ep".glob("nixl_ep_cpp*.so") and does
shutil.copy to dst and prints "[BUILD_NVEP] staged:") to also search
build.glob("nixl_ep_cpp*.so") (or iterate over both candidate directories), copy
any matches into dst / cand.name, and avoid duplicate copies so the runtime
probe in flashinfer/moe_ep/__init__.py::_probe_nixl_ep can find the plugin.
- Around line 359-378: The _nixl_buildable() probe is missing an nvcc
availability check so CUDA sources will fail later; add a check like in
_nccl_buildable(): call shutil.which("nvcc") inside _nixl_buildable() and if
missing return False with a clear message (e.g. "nvcc not on PATH (install CUDA
toolkit / nvcc)") so preflight fails early; place this check alongside the other
tool checks (meson, ninja, pkg-config) before invoking pkg-config for
ucx/libibverbs.
In `@flashinfer/moe_ep/__init__.py`:
- Around line 109-117: The import-time warning currently only checks BUILD_NVEP;
change the condition in flashinfer/moe_ep/__init__.py so the warning fires if
any per-backend build flag is set (BUILD_NVEP, BUILD_NCCL_EP, BUILD_NIXL_EP) and
available_backends() returns false. Update the if expression that wraps
warnings.warn to compute a boolean like any(os.environ.get(flag) == "1" for flag
in ("BUILD_NVEP","BUILD_NCCL_EP","BUILD_NIXL_EP")) and use that instead of only
BUILD_NVEP, keeping the same warning message and referencing _pkg_dir and
available_backends() as before.
---
Nitpick comments:
In `@docker/Dockerfile.flashinfer-nvep`:
- Around line 183-185: The smoke probe currently prints available_backends() but
doesn't fail the build if it returns an empty list; change the RUN that invokes
python -c "from flashinfer.moe_ep import available_backends; print(...)" to
check that available_backends() is non-empty and exit non-zero (raise SystemExit
or call sys.exit(1)) when it is empty so the Docker build fails; keep the
existing nccl_ep probe (python -c "import nccl_ep; from nccl.core.communicator
import Communicator; ...") as-is but ensure the mojo EP check uses
available_backends() to assert presence of at least one backend before
succeeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d86a23d-4939-4f38-8b8a-ca8ab5eab7e1
📥 Commits
Reviewing files that changed from the base of the PR and between c28bc150ca53c28f3351c4fba9045bf4d534dada and 8cf421fbcb95fb039c60c49e8d6117848b91c9de.
📒 Files selected for processing (5)
build_backend.pydocker/Dockerfile.flashinfer-nvepflashinfer/moe_ep/__init__.pyflashinfer/moe_ep/nccl_ep/__init__.pyflashinfer/moe_ep/nixl_ep/__init__.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
flashinfer/moe_ep/nixl_ep/__init__.py (1)
65-68: 💤 Low valueNarrow the exception clause to expected types.
Catching bare
Exceptionmasks unexpected errors. The expected failures here areAttributeError(non-package module without__path__),TypeError(non-subscriptable), orIndexError(empty__path__).Suggested fix
try: pkg_root = Path(mod.__path__[0]) - except Exception: + except (AttributeError, TypeError, IndexError): continue🤖 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/moe_ep/nixl_ep/__init__.py` around lines 65 - 68, Replace the broad "except Exception" in the block around "pkg_root = Path(mod.__path__[0])" with a narrow tuple of the expected exception types to avoid masking other errors; catch (AttributeError, TypeError, IndexError) instead so only the cases where mod has no __path__, is not subscriptable, or __path__ is empty are swallowed and all other exceptions propagate.docker/Dockerfile.flashinfer-nvep (1)
72-75: ⚡ Quick winPin UCX to a commit instead of the moving
v1.21.xbranch.Using a branch head makes this image non-reproducible and can break future builds without any repo change. A tested commit SHA is safer for build infra.
🤖 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 `@docker/Dockerfile.flashinfer-nvep` around lines 72 - 75, The Dockerfile currently uses ARG UCX_VERSION=v1.21.x and clones with --branch ${UCX_VERSION}, which pins to a moving branch; change this to a fixed commit SHA by replacing or adding an ARG like UCX_COMMIT and using that SHA in the git clone/checkout step (or clone default and run git checkout <UCX_COMMIT>) so the RUN git clone ... --branch ${UCX_VERSION} line uses a stable commit instead of the v1.21.x branch; update any references from UCX_VERSION to the new UCX_COMMIT symbol and document the tested SHA in the ARG default.
🤖 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.
Inline comments:
In `@build_backend.py`:
- Around line 212-213: The current logic skips running Meson setup when
build.exists() is true, which reuses a stale build dir across different
configurations (BUILD_NIXL_EP_HERMETIC, -Dnixl_ep_only, -Dnixl_wheel_lib_dir);
change it so that if the build directory exists you either remove it or invoke
Meson with reconfigure semantics instead of skipping: detect the existing build
dir (build.exists()) and call subprocess.run(setup_args + ['--reconfigure'],
check=True) or remove the dir and call subprocess.run(setup_args, check=True) to
ensure Meson is reconfigured for the current options.
In `@docker/Dockerfile.flashinfer-nvep`:
- Around line 46-49: The wget URL hard-codes "DOCA_v3.2.0" so overriding ARG
DOCA_VERSION will break; update the Dockerfile so the path is derived from
DOCA_VERSION instead of a fixed string: either add a new ARG (for example
DOCA_SERIES) and set it to the corresponding "DOCA_vX.Y.Z" series and use
${DOCA_SERIES} in the wget URL, or compute the series from DOCA_VERSION inside
the RUN (e.g. use shell expansion ${DOCA_VERSION%%-*} to build
"DOCA_v${DOCA_VERSION%%-*}") so the wget line that currently contains
"https://.../DOCA_v3.2.0/host/doca-host_${DOCA_VERSION}-ubuntu2404_amd64.deb"
becomes dynamically constructed from DOCA_VERSION/DOCA_SERIES.
---
Nitpick comments:
In `@docker/Dockerfile.flashinfer-nvep`:
- Around line 72-75: The Dockerfile currently uses ARG UCX_VERSION=v1.21.x and
clones with --branch ${UCX_VERSION}, which pins to a moving branch; change this
to a fixed commit SHA by replacing or adding an ARG like UCX_COMMIT and using
that SHA in the git clone/checkout step (or clone default and run git checkout
<UCX_COMMIT>) so the RUN git clone ... --branch ${UCX_VERSION} line uses a
stable commit instead of the v1.21.x branch; update any references from
UCX_VERSION to the new UCX_COMMIT symbol and document the tested SHA in the ARG
default.
In `@flashinfer/moe_ep/nixl_ep/__init__.py`:
- Around line 65-68: Replace the broad "except Exception" in the block around
"pkg_root = Path(mod.__path__[0])" with a narrow tuple of the expected exception
types to avoid masking other errors; catch (AttributeError, TypeError,
IndexError) instead so only the cases where mod has no __path__, is not
subscriptable, or __path__ is empty are swallowed and all other exceptions
propagate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3294f496-4426-491d-a600-89e3a00712a3
📥 Commits
Reviewing files that changed from the base of the PR and between 8cf421fbcb95fb039c60c49e8d6117848b91c9de and 180f4a4973d1813954199f723b5535b1fd10a747.
📒 Files selected for processing (5)
3rdparty_patches/nixl/0001-meson-add-blackwell-arches.patch3rdparty_patches/nixl/0002-ep-only-build.patchbuild_backend.pydocker/Dockerfile.flashinfer-nvepflashinfer/moe_ep/nixl_ep/__init__.py
🚧 Files skipped from review as they are similar to previous changes (1)
- 3rdparty_patches/nixl/0001-meson-add-blackwell-arches.patch
Wire NVIDIA NIXL (ai-dynamo/nixl) and NCCL (NVIDIA/nccl) as git submodules
under 3rdparty/ and add a BUILD_NVEP=1 build-time switch that produces the
EP transport libraries in-tree:
- 3rdparty/nixl — pinned to v1.1.0 (05e4243f)
- 3rdparty/nccl — pinned to master HEAD (1933fdd6)
- 3rdparty_patches/nixl/0001-meson-add-blackwell-arches.patch
Replaces NIXL's `-arch=sm_90` flag with multi-gencode covering
sm_90 + sm_100 + sm_103 (H100/B200/B300) plus sm_90 PTX for
forward-compat. Applied automatically to the submodule worktree.
build_backend.py grows _build_nvep_if_enabled() that runs meson on
3rdparty/nixl and `make src.build && make -C contrib/nccl_ep` on
3rdparty/nccl, stages the produced .so files into the flashinfer/moe_ep/
package, fixes RPATHs with patchelf, and editable-installs both nccl_ep
ctypes bindings and nccl4py from the NCCL submodule.
pyproject.toml gains a [nvep] optional-dependencies extra
(nixl-cu13, nvidia-nccl-cu13, cuda-python) and package-data entries so
the staged .so files ship in wheels.
flashinfer/moe_ep/ is a placeholder package with a runtime probe
(have_nccl_ep / have_nixl_ep / available_backends). MoEEpNotBuiltError is
raised when a backend is invoked without its libs. The Fleet/Handle
classes themselves arrive in Part B.
docker/Dockerfile.flashinfer-nvep is a reference image showing the full
system-dep stack (rdma-core, libibverbs-dev, UCX 1.21 with experimental
API, GDRCopy, openmpi) required to make BUILD_NVEP=1 succeed end-to-end.
Validated:
- BUILD_NVEP=0 install: unchanged behavior, vanilla flashinfer works.
- BUILD_NVEP=1 install: pipeline runs through patch overlay,
meson configure (CUDA 12.8 detected), meson subproject downloads
(taskflow, abseil, asio, tomlplusplus, prometheus-cpp), into ninja
compile. Compile fails on the dev box at <infiniband/mlx5dv.h>,
which is the expected system-package gap — full build needs a host
with libmlx5-dev / libibverbs-dev / DOCA gpunetio (the Dockerfile
above installs these).
Wire NVIDIA NIXL (ai-dynamo/nixl) and NCCL (NVIDIA/nccl) as git submodules
under 3rdparty/ and add a BUILD_NVEP=1 build-time switch that produces the
EP transport libraries in-tree:
- 3rdparty/nixl — pinned to v1.1.0 (05e4243f)
- 3rdparty/nccl — pinned to master HEAD (1933fdd6)
- 3rdparty_patches/nixl/0001-meson-add-blackwell-arches.patch
Replaces NIXL's `-arch=sm_90` flag with multi-gencode covering
sm_90 + sm_100 + sm_103 (H100/B200/B300) plus sm_90 PTX for
forward-compat. Applied automatically to the submodule worktree.
build_backend.py grows _build_nvep_if_enabled() that runs meson on
3rdparty/nixl and `make src.build && make -C contrib/nccl_ep` on
3rdparty/nccl, stages the produced .so files into the flashinfer/moe_ep/
package, fixes RPATHs with patchelf, and editable-installs both nccl_ep
ctypes bindings and nccl4py from the NCCL submodule.
pyproject.toml gains a [nvep] optional-dependencies extra
(nixl-cu13, nvidia-nccl-cu13, cuda-python) and package-data entries so
the staged .so files ship in wheels.
flashinfer/moe_ep/ is a placeholder package with a runtime probe
(have_nccl_ep / have_nixl_ep / available_backends). MoEEpNotBuiltError is
raised when a backend is invoked without its libs. The Fleet/Handle
classes themselves arrive later.
docker/Dockerfile.flashinfer-nvep is a reference image showing the full
system-dep stack (rdma-core, libibverbs-dev, UCX 1.21 with experimental
API, GDRCopy, openmpi) required to make BUILD_NVEP=1 succeed end-to-end.
Validated:
- BUILD_NVEP=0 install: unchanged behavior, vanilla flashinfer works.
- BUILD_NVEP=1 install: pipeline runs through patch overlay,
meson configure (CUDA 13.2 detected), meson subproject downloads
(taskflow, abseil, asio, tomlplusplus, prometheus-cpp), into ninja
compile.
Compile fails on the dev box at <infiniband/mlx5dv.h>,
which is the expected system-package gap — full build needs a host
with libmlx5-dev / libibverbs-dev / DOCA gpunetio (the Dockerfile
above installs these).
Three coupled fixes from a successful docker-image build pass:
1. docker/Dockerfile.flashinfer-nvep
- Build UCX 1.20.1 from source (was attempting v1.21.0 which doesn't
exist; apt's libucx0=1.16 lacks the UCS_BIT_GET macro NIXL v1.1.0
needs).
- Add `autoconf automake libtool m4` (UCX autogen.sh deps), `meson`
(apt; for NIXL meson configure), and full IB userspace stack.
- Create /opt/flashinfer-venv via `uv venv` to sidestep Ubuntu 24's
PEP 668 externally-managed-environment block on system pip.
- Post-build editable installs of `nccl_ep` ctypes wrapper and
`nccl4py` Cython bindings, against the target venv (build_backend.py
can't do these because uv's isolated build env has no pip).
- Smoke probes: `from flashinfer.moe_ep import available_backends`
and `import nccl_ep; from nccl.core.communicator import Communicator`.
2. build_backend.py
- For contrib/nccl_ep make: pass NVCC_GENCODE=sm_90+sm_100+sm_103
explicitly. The contrib/nccl_ep Makefile rejects any gencode below
sm_90 (Makefile:15), and NCCL's default gencode includes sm_75/80.
- Remove the in-hook `pip install -e nccl_ep/python` and `pip install
-e nccl4py[cu13]` calls — they failed because `sys.executable`
resolves to uv's isolated build env which has no pip. Defer to
Dockerfile post-build steps.
3. .dockerignore (new)
- Shrinks build context from 5.6GB to ~44MB by excluding .venv/,
build_nvep/, .git/, and the materialized meson subproject dirs that
get re-downloaded inside the container.
- NOTE: `subprojects/*-*/` over-matches via Docker's pattern engine
(also drops .wrap files with `-` in the name). Use exact dir-name
patterns instead.
End-to-end result:
- `docker build -f docker/Dockerfile.flashinfer-nvep -t flashinfer-nvep:dev .`
produces a 15.7 GB image.
- `docker run --rm flashinfer-nvep:dev python -c "from flashinfer.moe_ep
import available_backends; print(available_backends())"` -> ['nccl_ep']
- `docker run --rm flashinfer-nvep:dev python -c "import nccl_ep; from
nccl.core.communicator import Communicator"` -> succeeds.
Known limitation: NIXL-EP backend is silently skipped because meson's
`find_installation('python3')` resolves to uv's isolated build env which
lacks torch. NIXL's examples/device/ep/meson.build then hits its
"PyTorch not found, skipping nixl_ep build" early-exit. Fix in a
follow-up: point meson at /opt/flashinfer-venv/bin/python via a
machine-file or an explicit `-Dpython.path=...` option.
Replace the monolithic BUILD_NVEP=1 with three opt-in switches in
build_backend.py:
BUILD_NCCL_EP=1 → build NCCL-EP from 3rdparty/nccl
BUILD_NIXL_EP=1 → build NIXL-EP from 3rdparty/nixl
BUILD_NVEP=1 → legacy alias: turns BOTH on (back-compat)
Each backend has a different system-dep stack. NIXL-EP needs DOCA
gpunetio + UCX 1.21.x with --with-verbs; NCCL-EP doesn't. On a host
without DOCA, the monolithic BUILD_NVEP=1 used to abort the whole
install when NIXL's compile hit `uct/ib/mlx5/gdaki/gdaki.cuh: No such
file or directory`. With BUILD_NCCL_EP=1, the NIXL build is skipped
entirely and the user gets a working NCCL-EP install instead.
Changes:
- _flag() helper accepts "1"|"true"|"yes"|"on" (case-insensitive)
so BUILD_NCCL_EP=true works the same as BUILD_NCCL_EP=1.
- _build_nvep_if_enabled() dispatches each backend independently,
only fetches the submodule(s) actually being built (saves ~300MB
and a network round-trip on single-backend installs).
- _install_nvep_runtime_wheels() gates each runtime wheel on its
corresponding flag so `pip list` stays honest about what's built.
- docker/Dockerfile.flashinfer-nvep gains ARG BUILD_NCCL_EP= and
ARG BUILD_NIXL_EP= (empty default so BUILD_NVEP=1 stays the image
default). Pass via `docker build --build-arg BUILD_NVEP=0
--build-arg BUILD_NCCL_EP=1 ...` for an NCCL-only image.
Verified via a 10-case truth-table over the three env vars: empty env
→ all False; BUILD_NVEP=1 → all True; each single switch → only that
flag plus its NVEP=False; values "true"/"YES"/"on" recognized; "0"
and empty string both off.
Make BUILD_NVEP=1 resilient to partially-equipped hosts. Before today,
BUILD_NVEP=1 ran NIXL first, NCCL second — so a missing UCX would abort
the meson step, cascade the exception up through the build hook, and
prevent NCCL-EP from being built at all even though all its deps were
present. Now:
- _nixl_buildable() probes for meson, ninja, pkg-config, ucx, libibverbs.
- _nccl_buildable() probes for make and nvcc.
- _BUILD_NVEP_BEST_EFFORT is True only when the user opted in via the
legacy BUILD_NVEP=1 alias AND did NOT also set an explicit
BUILD_NCCL_EP / BUILD_NIXL_EP flag. In that mode, an unbuildable
backend is skipped with a warning. Otherwise (any explicit per-
backend flag) a missing dep is a hard error — the user asked for
that backend specifically.
- Each backend's actual build call is also wrapped in try/except in
best-effort mode so a late failure (e.g. compile error past the
probe) still allows the other backend to build.
- _install_nvep_runtime_wheels() is now passed the set of backends
that ACTUALLY built (not just requested), so pip list stays honest
when one backend was skipped.
Verified via 6-case truth table covering:
- BUILD_NVEP=1, all deps OK → both build
- BUILD_NVEP=1, no UCX → NIXL skipped (warn), NCCL builds
- BUILD_NIXL_EP=1 (explicit), no UCX → RuntimeError
- both explicit, no UCX → RuntimeError (NIXL strict)
- BUILD_NVEP=1 + BUILD_NCCL_EP=1, no UCX → strict (explicit promotes)
- nothing set → no build attempted
This directly answers the "what if my host has CUDA + IB but no UCX/DOCA?"
question: `BUILD_NVEP=1 pip install -e ".[nvep]"` now gives you NCCL-EP
with a clear warning that NIXL-EP was skipped, instead of aborting the
whole install.
Drop libnccl.so.2 (~214 MB) and the libnixl tree from the FlashInfer
package staging. The EP plugin .so files (libnccl_ep.so, nixl_ep_cpp.so)
remain staged into flashinfer/moe_ep/<backend>/_libs/, but the base libs
they depend on are now provided by the pip-installed nvidia-nccl-cu13 /
nixl-cu13 wheels.
Wheel size: ~225 MB smaller for the NCCL case.
Three coordinated changes:
1. build_backend._build_nccl_ep / _build_nixl_ep: stop copying the base
libs. _fix_rpaths drops the $ORIGIN/_libs/nixl_lib entry since that
tree no longer exists.
2. build_backend._install_nvep_runtime_wheels: fix a silent no-op bug
discovered during the host build experiment. The function was calling
[sys.executable, '-m', 'pip', 'install', ...] with check=False. In a
venv created by `uv venv` (no --seed), there is no pip module, so the
subprocess fails with "No module named pip" and gets swallowed by
check=False. After today, prefer `uv pip install --python <sys.executable>`
when uv is on PATH; fall back to `python -m pip` otherwise. Drop
check=False — failure must be visible now that we depend on the
wheels being installed for the base libs.
3. flashinfer.moe_ep.nccl_ep / nixl_ep: new lazy preloaders
_preload_libnccl / _preload_libnixl that ctypes.CDLL the base lib(s)
from the pip wheel's site-packages path (nvidia/nccl/lib/libnccl.so.2,
nixl/lib/x86_64-linux-gnu/libnixl.so) with RTLD_GLOBAL before opening
the EP plugin. Mirrors how PyTorch loads its bundled NCCL. _probe_nccl_ep
no longer checks for libnccl.so.2 — only the plugin matters for the
"did the EP build succeed" question.
Verified end-to-end on this host:
- Stripped libnccl.so.2 from _libs/; available_backends() still
returns ['nccl_ep'].
- _find_libnccl() resolves to .venv/lib/python3.12/site-packages/
nvidia/nccl/lib/libnccl.so.2.
- _install_nvep_runtime_wheels() now correctly upgrades
nvidia-nccl-cu13 from torch's pinned 2.28.9 to 2.30.4 via uv pip.
- _load_libnccl_ep() loads cleanly after the upgrade. Before the
upgrade it failed with "undefined symbol: ncclCommQueryProperties" —
exactly the ABI-drift signal we want users to see if they bypass
the wheel install.
moe_ep: declare nixl-cu13 + nvidia-nccl-cu13 explicitly in Dockerfile
The previous commit (13f0d758) stripped libnccl.so.2 and the libnixl
lib tree from the FlashInfer package and made the EP plugins load
their base libs from the pip-installed nvidia-nccl-cu13 / nixl-cu13
wheels at runtime. _install_nvep_runtime_wheels in build_backend.py
auto-installs those wheels during BUILD_NVEP=1, but the Dockerfile
didn't surface this dependency.
Add an explicit `uv pip install --no-deps 'nixl-cu13>=1.0.1'
'nvidia-nccl-cu13>=2.30.4'` line BEFORE the BUILD_NVEP install step.
Two reasons:
1. The dep tree is now visible by reading the Dockerfile alone,
not buried in build_backend._install_nvep_runtime_wheels.
2. The build-hook's install becomes idempotent (uv pip install
with a satisfied >= constraint is a no-op). If a future change
to _install_nvep_runtime_wheels regresses, the Dockerfile still
produces a working image.
--no-deps is mandatory here: nvidia-nccl-cu13's transitive constraints
would downgrade torch, and nixl-cu13 pulls nvidia-nccl-cu12 which
collides with the cu13 wheel.
moe_ep: skip make src.build, synthesize BUILDDIR from nvidia-nccl-cu13 wheel
Reduce _build_nccl_ep wall time by skipping the ~10-min `make src.build`
step on the NCCL submodule. Since Section 8 (strip-base-libs) and the
existing _install_nvep_runtime_wheels() established that we rely on the
pip-installed nvidia-nccl-cu13 wheel for the base libnccl.so.2 at
runtime, src.build was producing artifacts we immediately threw away.
Investigation (read-only): the contrib/nccl_ep Makefile consumes
$(BUILDDIR)/include and $(BUILDDIR)/lib/libnccl.so. The pip wheel ships
exactly these — including a bit-identical copy of nccl_device.h (verified
via diff -q against the submodule's src/include/nccl_device.h). The
Makefile's -I../../src/include flag is dead weight; contrib/nccl_ep
sources only #include nccl_device.h which is also in the wheel.
Changes:
- _find_nccl_wheel_root(): locate <site-packages>/nvidia/nccl/.
- _synthesize_nccl_builddir(build): create build/ with `include`
symlink → wheel/include, and lib/libnccl.so{,.2} symlinks →
wheel/lib/libnccl.so.2. Two lib symlinks because the linker uses
`-lnccl` (resolves via libnccl.so SONAME) and the SONAME embedded
in libnccl.so.2 is libnccl.so.2.
- _check_nccl_version_drift(): parse NCCL_VERSION_CODE from submodule's
src/nccl.h.in vs wheel's include/nccl.h; warn loudly on mismatch.
- _build_nccl_ep(): replace `make src.build` invocation with
_synthesize_nccl_builddir(); contrib/nccl_ep make unchanged.
- _nccl_buildable(): require nvidia.nccl import-able unless
BUILD_NCCL_EP_HERMETIC=1 (opt-out for fully-from-source builds).
End-to-end verification on this host (BUILD_NCCL_EP=1, BUILD_NVEP off):
- EXIT=0; wall time 70:34 (comparable to prior 70-min BUILD_NVEP=1
runs that DID do src.build — savings here are masked by host
contention; cicc compute_103 alone took 13 min vs ~5 min in less
contended runs).
- build_nvep/nccl/include and lib/libnccl.so.2 are SYMLINKS to the
pip wheel — proves src.build never ran.
- flashinfer/moe_ep/nccl_ep/_libs/libnccl_ep.so staged (9.2 MB);
libnccl.so.2 NOT staged.
- `available_backends()` == ['nccl_ep'].
- `_load_libnccl_ep()` resolves libnccl.so.2 from the wheel, loads
libnccl_ep.so successfully.
- All expected symbols present: ncclEpCreateGroup, ncclEpCreateHandle,
ncclEpDispatch, ncclEpCombine, ncclEpComplete, etc.
Trade-off: builds in cleanrooms without PyPI access need
BUILD_NCCL_EP_HERMETIC=1 to fall back to the prior `make src.build`
behavior.
Mirror the Section 9 NCCL-EP change for NIXL: build only the
nixl_ep_cpp.so torch extension and link it against the libnixl.so
shipped in the nixl-cu13 pip wheel, instead of compiling the full
parent NIXL meson tree (src/ + plugins) on every install.
A new patch overlay 3rdparty_patches/nixl/0002-ep-only-build.patch
adds two meson options to the v1.1.0 submodule pin:
-Dnixl_ep_only=true skips subdir('src'), subdir('test'),
install_headers; routes directly to
examples/device/ep.
-Dnixl_wheel_lib_dir=PATH feeds cc.find_library('nixl', dirs:[PATH])
so the EP example links against an
external libnixl.so.
build_backend._build_nixl_ep now probes for nixl-cu13 via the
meson-python sidecar `<site-packages>/.nixl_cu13.mesonpy.libs/` and
passes those options; the fallback flag BUILD_NIXL_EP_HERMETIC=1
restores the original full-tree build for hosts without the wheel.
The runtime loader `_find_nixl_lib_dir` is fixed to look at the same
`.nixl_cu13.mesonpy.libs/` path — the prior `import nixl` probe was
silently broken against the meson-python-packaged wheel (which
installs as `nixl_cu13`, not `nixl`) and only worked because the
preload fell back to ldconfig's default search.
A `_time_phase` context manager prints per-phase wall times so future
build logs distinguish NIXL vs NCCL compile costs. uv currently
swallows the build hook's stdout under `pip install -e`, so the
prints don't appear in docker layer logs today — but they do appear
under `python -m pip install -v` and inside the BUILD_NIXL_EP_HERMETIC
path on the host.
Per-backend timings for this commit (docker, derived from artifact
mtimes in flashinfer-nvep:dev image sha 77b25256):
NIXL-EP compile (nixl_ep_only) 102 s ~1m 42s
NCCL-EP compile (wheel-linked) 1599 s ~26m 39s
step flashinfer-ai#17 total (incl. uv I/O) 2305 s ~38m 25s
full docker build ~2302 s ~38m 22s
End-to-end smoke probe inside the image:
available_backends() -> ['nccl_ep', 'nixl_ep']
_load_nixl_ep_cpp() -> loads OK
_load_libnccl_ep() -> loads OK
nccl_ep/_libs/ -> libnccl_ep.so (8.6 MB) only
nixl_ep/_libs/ -> nixl_ep_cpp.cpython-*.so (10.4 MB) only
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…st sm_90 Patch 0001 only added the multi-arch -gencode flags to the EP module's nvcc cuda_args (compile-side). With NIXL's -rdc=true device-link configuration, the project-wide nvcc_flags_link in the parent meson.build still listed only -gencode=arch=compute_90,code=sm_90 -- so nvcc correctly produced .o files containing sm_90/sm_100/sm_103 cubins but nvlink then discarded the sm_100 and sm_103 sections during the final device link, leaving an sm_90-only nixl_ep_cpp.so. Verified via cuobjdump --list-elf nixl_ep_cpp.cpython-*.so on the prior commit: only sm_90.cubin was present. Extend 0001 to also patch the parent meson.build so the project-wide nvcc_flags and nvcc_flags_link include -gencode entries for sm_100 and sm_103 when build_nixl_ep=true. Gated behind the EP option so non-EP NIXL builds (and other consumers of the submodule) are unaffected. Post-fix verification (flashinfer-nvep:dev image after this commit): cuobjdump --list-elf nixl_ep_cpp.cpython-*.so -> sm_90, sm_100, sm_103 available_backends() -> ['nccl_ep', 'nixl_ep'] Wall time impact (docker step flashinfer-ai#17): 2304.8s -> 2319.7s (~+15s). The compile is fast because nixl_ep only has 4 .cu + 2 .cpp files, all small kernels; nvcc reuses preprocessing across gencode targets. Apprx build time ┌──────────────────────────────────────────────────────────────────┬───────────────────┐ │ phase │ wall time │ ├──────────────────────────────────────────────────────────────────┼───────────────────┤ │ NIXL-EP compile (nixl_ep_only) │ 102 s — 1m 42s │ ├──────────────────────────────────────────────────────────────────┼───────────────────┤ │ NCCL-EP compile (wheel-linked) │ 1599 s — 26m 39s │ ├──────────────────────────────────────────────────────────────────┼───────────────────┤ │ step flashinfer-ai#17 total (incl. uv resolve + downloads + editable install) │ 2305 s — 38m 25s │ ├──────────────────────────────────────────────────────────────────┼───────────────────┤ │ full docker build │ ~2302 s — 38m 22s │ └──────────────────────────────────────────────────────────────────┴───────────────────┘ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
flashinfer/moe_ep/nixl_ep/__init__.py (1)
91-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreload all required NIXL base libs and wrap load failures consistently.
Current logic can proceed with a partial base-lib set and can leak raw
OSError(including at Line 128), which undermines predictableMoEEpNotBuiltErrordiagnostics.Suggested fix
def _preload_libnixl() -> None: @@ nixl_lib_dir = _find_nixl_lib_dir() - if nixl_lib_dir is None: - # Try the dynamic linker's default search for the minimum lib. - try: - ctypes.CDLL("libnixl.so", mode=ctypes.RTLD_GLOBAL) - return - except OSError as e: - raise MoEEpNotBuiltError( - "Could not locate the NIXL runtime libraries. Install with " - "one of:\n" - " uv pip install --no-deps 'nixl-cu13>=1.0.1'\n" - " pip install --no-deps 'nixl-cu13>=1.0.1'\n" - "or set LD_LIBRARY_PATH to a directory containing libnixl.so." - ) from e - - for libname in _NIXL_BASE_LIBS: - libpath = nixl_lib_dir / libname - if libpath.exists(): - ctypes.CDLL(str(libpath), mode=ctypes.RTLD_GLOBAL) + candidates = ( + _NIXL_BASE_LIBS + if nixl_lib_dir is None + else tuple(str(nixl_lib_dir / libname) for libname in _NIXL_BASE_LIBS) + ) + try: + for candidate in candidates: + ctypes.CDLL(candidate, mode=ctypes.RTLD_GLOBAL) + except OSError as e: + raise MoEEpNotBuiltError( + "Could not locate or load the NIXL runtime libraries. Install with " + "one of:\n" + " uv pip install --no-deps 'nixl-cu13>=1.0.1'\n" + " pip install --no-deps 'nixl-cu13>=1.0.1'\n" + "or set LD_LIBRARY_PATH to a directory containing the NIXL runtime libs." + ) from e @@ def _load_nixl_ep_cpp() -> ctypes.CDLL: @@ _preload_libnixl() - return ctypes.CDLL(str(so_files[0]), mode=ctypes.RTLD_GLOBAL) + try: + return ctypes.CDLL(str(so_files[0]), mode=ctypes.RTLD_GLOBAL) + except OSError as e: + raise MoEEpNotBuiltError( + f"Failed to load staged NIXL-EP extension at {so_files[0]}. " + "Rebuild with BUILD_NIXL_EP=1 or BUILD_NVEP=1." + ) from eAlso applies to: 127-128
🤖 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/moe_ep/nixl_ep/__init__.py` around lines 91 - 112, In _preload_libnixl(), ensure every library in _NIXL_BASE_LIBS is loaded and any ctypes.CDLL OSError is caught and re-raised as MoEEpNotBuiltError with a clear message; if _find_nixl_lib_dir() returns None keep the current attempt to load "libnixl.so" but wrap its OSError into MoEEpNotBuiltError, and in the loop over _NIXL_BASE_LIBS catch OSError for each libpath (or missing file) and raise MoEEpNotBuiltError rather than letting raw OSError propagate so callers of _preload_libnixl get consistent diagnostics.flashinfer/moe_ep/nccl_ep/__init__.py (1)
73-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize NCCL loader failures to
MoEEpNotBuiltError.Line 73 and Line 101 can raise raw
OSError, which bypasses the actionable error path this module is trying to provide.Suggested fix
def _preload_libnccl() -> None: @@ nccl_so = _find_libnccl() if nccl_so is not None: - ctypes.CDLL(str(nccl_so), mode=ctypes.RTLD_GLOBAL) - return + try: + ctypes.CDLL(str(nccl_so), mode=ctypes.RTLD_GLOBAL) + return + except OSError as e: + raise MoEEpNotBuiltError( + f"Failed to load NCCL runtime at {nccl_so}. " + "Ensure the nvidia-nccl-cu13 runtime is compatible with this environment, " + "or expose a compatible libnccl.so.2 via LD_LIBRARY_PATH." + ) from e @@ def _load_libnccl_ep() -> ctypes.CDLL: @@ _preload_libnccl() - return ctypes.CDLL(str(so), mode=ctypes.RTLD_GLOBAL) + try: + return ctypes.CDLL(str(so), mode=ctypes.RTLD_GLOBAL) + except OSError as e: + raise MoEEpNotBuiltError( + f"Failed to load staged NCCL-EP plugin at {so}. " + "Rebuild with BUILD_NCCL_EP=1 or BUILD_NVEP=1." + ) from eAlso applies to: 100-101
🤖 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/moe_ep/nccl_ep/__init__.py` around lines 73 - 74, Wrap the ctypes.CDLL(...) calls that load nccl_so in a try/except that catches OSError and raises MoEEpNotBuiltError instead (preserving the original error message as context), replacing the raw return-on-success behavior; specifically update the blocks that call ctypes.CDLL(str(nccl_so), mode=ctypes.RTLD_GLOBAL) so any OSError is normalized to MoEEpNotBuiltError with the original exception chained or included in the message (reference symbols: nccl_so, ctypes.CDLL, and MoEEpNotBuiltError).flashinfer/moe_ep/__init__.py (1)
109-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude granular EP build flags in the import-time warning gate.
The warning currently only triggers for
BUILD_NVEP=1; users settingBUILD_NCCL_EP=1orBUILD_NIXL_EP=1won’t get this diagnostic when no backend is staged.Suggested fix
-if os.environ.get("BUILD_NVEP") == "1" and not available_backends(): +_requested_build = any( + os.environ.get(k) == "1" for k in ("BUILD_NVEP", "BUILD_NCCL_EP", "BUILD_NIXL_EP") +) +if _requested_build and not available_backends():🤖 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/moe_ep/__init__.py` at line 109, The import-time warning gate currently only checks BUILD_NVEP; update the condition that reads if os.environ.get("BUILD_NVEP") == "1" and not available_backends(): to trigger when any EP build flag is set by checking BUILD_NVEP, BUILD_NCCL_EP, and BUILD_NIXL_EP (e.g. replace the single check with any(os.environ.get(v) == "1" for v in ("BUILD_NVEP","BUILD_NCCL_EP","BUILD_NIXL_EP")) and not available_backends()). Ensure available_backends() is still used to suppress the warning when a backend is staged.
🧹 Nitpick comments (1)
docker/Dockerfile.flashinfer-nvep (1)
193-195: ⚡ Quick winMake the smoke probe fail when NIXL-EP is missing.
The first probe only prints
available_backends(), so this image still builds successfully if NIXL quietly disappears. Since this Dockerfile is meant to validate the full nvep path, assert the expected backend set instead of logging it.Suggested check
-RUN python -c "from flashinfer.moe_ep import available_backends; print('moe_ep backends:', available_backends())" +RUN python - <<'PY' +from flashinfer.moe_ep import available_backends +backends = set(available_backends()) +print("moe_ep backends:", sorted(backends)) +assert {"nccl_ep", "nixl_ep"} <= backends, backends +PY🤖 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 `@docker/Dockerfile.flashinfer-nvep` around lines 193 - 195, The smoke probe should fail when NIXL-EP is missing: replace the current RUN that just prints available_backends() with a Python check that raises a nonzero exit when the expected backend set is not present (e.g. assert 'nixl_ep' in available_backends() or assert set(available_backends()) == {...expected_backends...}); update the RUN invoking available_backends() (from flashinfer.moe_ep) so it asserts the presence/identity of NIXL-EP and exits nonzero on mismatch, optionally keeping a print for success.
🤖 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.
Inline comments:
In `@build_backend.py`:
- Around line 223-236: The builder silently continues when no plugin .so files
are found; modify the staging loops (e.g., the loop iterating (build /
"examples/device/ep").glob("nixl_ep_cpp*.so") that copies into dst = _moe_ep_pkg
/ "nixl_ep" / "_libs") to count how many artifacts were actually copied and, if
the count is zero, raise an exception (RuntimeError or similar) with a clear
message before any code sets built_nixl or built_nccl to True; apply the same
pattern to the other builder loops referenced in the comment (the ranges around
lines 397-408 and 643-669) so each builder fails loudly when no expected
artifacts were staged.
- Around line 512-578: The runtime-wheel installation in
_install_nvep_runtime_wheels is currently executed during both wheel builds and
editable installs, which causes base libs to be installed into an isolated
PEP517 build env and not bundled with distributed wheels; change the call sites
instead of installing during wheel builds: remove or gate calls to
_install_nvep_runtime_wheels from the wheel-build path (the invocation in
_prepare_for_wheel) and ensure it is only invoked from the editable-install path
(the invocation in _prepare_for_editable), or alternatively add a boolean
parameter to _install_nvep_runtime_wheels (e.g. editable_only=True) and have
callers pass the appropriate flag so that the function no-ops during wheel
creation; update error/messages accordingly and keep the function signature
_install_nvep_runtime_wheels(built_nixl: bool, built_nccl: bool, editable_only:
bool=False) if you choose the flag approach.
In `@docker/Dockerfile.flashinfer-nvep`:
- Around line 66-74: The Dockerfile currently pins UCX with ARG
UCX_VERSION=v1.21.x which tracks a moving branch; replace that with the
immutable commit SHA by setting ARG
UCX_VERSION=c982cef7cdfc80008c838741ff4cc38a4ce54c89 (or otherwise use that SHA
as the ref in the git clone command) so the RUN git clone --depth=1 --branch
${UCX_VERSION} https://github.com/openucx/ucx.git /tmp/ucx will always fetch the
exact commit; update the ARG value and/or the clone invocation to reference the
provided SHA instead of the branch name.
---
Duplicate comments:
In `@flashinfer/moe_ep/__init__.py`:
- Line 109: The import-time warning gate currently only checks BUILD_NVEP;
update the condition that reads if os.environ.get("BUILD_NVEP") == "1" and not
available_backends(): to trigger when any EP build flag is set by checking
BUILD_NVEP, BUILD_NCCL_EP, and BUILD_NIXL_EP (e.g. replace the single check with
any(os.environ.get(v) == "1" for v in
("BUILD_NVEP","BUILD_NCCL_EP","BUILD_NIXL_EP")) and not available_backends()).
Ensure available_backends() is still used to suppress the warning when a backend
is staged.
In `@flashinfer/moe_ep/nccl_ep/__init__.py`:
- Around line 73-74: Wrap the ctypes.CDLL(...) calls that load nccl_so in a
try/except that catches OSError and raises MoEEpNotBuiltError instead
(preserving the original error message as context), replacing the raw
return-on-success behavior; specifically update the blocks that call
ctypes.CDLL(str(nccl_so), mode=ctypes.RTLD_GLOBAL) so any OSError is normalized
to MoEEpNotBuiltError with the original exception chained or included in the
message (reference symbols: nccl_so, ctypes.CDLL, and MoEEpNotBuiltError).
In `@flashinfer/moe_ep/nixl_ep/__init__.py`:
- Around line 91-112: In _preload_libnixl(), ensure every library in
_NIXL_BASE_LIBS is loaded and any ctypes.CDLL OSError is caught and re-raised as
MoEEpNotBuiltError with a clear message; if _find_nixl_lib_dir() returns None
keep the current attempt to load "libnixl.so" but wrap its OSError into
MoEEpNotBuiltError, and in the loop over _NIXL_BASE_LIBS catch OSError for each
libpath (or missing file) and raise MoEEpNotBuiltError rather than letting raw
OSError propagate so callers of _preload_libnixl get consistent diagnostics.
---
Nitpick comments:
In `@docker/Dockerfile.flashinfer-nvep`:
- Around line 193-195: The smoke probe should fail when NIXL-EP is missing:
replace the current RUN that just prints available_backends() with a Python
check that raises a nonzero exit when the expected backend set is not present
(e.g. assert 'nixl_ep' in available_backends() or assert
set(available_backends()) == {...expected_backends...}); update the RUN invoking
available_backends() (from flashinfer.moe_ep) so it asserts the
presence/identity of NIXL-EP and exits nonzero on mismatch, optionally keeping a
print for success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fdcc28f-85d6-472d-8c95-2f62a8366b31
📥 Commits
Reviewing files that changed from the base of the PR and between 180f4a4973d1813954199f723b5535b1fd10a747 and 12f7a12.
📒 Files selected for processing (13)
.dockerignore.gitignore.gitmodules3rdparty/nccl3rdparty/nixl3rdparty_patches/nixl/0001-meson-add-blackwell-arches.patch3rdparty_patches/nixl/0002-ep-only-build.patchbuild_backend.pydocker/Dockerfile.flashinfer-nvepflashinfer/moe_ep/__init__.pyflashinfer/moe_ep/nccl_ep/__init__.pyflashinfer/moe_ep/nixl_ep/__init__.pypyproject.toml
✅ Files skipped from review due to trivial changes (4)
- 3rdparty/nixl
- .gitmodules
- .gitignore
- .dockerignore
🚧 Files skipped from review as they are similar to previous changes (4)
- 3rdparty/nccl
- pyproject.toml
- 3rdparty_patches/nixl/0001-meson-add-blackwell-arches.patch
- 3rdparty_patches/nixl/0002-ep-only-build.patch
Real bugs flagged by coderabbitai + gemini-code-assist on PR flashinfer-ai#3315: * _nixl_buildable / _nccl_buildable now probe for nvcc and git up front, matching what the build actually runs (nvcc for CUDA kernels, git for `git apply` of the patch overlays). Previously a missing nvcc on PATH led to "best-effort" silent skips that produced an EP-less wheel. * `git submodule update --init` is now guarded on `.git` existing in the superproject. Without the guard, sdist installs (which travel with submodule trees expanded inline and no `.git` metadata) crash with "not a git repository" instead of either succeeding or surfacing a clear error. * `_build_nixl_ep` re-runs `meson setup --reconfigure` when the build dir already exists. Previously a stale config persisted across patch / option changes (e.g. flipping BUILD_NIXL_EP_HERMETIC, bumping the wheel path) and required a manual rm -rf to take effect. * `_fix_rpaths` no longer swallows patchelf failures silently. It still uses check=False (patchelf legitimately exits nonzero on files that already have the desired RPATH), but now prints the stderr/stdout on nonzero rc so real failures (e.g. missing .dynamic) surface in the build log. * Import-time warning in flashinfer.moe_ep now fires for any of BUILD_NVEP / BUILD_NCCL_EP / BUILD_NIXL_EP being set rather than only BUILD_NVEP. The previous heuristic missed the per-backend flags entirely. * Narrowed `except Exception` in _find_nixl_lib_dir to the actual expected error set (AttributeError / IndexError / TypeError) so an unrelated failure (e.g. permission error reading site-packages) doesn't get masked. * ctypes.CDLL failure paths in _preload_libnccl / _load_libnccl_ep / _preload_libnixl / _load_nixl_ep_cpp now translate raw OSError into MoEEpNotBuiltError with an actionable rebuild hint (BUILD_*_EP_HERMETIC=1 or --force-reinstall the matching wheel), so callers don't have to interpret cryptic dlopen messages. * Dockerfile: removed the stale "DOCA gpunetio — NOT installed here" comment block that contradicted the apt install step right above it. * Dockerfile smoke probe now asserts that both 'nccl_ep' and 'nixl_ep' appear in available_backends(); previously it just printed the list and a silent best-effort skip in the build hook produced an empty list without failing the image build. Pushed back on (not addressed in this commit): * DOCA URL sha256 checksum — Mellanox URL is version-pinned; risk is bounded by version, adding/maintaining a checksum is more friction than it's worth for an internal dev image. * UCX `v1.21.x` branch -> commit SHA pin — matches upstream NIXL's own contrib/Dockerfile pin; deviating would create drift. * `curl | sh` for the uv installer — standard install pattern per astral.sh/uv docs. * Run as root in container — NVIDIA's official CUDA base images do the same; switching to a non-root user breaks the GPU device permissions on most clusters that mount /dev/nvidia* with root ownership. * x86_64-linux-gnu hardcode in _find_nixl_wheel_lib_dir — the probe has a fallback chain that covers the alternate aarch64 layout via the bare `lib/` and meson-python `.{pkg}.mesonpy.libs/` candidates; no functional gap on aarch64. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the hardcoded `x86_64-linux-gnu` fallback in both _find_nixl_wheel_lib_dir (build-time probe) and _find_nixl_lib_dir (runtime loader) with `<platform.machine()>-linux-gnu`, so the probe resolves to `aarch64-linux-gnu` on NVIDIA Grace / AWS Graviton hosts without falling through to the less-specific bare `lib/` candidate. No functional change on x86_64 (the probe already worked via the meson-python `.nixl_*.mesonpy.libs/` sidecar, with the multiarch candidate as a backup). Addresses one of the bot review comments on PR flashinfer-ai#3315 that flagged the x86_64 hardcode as Ubuntu-specific; this makes the fallback chain explicitly arch-aware while keeping the same behavior on x86_64 hosts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (4)
docker/Dockerfile.flashinfer-nvep (1)
46-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the DOCA download path in sync with
DOCA_VERSION.
DOCA_VERSIONis exposed as an ARG, but the URL hardcodesDOCA_v3.2.0. OverridingDOCA_VERSIONto a different series (e.g.,3.3.0-*) will cause a 404.Suggested fix
ARG DOCA_VERSION=3.2.0-125000-25.10 RUN wget --tries=3 --waitretry=5 --no-verbose \ - https://www.mellanox.com/downloads/DOCA/DOCA_v3.2.0/host/doca-host_${DOCA_VERSION}-ubuntu2404_amd64.deb \ + https://www.mellanox.com/downloads/DOCA/DOCA_v${DOCA_VERSION%%-*}/host/doca-host_${DOCA_VERSION}-ubuntu2404_amd64.deb \ -O /tmp/doca-host.deb \🤖 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 `@docker/Dockerfile.flashinfer-nvep` around lines 46 - 49, The Dockerfile hardcodes "DOCA_v3.2.0" in the wget URL which won't match other DOCA_VERSION values; update the RUN/wget invocation to derive the path from the DOCA_VERSION ARG (e.g., interpolate DOCA_VERSION into the URL or build a DOCA_URL ARG) so the segment "DOCA_v<version>" is generated from DOCA_VERSION rather than hardcoded, ensuring the download path and the DOCA_VERSION variable stay in sync.build_backend.py (3)
245-248:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStage
nixl_ep_cpp*.sofrom both documented output locations.The comment at Line 245 indicates the torch extension can land in either
build/orbuild/examples/device/ep/, but Line 246 only globs the latter. This can leave NIXL built but undetected.Suggested fix
- # The torch extension lands either in build/ or build/examples/device/ep/ - for cand in (build / "examples/device/ep").glob("nixl_ep_cpp*.so"): - shutil.copy(cand, dst / cand.name) - print(f"[BUILD_NVEP] staged: {cand.name}") + # The torch extension lands either in build/ or build/examples/device/ep/ + staged_count = 0 + for root in (build, build / "examples/device/ep"): + for cand in root.glob("nixl_ep_cpp*.so"): + shutil.copy(cand, dst / cand.name) + staged_count += 1 + print(f"[BUILD_NVEP] staged: {cand.name}") + if staged_count == 0: + raise RuntimeError( + "NIXL-EP build completed but no nixl_ep_cpp*.so was produced. " + "Check the Meson build output for errors." + )🤖 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 `@build_backend.py` around lines 245 - 248, The staging loop only searches (build / "examples/device/ep") for "nixl_ep_cpp*.so" but the comment says the extension may land in build/ as well; update the logic in build_backend.py (the loop that uses (build / "examples/device/ep").glob(...) and copies to dst) to search both locations — e.g., iterate over both build.glob("nixl_ep_cpp*.so") and (build / "examples/device/ep").glob("nixl_ep_cpp*.so"), copy any matches to dst, and avoid double-copying the same filename if found in both places.
541-607:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftWheel builds won't carry runtime base library dependencies.
This function runs during both
_prepare_for_wheel()and_prepare_for_editable()(via_build_nvep_if_enabled()). For editable installs, the wheels are installed into the user's environment. For wheel builds, PEP 517 isolation means these install into a temporary build environment that doesn't transfer to the final installation.Distributed wheels will ship the EP plugins (
libnccl_ep.so,nixl_ep_cpp.so) but lack the base libraries (libnccl.so.2,libnixl.so) they depend on. The[nvep]optional dependency only declarescuda-python>=13.0.Consider one of:
- Add
nvidia-nccl-cu13andnixl-cu13to[nvep]optional-dependencies in pyproject.toml- Skip
_install_nvep_runtime_wheels()during wheel builds (keep for editable only)- Document that distributed wheels require manual installation of these runtime wheels
🤖 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 `@build_backend.py` around lines 541 - 607, The current _install_nvep_runtime_wheels installs runtime base-library wheels even during PEP517 wheel builds (via calls from _prepare_for_wheel), causing distributed wheels to miss libnccl/libnixl; change the flow so runtime wheels are only installed for editable installs: modify _build_nvep_if_enabled and its callers (_prepare_for_editable and _prepare_for_wheel) to accept and propagate a flag like install_runtime (default False), call _install_nvep_runtime_wheels only when install_runtime is True (i.e., from _prepare_for_editable), and leave wheel-build paths skipping that function; alternatively, if you prefer shipping runtime deps, add nvidia-nccl-cu13 and nixl-cu13 to the [nvep] optional-dependencies in pyproject.toml instead (pick one approach and apply consistently).
416-421:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail the build if
libnccl_ep.sowas not staged.The staging loop at Lines 416-420 silently completes even if
libnccl_ep.sodoesn't exist in the build output. This can markbuilt_nccl = Trueupstream while producing a package with no usable EP plugin.Suggested fix
+ staged_count = 0 for soname in ("libnccl_ep.so",): sopath = build / "lib" / soname if sopath.exists(): shutil.copy(sopath, dst / soname) + staged_count += 1 print(f"[BUILD_NVEP] staged: {soname}") + if staged_count == 0: + raise RuntimeError( + "NCCL-EP build completed but libnccl_ep.so was not produced. " + "Check the contrib/nccl_ep Makefile output for errors." + )🤖 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 `@build_backend.py` around lines 416 - 421, The staging loop over soname ("libnccl_ep.so",) currently does nothing when the file is missing, so downstream code (e.g., built_nccl flag) can be set incorrectly; update the block that iterates soname/sopath to explicitly fail the build when libnccl_ep.so is not found by raising an exception or calling sys.exit(1) with a clear message (e.g., "libnccl_ep.so not found, failing build"), or alternatively return/raise from the function that contains this loop; ensure you reference the same variables (soname, sopath, build, dst) and adjust any logic that sets built_nccl so it only becomes True after a successful copy.
🤖 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.
Duplicate comments:
In `@build_backend.py`:
- Around line 245-248: The staging loop only searches (build /
"examples/device/ep") for "nixl_ep_cpp*.so" but the comment says the extension
may land in build/ as well; update the logic in build_backend.py (the loop that
uses (build / "examples/device/ep").glob(...) and copies to dst) to search both
locations — e.g., iterate over both build.glob("nixl_ep_cpp*.so") and (build /
"examples/device/ep").glob("nixl_ep_cpp*.so"), copy any matches to dst, and
avoid double-copying the same filename if found in both places.
- Around line 541-607: The current _install_nvep_runtime_wheels installs runtime
base-library wheels even during PEP517 wheel builds (via calls from
_prepare_for_wheel), causing distributed wheels to miss libnccl/libnixl; change
the flow so runtime wheels are only installed for editable installs: modify
_build_nvep_if_enabled and its callers (_prepare_for_editable and
_prepare_for_wheel) to accept and propagate a flag like install_runtime (default
False), call _install_nvep_runtime_wheels only when install_runtime is True
(i.e., from _prepare_for_editable), and leave wheel-build paths skipping that
function; alternatively, if you prefer shipping runtime deps, add
nvidia-nccl-cu13 and nixl-cu13 to the [nvep] optional-dependencies in
pyproject.toml instead (pick one approach and apply consistently).
- Around line 416-421: The staging loop over soname ("libnccl_ep.so",) currently
does nothing when the file is missing, so downstream code (e.g., built_nccl
flag) can be set incorrectly; update the block that iterates soname/sopath to
explicitly fail the build when libnccl_ep.so is not found by raising an
exception or calling sys.exit(1) with a clear message (e.g., "libnccl_ep.so not
found, failing build"), or alternatively return/raise from the function that
contains this loop; ensure you reference the same variables (soname, sopath,
build, dst) and adjust any logic that sets built_nccl so it only becomes True
after a successful copy.
In `@docker/Dockerfile.flashinfer-nvep`:
- Around line 46-49: The Dockerfile hardcodes "DOCA_v3.2.0" in the wget URL
which won't match other DOCA_VERSION values; update the RUN/wget invocation to
derive the path from the DOCA_VERSION ARG (e.g., interpolate DOCA_VERSION into
the URL or build a DOCA_URL ARG) so the segment "DOCA_v<version>" is generated
from DOCA_VERSION rather than hardcoded, ensuring the download path and the
DOCA_VERSION variable stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0e4ad11-9441-4966-9577-d3c9e89fab9a
📒 Files selected for processing (5)
build_backend.pydocker/Dockerfile.flashinfer-nvepflashinfer/moe_ep/__init__.pyflashinfer/moe_ep/nccl_ep/__init__.pyflashinfer/moe_ep/nixl_ep/__init__.py
|
/bot run |
|
@Anerudhan is not authorized to trigger this CI job. cc: @yzh119, @sricketts, @yongwww |
|
/bot run |
dierksen
left a comment
There was a problem hiding this comment.
I can't really evaluate the intent/implementation of these dependencies, but the infra side LGTM.
|
wanna talk to Ane at 4:30 before merging |
| 3rdparty/nixl/subprojects/.wraplock | ||
|
|
||
| # git internals (not needed; submodules already at correct commits via COPY) | ||
| .git/ |
There was a problem hiding this comment.
So these aren't submodules but copies ? Oh this is docker ignore file... misunderstood
| @@ -0,0 +1,65 @@ | |||
| From: FlashInfer build infra | |||
There was a problem hiding this comment.
What's this file for ? Patching at runtime ?
| # pattern) so they don't drag transitive constraints that downgrade torch. | ||
| # Only cuda-python is listed here because it has no torch-conflicting deps. | ||
| nvep = [ | ||
| "cuda-python>=13.0", |
There was a problem hiding this comment.
Does torch exposed cuda access not satisfy it?
| # flashinfer/moe_ep/{nixl_ep,nccl_ep}/_libs/ (gitignored; populated by | ||
| # build_backend._build_nvep_if_enabled). | ||
| "flashinfer.moe_ep.nixl_ep" = ["_libs/**"] | ||
| "flashinfer.moe_ep.nccl_ep" = ["_libs/*.so*"] |
There was a problem hiding this comment.
Im fuzzy what the package-data is used for...
|
Turned out I didn't submit my comments from mobile they were pending ... but reviewed in a meeting everything good |
📌 Description
This PR allows the user to build the NCCL-EP and NIXL-EP as part Flashinfer install
🔍 Related Issues
🚀 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
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
Reviewer Notes
Added NCCL-EP and NIXL-EP as git sub-modules (and optional pip installs).
The following commits will have tests and FI logic.
If using build-isolation:
Summary by CodeRabbit
New Features
Chores