-
Notifications
You must be signed in to change notification settings - Fork 6
change to dynamic linking and add interface and threading layers #72
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
base: main
Are you sure you want to change the base?
Conversation
69cb600
to
1bfa605
Compare
In general I don't see a problem with this change, but changing a dependency is a pretty big tweak. @ekomarova @AndresGuzman-Ballen is there any way Intel NumPy tests can be run with this branch to make sure it doesn't introduce any major failures? |
@ndgrigorian I suppose I could point mkl_umath's commit in numpy-2.2.5 branch to this branch, but I'd prefer doing that after I can get all subpackages to build and pass tests in our CI |
This PR does not break testing with Intel Numpy 2.2.5. All checks are green. I will also check v1.26.4 |
@ndgrigorian I see some problems in build/tests in numpy 1.26.4. Could you please take a look at it here https://github.com/intel-innersource/libraries.python.intel.condarecipes.numpy-recipe/pull/152? |
@ekomarova To fix build failure you need to pin |
Linux test failure is because of tolerance, I think it should be patched to fix. FAILED core/tests/test_umath.py::TestPower::test_power_float - AssertionError:
Arrays are not almost equal to 7 decimals
unary offset=(0, 0), size=8, dtype=<class 'numpy.float32'>, out of place
Mismatched elements: 1 / 8 (12.5%)
Max absolute difference: 2.3841858e-07
Max relative difference: 9.7333974e-08
x: array([0. , 0.9999999, 1.4142134, 1.7320508, 1.9999999, 2.236068 ,
2.4494896, 2.6457512], dtype=float32)
y: array([0. , 1. , 1.4142135, 1.7320508, 2. , 2.236068 ,
2.4494898, 2.6457512], dtype=float32)
FAILED core/tests/test_umath.py::TestAVXUfuncs::test_avx_based_ufunc - AssertionError:
Arrays are not equal
Mismatched elements: 1 / 2 (50%)
Max absolute difference: 4.7683716e-07
Max relative difference: 6.150229e-08
x: array([2.410054, 7.75316 ], dtype=float32)
y: array([2.410054, 7.75316 ], dtype=float32) |
Rather than tweak the precision, what if the numpy PR just include this? https://github.com/intel-innersource/libraries.python.intel.condarecipes.numpy-recipe/blob/2.2.5/patches/add_precision_flags.patch |
@ekomarova in case you're curious how I came to that conclusion, this is some context: #73 |
Windows failure are not related to this PR, they are related to #73 |
I think numpy 1.26.4 wasn't tested recently with recent mkl_umath changes. I'll push some tweaks so that way the numpy 1.26.4 PR works. |
set(MKL_ARCH "intel64") | ||
set(MKL_LINK "dynamic") | ||
set(MKL_THREADING "tbb_thread") | ||
set(MKL_INTERFACE "ilp64") |
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.
should it be ilp64 vs defailt lp64 used in numpy/scipy?
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.
Or might be to put this differently after reading initial issue - how this would work and what implications we would get because of ILP64? Compatibility with stock numpy? Performance and memory consumption implications because of longer int?
Also how does this works currently in case of stock numpy as they use LP64 by default and not facing reported problem?
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.
I chose ilp64
because MKL_INT
is long long
(8 bytes) from this table while lp64
is int
(4 bytes).
However, I tested with lp64
right now and it works with it as well.
@@ -47,6 +47,7 @@ If these are installed as part of a `oneAPI` installation, the following package | |||
|
|||
If build dependencies are to be installed with Conda, the following packages must be installed from the Intel(R) channel | |||
- `mkl-devel` | |||
- `tbb-devel` | |||
- `dpcpp_linux-64` (or `dpcpp_win-64` for Windows) |
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.
side question - why dpcpp compiler referenced here as there is no sycl us in mkl_umath?
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.
The build instructions have been recommending using the Intel LLVM compiler for some time now—mostly to have a working C compiler, especially one we test with
resolves IntelPython/mkl_fft#165
Main branch
This branch