-
Notifications
You must be signed in to change notification settings - Fork 529
fix(lance-linalg): check fp16kernels feature before arch-specific code #5747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(lance-linalg): check fp16kernels feature before arch-specific code #5747
Conversation
|
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
1e02163 to
a7a6397
Compare
wjones127
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@durch Thanks for creating a PR.
I think we still want to execute build_dist_table_with_flags if fp16kernels is not enabled, but this short-circuits before that. Could you address that?
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.
@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! |
wjones127
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Thanks!
lance-format#5747) ## Summary 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 Problem On line 78-79 of `rust/lance-linalg/build.rs`: ```rust } else { return Err("Unable to build f16 kernels on given target_arch. Please use x86_64 or aarch64 or remove the fp16kernels feature".to_string()); } ``` 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"))` inside `build_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_FP16KERNELS` env 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: - lancedb v0.23.1 - Tauri 2.x iOS target - `fp16kernels` feature disabled (default) Fixes: lance-format#5746
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