Skip to content

[CI/Build] Enable KV events in release wheels - #4039

Open
ishandhanani wants to merge 1 commit into
mainfrom
idhanani/enable-kv-events-wheel
Open

[CI/Build] Enable KV events in release wheels#4039
ishandhanani wants to merge 1 commit into
mainfrom
idhanani/enable-kv-events-wheel

Conversation

@ishandhanani

@ishandhanani ishandhanani commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Description

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Reshard (mooncake-reshard)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

Test commands:

# Example: bash scripts/run_ci_test.sh

Test results:

  • Unit tests pass
  • Integration tests pass (if applicable)
  • Manual testing done (describe below)

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
  • I have run pre-commit on the files changed in this PR and all hooks pass
  • I have updated the documentation (if applicable)
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue

AI Assistance Disclosure

  • No AI tools were used
  • AI tools were used (specify below)

Opus

Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
@Aionw

Aionw commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Once CI passed, this PR can be merged.

Copy link
Copy Markdown
Collaborator

The wheel jobs fail during CMake configuration with libzmq development files not found. ENABLE_KV_EVENTS=ON requires ZeroMQ, but the AlmaLinux/RHEL package list in dependencies.sh is missing zeromq-devel (the Debian/Ubuntu list already includes libzmq3-dev). Failing job.

This patch keeps KV events enabled and makes two changes:

  • Install zeromq-devel in the shared RPM dependency branch.
  • Extend the repaired-wheel smoke test to require the master to resolve libzmq inside the installed wheel's site-packages, rejecting a fallback to the builder's system library.

scripts/build_wheel.sh already runs auditwheel repair without excluding libzmq, so no manual library copy or Python dependency change is needed.

dependencies.sh is outside the current PR diff, so the complete change is provided as an applyable patch below. From the repository root on this PR branch (verified against 28020985ff84d0afbcfea0e66d88710fa2484e9f), copy and run:

git apply <<'PATCH'
diff --git a/.github/workflows/_build-wheel.yaml b/.github/workflows/_build-wheel.yaml
index c1ca0533..b53f332f 100644
--- a/.github/workflows/_build-wheel.yaml
+++ b/.github/workflows/_build-wheel.yaml
@@ -221,10 +221,10 @@ jobs:
           python -m venv "$smoke_venv"
           "$smoke_venv/bin/python" -m pip install --no-deps \
             mooncake-wheel/dist-py${{ steps.pytag.outputs.tag }}/*.whl
+          site_packages=$("$smoke_venv/bin/python" -c \
+            'import sysconfig; print(sysconfig.get_paths()["purelib"])')
 
           if [ "${VARIANT_FLAG:-}" = "NON_CUDA_BUILD" ]; then
-            site_packages=$("$smoke_venv/bin/python" -c \
-              'import sysconfig; print(sysconfig.get_paths()["purelib"])')
             cuda_dependency_found=false
             for package_path in "$site_packages"/mooncake*; do
               [ -e "$package_path" ] || continue
@@ -269,6 +269,20 @@ jobs:
             fi
           fi
 
+          # KV events must link to the libzmq bundled by auditwheel, even when
+          # the builder has a system copy that could hide a packaging error.
+          zmq_library=$(ldd "$site_packages/mooncake/mooncake_master" | \
+            awk '$1 ~ /^libzmq[.-]/ && $2 == "=>" {print $3}')
+          case "$(readlink -f "$zmq_library")" in
+            "$site_packages"/*)
+              echo "Wheel uses bundled ZeroMQ: $zmq_library"
+              ;;
+            *)
+              echo "::error::Wheel master must resolve libzmq inside site-packages; got: $zmq_library"
+              exit 1
+              ;;
+          esac
+
           "$smoke_venv/bin/mooncake_master" --version
 
       - name: Upload Python wheel artifact
diff --git a/dependencies.sh b/dependencies.sh
index 1d63009b..e8a11deb 100755
--- a/dependencies.sh
+++ b/dependencies.sh
@@ -208,6 +208,7 @@ elif [ "$OS" = "centos" ] || [ "$OS" = "rhel" ] || [ "$OS" = "rocky" ] || [ "$OS
                      liburing-devel \
                      jemalloc-devel \
                      msgpack-devel \
+                     zeromq-devel \
                      libzstd-devel \
                      pkgconf-pkg-config \
                      elfutils-libelf-devel \
PATCH

Validation: the patch applies cleanly to the exact PR head and reproduces the checked local files. PR-scoped pre-commit, Shell/YAML syntax checks, all six build-profile configurations, and seven smoke-gate cases passed. The gate cases used mocked ldd output on macOS; an actual Linux wheel build, repaired-wheel runtime check, and KV event publication test have not been run with this patch. Please rerun CI after applying it.

AI assistance: patch and local checks prepared with Codex.

@stmatengss stmatengss left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two directly applicable suggestions keep KV events enabled, install the missing ZeroMQ development package before CMake, and verify that the repaired wheel loads its bundled libzmq. Apply both suggestions as a batch. This workflow-local dependency fix replaces the earlier conversation patch. Shell/YAML validation passed; the loader gate passed mocked checks on macOS. Linux wheel build/runtime validation remains pending. AI assistance: Codex.

if [ -n "$SUDO" ] && command -v apt-get >/dev/null 2>&1; then
$SUDO apt-get update -y || true
fi
$SUDO bash -x dependencies.sh -y

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep ENABLE_KV_EVENTS=ON and install the missing native ZeroMQ development package after dependencies.sh has configured the RPM repositories, before CMake runs. This is the workflow-local equivalent of adding zeromq-devel to the RPM dependency list; use these inline suggestions instead of the earlier conversation patch. Debian/Ubuntu already receive libzmq3-dev from dependencies.sh.

Suggested change
$SUDO bash -x dependencies.sh -y
# dependencies.sh configures EPEL/PowerTools before installing packages.
if command -v yum >/dev/null 2>&1; then
$SUDO yum install -y zeromq-devel
fi

fi
fi

"$smoke_venv/bin/mooncake_master" --version

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check that the repaired wheel master resolves libzmq from its installed package directory. This rejects a system-library fallback that could hide a missing bundled dependency. auditwheel repair already handles bundling, and the existing --version check is preserved.

Suggested change
site_packages=$("$smoke_venv/bin/python" -c \
'import sysconfig; print(sysconfig.get_paths()["purelib"])')
# KV events must link to the libzmq bundled by auditwheel, even when
# the builder has a system copy that could hide a packaging error.
zmq_library=$(ldd "$site_packages/mooncake/mooncake_master" | \
awk '$1 ~ /^libzmq[.-]/ && $2 == "=>" {print $3}')
case "$(readlink -f "$zmq_library")" in
"$site_packages"/*)
echo "Wheel uses bundled ZeroMQ: $zmq_library"
;;
*)
echo "::error::Wheel master must resolve libzmq inside site-packages; got: $zmq_library"
exit 1
;;
esac
"$smoke_venv/bin/mooncake_master" --version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants