Repository navigation
chore(build): make the MLX pin a single source of truth so a partial bump cannot silently build the old MLX #1047
Description
Problem / Background
The pinned MLX commit is written in two places that must agree, with no check that they do. Updating only one of them produces a build that looks bumped but compiles the old MLX, with no error and no warning.
The two locations, both currently 2c46b953db88965c4270cc7306eda6887a3247f2:
src/lib/mlx-cpp/CMakeLists.txt:95, theGIT_TAGpassed toFetchContent_Declare(mlx ...). This decides what gets fetched.src/lib/mlxcel-core/build.rs:217,const MLX_EXPECTED_COMMIT. This drivespurge_stale_mlx_cacheandmark_mlx_cache_valid, and is also exported to the crate asMLXCEL_MLX_COMMITviacargo:rustc-env(build.rs:26).
The doc comment on the constant already states the coupling ("must match GIT_TAG in mlx-cpp/CMakeLists.txt") but nothing enforces it.
The failure mode, concretely
CMakeLists.txt prefers an existing cached checkout over the declared tag:
if(EXISTS "${_mlx_cached_source_dir}/CMakeLists.txt")
set(mlx_SOURCE_DIR "${_mlx_cached_source_dir}")
set(mlx_BINARY_DIR "${_mlx_cached_binary_dir}")
message(STATUS "Using cached MLX source from ${mlx_SOURCE_DIR}")
else()
FetchContent_Declare(
mlx
GIT_REPOSITORY "https://github.com/ml-explore/mlx.git"
GIT_TAG 2c46b953db88965c4270cc7306eda6887a3247f2)The comment above that block says so explicitly: "Stale-cache validation is handled by build.rs (purge_stale_mlx_cache) which runs before CMake, ensuring _deps/ is either valid or absent." So cache invalidation is delegated entirely to build.rs, which compares the _deps/.mlx-build-commit marker against MLX_EXPECTED_COMMIT and returns early when they match.
If a contributor bumps GIT_TAG alone, the marker still equals MLX_EXPECTED_COMMIT, _deps/ is judged valid and never purged, FetchContent is never re-run, and the build compiles the previous MLX. Nothing surfaces the mismatch: the tag in the repo says one commit, the linked library is another, and MLXCEL_MLX_COMMIT reports the stale one.
This was hit during the bump in #1042 / PR #1046 and caught only because the fetched _deps/mlx-src HEAD was checked by hand. It is the same class of silent-no-op that made the broken RMSNorm overlay in #824 / #829 survive as long as it did: the build appeared to do what was asked.
Proposed Solution
In preference order:
- Make
build.rsparseGIT_TAGout ofsrc/lib/mlx-cpp/CMakeLists.txtat build time and use that value, deletingMLX_EXPECTED_COMMITas an independent constant. One place to edit, no possible divergence. - Keep both but add a build-time assert in
build.rsthat reads theGIT_TAGline and fails the build with a clear message when it disagrees withMLX_EXPECTED_COMMIT. Cheap and fully removes the silent mode, though it still requires two edits. - At minimum, after a successful build, verify the fetched
_deps/mlx-srcHEAD equals the expected commit and fail if not. This also catches a cache that was restored or seeded out of band, which options 1 and 2 do not.
Options 1 and 3 are complementary: 1 removes the divergence, 3 verifies what actually landed on disk. Consider both.
Already in place, no action needed
cargo:rerun-if-changed=../mlx-cpp/CMakeLists.txt is already present at src/lib/mlxcel-core/build.rs:181, so editing the tag does re-run the build script. Under option 1 the parsed value would then flow through correctly on that re-run. This is listed only so the implementer does not add a duplicate.
Acceptance Criteria
- Editing the MLX pin in exactly one place produces a correct build.
- Verified by bumping to a different commit and confirming the fetched
_deps/mlx-srcHEAD, the_deps/.mlx-build-commitmarker, and the binary's embeddedMLXCEL_MLX_COMMITall move together. - A deliberately mismatched pin fails the build with an actionable message instead of silently compiling the old MLX.
- No duplicate
rerun-if-changedline is introduced for../mlx-cpp/CMakeLists.txt.
Technical Considerations
purge_stale_mlx_cache runs from build_mlx() before Config::new("../mlx-cpp"), and mark_mlx_cache_valid runs after the CMake build returns, so any parsed-tag value must be resolved before the purge decision, not after. Both the purge comparison and the cargo:rustc-env export must consume the same resolved value, otherwise the fix reintroduces the divergence one level down.
Activity
docs(ci): document the unconditional workspace _deps sweep in release…
chore(ci): gate the MLX pin extraction script at PR time
docs(ci): correct the workspace member count in nightly-verify
fix(build): close two ways an unverified MLX tree could pass the pin …
fix(build): close review gaps in the MLX pin single source of truth
chore(ci): gate workspace crate versions against the root (#1056)
chore(build): make the MLX pin a single source of truth (lablup#1051)
chore(build): make the MLX pin a single source of truth (lablup#1051)
chore(ci): gate workspace crate versions against the root (lablup#1056)