Repository navigation
[ML] Retry 3rd-party git clones and propagate clone failures - #3233
Merged
Merged
Conversation
|
Pinging @elastic/ml-core (Team:ML) |
|
Hi @edsavage, I've created a changelog YAML for you. |
edsavage
added
v8.19.24
edsavage
added a commit
to edsavage/ml-cpp
that referenced
this pull request
Oct 6, 2026
The retry-with-backoff clone loop added in elastic#3233 was duplicated verbatim in 3rd_party/pull-eigen.cmake and 3rd_party/pull-valijson.cmake. Extract it into ml_clone_git_dependency() in a new cmake/clone_git_dependency.cmake module that both scripts include. The helper lives in its own file rather than cmake/functions.cmake because the pull-*.cmake scripts run in `cmake -P` script mode, where functions.cmake's trailing add_custom_target() calls are invalid; a dedicated module also keeps the change self-contained and backport-clean. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A final failed Valijson clone can leave a directory that incorrectly suppresses cloning during the next configure.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds resilient cloning for header-only dependencies and ensures configuration stops on exhausted failures.
Changes:
- Adds a shared bounded-retry clone helper with backoff.
- Uses the helper for Eigen and Valijson.
- Propagates child-script failures during configuration.
| File | Description |
|---|---|
cmake/clone_git_dependency.cmake |
Implements clone retries and failure reporting. |
3rd_party/pull-eigen.cmake |
Uses the shared helper for Eigen. |
3rd_party/pull-valijson.cmake |
Uses the shared helper for Valijson. |
3rd_party/CMakeLists.txt |
Makes clone-script failures fatal. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+68
to
+70
| if(NOT GIT_RESULT EQUAL 0) | ||
| message(FATAL_ERROR "Failed to clone ${CLONE_NAME} from ${CLONE_URL} after ${CLONE_MAX_ATTEMPTS} attempts: git exited with ${GIT_RESULT}. Check network connectivity, proxy settings, and git availability.") | ||
| endif() |
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed in f7b7d8e. The helper now removes the destination directory before raising FATAL_ERROR on exhausted retries, so a partial checkout can't cause a subsequent configure to skip the clone (which pull-valijson.cmake's existence guard would otherwise do) and fail later on missing headers. This matches the helper's "start from a clean slate on every attempt" behaviour. Verified locally: a seeded partial directory is removed when the fatal error fires.
The Eigen and Valijson sources are cloned at CMake configure time from gitlab.com and github.com respectively. Those hosts occasionally return transient errors (e.g. GitLab "currently unable to handle this request due to load"), and a single failed clone was enough to break an entire CI build, requiring a manual rebuild. Wrap each clone in a bounded retry loop (5 attempts, increasing backoff) that starts from a clean slate on every attempt, so a brief hosting outage no longer fails the build. Also propagate the failure from the outer execute_process() calls that run these scripts. Previously the FATAL_ERROR raised inside the child `cmake -P` process was swallowed: configure logged the error but continued with an empty 3rd_party/eigen, so the failure only surfaced much later as a cryptic "Eigen/Core: No such file or directory" compile error. COMMAND_ERROR_IS_FATAL ANY makes configure stop immediately with the clear message once retries are exhausted, finally delivering the behaviour elastic#3164 intended. Co-authored-by: Cursor <cursoragent@cursor.com>
The retry-with-backoff clone loop added in elastic#3233 was duplicated verbatim in 3rd_party/pull-eigen.cmake and 3rd_party/pull-valijson.cmake. Extract it into ml_clone_git_dependency() in a new cmake/clone_git_dependency.cmake module that both scripts include. The helper lives in its own file rather than cmake/functions.cmake because the pull-*.cmake scripts run in `cmake -P` script mode, where functions.cmake's trailing add_custom_target() calls are invalid; a dedicated module also keeps the change self-contained and backport-clean. Co-authored-by: Cursor <cursoragent@cursor.com>
edsavage
force-pushed
the
ml/retry-3rd-party-git-clone
branch
from
October 6, 2026 22:15
0629022 to
fd33d4e
Compare
…ries If every clone attempt fails, the helper raised FATAL_ERROR without cleaning up the destination. A partial directory left behind would cause a subsequent configure to skip the clone (pull-valijson.cmake guards on directory existence) and fail much later with a cryptic missing-header compile error. Remove the destination before reporting the fatal error so the next configure re-attempts the clone from a clean slate. Co-authored-by: Cursor <cursoragent@cursor.com>
prwhelan
approved these changes
Oct 8, 2026
Contributor
💔 Some backports could not be created
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
Contributor
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
This was referenced Oct 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Eigen and Valijson sources are cloned at CMake configure time from
gitlab.comandgithub.com. Those hosts occasionally return transient errors(e.g. GitLab "currently unable to handle this request due to load"), and a
single failed clone has been enough to break an entire CI build — see the
version-bump builds that failed on 2026-10-06 (#3290, #3291, #3292), each needing
a manual rebuild.
This PR makes the 3rd-party pull steps resilient and fail cleanly:
Retry with backoff. Each clone in
pull-eigen.cmakeandpull-valijson.cmakeis wrapped in a bounded retry loop (5 attempts, increasing backoff), starting from
a clean slate on every attempt (a failed clone can leave a partial directory
behind). A brief hosting outage no longer fails the build.
Propagate the failure. The outer
execute_process()calls in3rd_party/CMakeLists.txtthat run these scripts previously swallowed thechild's
FATAL_ERROR: configure logged the error but continued with an empty3rd_party/eigen, so the failure only surfaced much later as a crypticfatal error: Eigen/Core: No such file or directorycompile error (exactly whatbuild #3292 hit). Adding
COMMAND_ERROR_IS_FATAL ANYmakes configure stopimmediately with the clear message once retries are exhausted — finally
delivering the behaviour [ML]Fail CMake configure if 3rd-party git clone fails #3164 intended.
Test plan
cmake -Pparse-check of both scripts (happy path, clone skipped when sources present)FATAL_ERRORwith non-zero exitCOMMAND_ERROR_IS_FATAL ANYaborts the parent configure when the child script fatalsMade with Cursor