WIP: static libraries handling#3253
WIP: static libraries handling#3253Alexandr-Solovev wants to merge 4 commits intouxlfoundation:rfcsfrom
Conversation
|
@Alexandr-Solovev let's move it to folder named rfcs/20250612-static-libs-handling and name file itself as README.md there. In this case it also would be rendered properly and you could add images and other content related to RFC |
|
Might be elaborate more on several things.
|
| We propose to change the structure of static library handling in oneDAL to address several growing concerns: | ||
|
|
||
| 1. **Remove symbol duplication and overbundling**: | ||
| Eliminate the current `addlib` logic that embeds all symbols from backend libraries (e.g., MKL/OpenBLAS) directly into the oneDAL static archives. |
There was a problem hiding this comment.
@Alexandr-Solovev Is the aim here to also eliminate static linkage of standard (=present in netlib) CPU-based BLAS/LAPACK functions from MKL? That would be very problematic for the PyPI distribution of sklearnex, and would require some changes in the conda distribution to make it work, plus would make this conda distribution a lot less compatible with installed user environments due to interdependencies and bugs that block upgrades.
| ## Open Questions | ||
|
|
||
| - How will this change impact current downstream consumers who rely on bundled behavior? | ||
| - Should we maintain backward compatibility (e.g., via a CMake switch)? |
There was a problem hiding this comment.
Which cmake scripts does this refer to?
There was a problem hiding this comment.
|
|
||
| This approach leads to bundling large static libraries into our own library archives, increasing binary size and leading to symbol duplication or conflicts if the final application also links to the same libraries. | ||
|
|
||
| In addition, the upcoming oneMKL releases will **remove support for static SYCL libraries** (`libmkl_sycl*.a`). To remain compatible with the future oneMKL toolchain and simplify our own build system, we propose to **drop static SYCL support in oneDAL** as well. |
There was a problem hiding this comment.
i think we should separate proposals to make them clear - in fact i don't think there is need for SYCL one as currently it impact no other parties.
The core gist of this proposal is idea of changing way how we handle static cases for oneDAL - instead of addlib math backends - define build time dependency on them
Vika-F
left a comment
Vika-F
left a comment
There was a problem hiding this comment.
Agree with Nikolay that the description of the changes that have to be implemented for this RFC needs to be added (besides the link to the actual PR).
| Simplifies both Bazel and CMake logic by removing fragile `addlib`-style manual bundling. | ||
|
|
||
| * What is the customer impact if we don't do this? | ||
| * Users may face issues with dependencies handling. |
There was a problem hiding this comment.
Examples needed here. Which particular issues would users face?
It would be good to provide the link lines "before" and "after" for some particular configuration, Linux + oneMKL, for example.
|
A different and mutually exclusive RFC: #3290 |
|
|
||
| - How will this change impact current downstream consumers who rely on bundled behavior? | ||
| - Should we maintain backward compatibility (e.g., via a CMake switch)? | ||
| - Should we provide guidance or tooling for users to configure correct BLAS/LAPACK backend linkage? |
There was a problem hiding this comment.
I agree on this point, rather than providing backward compatibility it’s better to provide documentation on how to link.
5 participants
Description
Add a comprehensive description of proposed changes
List associated issue number(s) if exist(s): #6 (for example)
Documentation PR (if needed): #1340 (for example)
Benchmarks PR (if needed): IntelPython/scikit-learn_bench#155 (for example)
PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.
You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance