fix(lance-linalg): check fp16kernels feature before arch-specific code #5747
+8
−1
Conversation
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
Contributor
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
The build script was checking target architecture BEFORE verifying if the fp16kernels feature was enabled. This caused builds to fail on platforms like iOS/Android when falling through to the error branch, even when the feature was disabled. The fix adds an early exit using CARGO_FEATURE_FP16KERNELS env var check (not cfg!() which checks build script features, not library features). Fixes: lance-format#5746
durch
force-pushed
the
fix-ios-fp16kernels-build-v1.0.1
branch
from
1e02163 to
a7a6397
Compare
durch
changed the title
Address review feedback: instead of early exit which skips build_dist_table_with_flags on x86_64, only error in the else branch when fp16kernels is explicitly requested on unsupported platforms. This preserves dist_table kernel builds on supported platforms while still allowing iOS/Android builds when fp16kernels is disabled.
Author
@wjones127, thanks for the speedy review, this should do it |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 build script was checking target architecture BEFORE verifying if the
fp16kernelsfeature was enabled. This caused builds to fail on platforms like iOS/Android when falling through to the error branch, even when the feature was disabled.The Problem
On line 78-79 of
rust/lance-linalg/build.rs:This error is returned for any target_arch not in the supported list (x86_64, aarch64 macos/linux, loongarch64), before the feature check in
build_f16_with_flags()is ever called.Additionally,
cfg!(not(feature = "fp16kernels"))insidebuild_f16_with_flags()checks the build script's features, not the library's features (build scripts are compiled for the host, not the target).The Fix
Add an early exit using
CARGO_FEATURE_FP16KERNELSenv var check (which correctly reflects the library's features, not the build script's) before the target_arch branching logic.Testing
Verified fix enables successful iOS builds with:
fp16kernelsfeature disabled (default)Fixes: #5746