chore(deps): update Mooncake to v0.3.9 - #1313
staryxchen wants to merge 37 commits into
Conversation
- Modify build script to dynamically fetch and clone the latest release tag. - Update ATTRIBUTIONS-CPP.md to reflect version v0.3.9. Signed-off-by: staryxchen <staryxchen@tencent.com>
|
👋 Hi staryxchen! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
/build |
|
/ok to test ebe718f |
- change`--sort=-v:refSort`to`--sort=-v:refname` - add continuation character after echo command Signed-off-by: staryxchen <staryxchen@tencent.com>
|
Hi @ovidiusm |
|
/ok to test fa7d185 |
|
/build |
|
The results of two CI checks (nixl-ci-gpu and nixl-ci-non-gpu) are not visible. I'm not sure what the issue is. |
📝 WalkthroughWalkthroughThe pull request updates the Mooncake dependency handling and CI infrastructure. The build script now dynamically resolves the Mooncake version from git tags instead of using a static clone. The Mooncake version is bumped to v0.3.9, and CI image tags are updated to a new build variant. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Signed-off-by: staryxchen <staryxchen@tencent.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Stary <staryxchen@tencent.com>
|
Hi @brminich, I've updated the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.gitlab/build.sh (2)
222-224:⚠️ Potential issue | 🟠 MajorValidate and quote
MOONCAKE_VERSIONbefore cloning.Line 222 should use
--refsand fail fast when no matching tag is found; Line 224 should quote"${MOONCAKE_VERSION}". As written, annotated-tag output or an empty resolution can make this step brittle, and the unquoted branch name still triggers SC2086.🔧 Proposed hardening
- MOONCAKE_VERSION=$(git ls-remote --tags --sort=-v:refname https://github.com/kvcache-ai/Mooncake.git 'v*' | head -1 | sed 's/.*refs\/tags\///') && \ + MOONCAKE_VERSION=$(git ls-remote --refs --tags --sort=-v:refname https://github.com/kvcache-ai/Mooncake.git 'v*' | head -1 | sed 's/.*refs\/tags\///') && \ + [ -n "${MOONCAKE_VERSION}" ] || { echo "Failed to resolve Mooncake tag." >&2; exit 1; } && \ echo "MOONCAKE_VERSION: ${MOONCAKE_VERSION}" && \ - git clone --depth 1 --branch ${MOONCAKE_VERSION} https://github.com/kvcache-ai/Mooncake.git && \ + git clone --depth 1 --branch "${MOONCAKE_VERSION}" https://github.com/kvcache-ai/Mooncake.git && \Run the following script to compare the current lookup with a refs-only lookup:
#!/bin/bash set -euo pipefail echo "=== build.sh snippet ===" sed -n '222,224p' .gitlab/build.sh echo echo "=== raw git ls-remote output ===" git ls-remote --tags --sort=-v:refname https://github.com/kvcache-ai/Mooncake.git 'v*' | head -5 echo echo "=== refs-only git ls-remote output ===" git ls-remote --refs --tags --sort=-v:refname https://github.com/kvcache-ai/Mooncake.git 'v*' | head -5Expected result: the refs-only output excludes any
^{}deref entries, the selected tag is non-empty, and the clone command should quote the branch variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitlab/build.sh around lines 222 - 224, The MOONCAKE_VERSION resolution and clone are brittle: change the git ls-remote call that sets MOONCAKE_VERSION to use --refs (so it excludes ^{} deref entries) and make the script fail fast if no tag was found (i.e., test that MOONCAKE_VERSION is non-empty before proceeding), and then quote the variable when cloning (use "${MOONCAKE_VERSION}") to avoid word-splitting/SC2086; update the assignment that uses git ls-remote and the git clone invocation that references MOONCAKE_VERSION accordingly.
222-224:⚠️ Potential issue | 🟠 MajorDo not resolve Mooncake from “latest” at build time.
This still makes the build non-reproducible and can drift past the version recorded in
ATTRIBUTIONS-CPP.md, so a future Mooncake release changes what this PR builds without any code change. Source the version from a checked-in pin or explicit CI input instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitlab/build.sh around lines 222 - 224, Replace the dynamic resolution of MOONCAKE_VERSION via git ls-remote with a pinned source: read MOONCAKE_VERSION from a checked-in pin file or an explicit CI variable (e.g., an environment variable provided by CI), and refuse to proceed if that value is empty; update the git clone invocation to use that pinned MOONCAKE_VERSION (referencing the MOONCAKE_VERSION variable and the git clone line) and remove the ls-remote lookup so builds are reproducible and match ATTRIBUTIONS-CPP.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.gitlab/build.sh:
- Around line 222-224: The MOONCAKE_VERSION resolution and clone are brittle:
change the git ls-remote call that sets MOONCAKE_VERSION to use --refs (so it
excludes ^{} deref entries) and make the script fail fast if no tag was found
(i.e., test that MOONCAKE_VERSION is non-empty before proceeding), and then
quote the variable when cloning (use "${MOONCAKE_VERSION}") to avoid
word-splitting/SC2086; update the assignment that uses git ls-remote and the git
clone invocation that references MOONCAKE_VERSION accordingly.
- Around line 222-224: Replace the dynamic resolution of MOONCAKE_VERSION via
git ls-remote with a pinned source: read MOONCAKE_VERSION from a checked-in pin
file or an explicit CI variable (e.g., an environment variable provided by CI),
and refuse to proceed if that value is empty; update the git clone invocation to
use that pinned MOONCAKE_VERSION (referencing the MOONCAKE_VERSION variable and
the git clone line) and remove the ls-remote lookup so builds are reproducible
and match ATTRIBUTIONS-CPP.md.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82f6eaa7-59b8-4723-a2e1-3e694dbd8bcb
📒 Files selected for processing (3)
.ci/jenkins/lib/build-matrix.yaml.ci/jenkins/lib/test-matrix.yaml.gitlab/build.sh
|
@staryxchen Need to resolve conflicts first |
|
Hi @roiedanino @brminich, This PR has accumulated too many conflicts with the latest main branch. I've created a clean re-implementation based on the latest main: #1448 Closing this one in favor of the new PR. Thanks for all the reviews and feedback! |
18 participants
What?
This PR changes the build script to automatically resolve the latest stable release tag (via git ls-remote --tags) and clone that specific version, ensuring NIXL always builds against a known-good Mooncake release. The ATTRIBUTIONS file is updated accordingly to reflect Mooncake v0.3.9, which includes the fix for the initialization crash.
Why?
Previously, the build script pinned Mooncake at a shallow clone of the default branch (--depth 1), which could pull in unstable or breaking changes. This caused issues such as the segmentation fault reported in kvcache-ai/Mooncake#1510, where mooncake-transfer-engine 0.3.8.post1 crashes with signal 11 during TransferEngine initialization when loaded alongside vLLM + NIXL + LMCache, due to a library collision or a regression introduced in the 0.3.8 refactoring.
How?
It is optional, but for complex PRs, please provide information about the design,
architecture, approach, etc.
Summary by CodeRabbit