Repository navigation
fix the pyresample for intel issue - #94
Conversation
|
Thanks @Hang-Lei-NOAA - have you tested these with Intel oneAPI (ifort or ifx)? You can create a PR for spack-stack that temporarily points to your spack-packages branch if you want to use the CI runners for spack-stack for testing. |
|
@climbfuji This has been tested within spack-stack (JCSDA/spack-stack#2140) with intel oneapi. |
climbfuji
left a comment
There was a problem hiding this comment.
@Hang-Lei-NOAA @AlexanderRichert-NOAA I wonder if there's a cleaner way to patch the code, or not bother and build it with gcc (if it's a build dependency)?
There's no version specifier in the patch function. Does this really apply cleanly to all versions listed above?
Also, line 74 should probably become a with when decorator just before def patch if the patch stays this way.
Lastly, we need to send the changes we merge here upstream once they are final.
|
1, The patch is the right approach. Forcing GCC isn't clean because py-numexpr is type=("build", "run") — constraining it to %gcc in a mostly-Intel environment creates a mixed-compiler stack that's worse. The patch is invasive but correct. 2, bytes.replace() is a silent no-op when the target string isn't present, so old versions won't break. Older versions may lack the fmaxd/fmind helper functions entirely (added around 2.6), in which case those replace calls just do nothing and there's no Intel build failure for those older code paths anyway. No version specifier needed on the method itself — the @when compiler guards are the right scope. 3, Great suggestion! Now split it into @when("%oneapi") and @when("%intel") decorators that both delegate to _patch_intel(). This is the canonical spack pattern when you need OR across two conditions: two decorators, one helper. |
climbfuji
left a comment
There was a problem hiding this comment.
The patch for py-numexpr is far too invasive and likely not acceptable upstream. What's more, we don't even need it.
First, I established that out of all py-numexpr versions defined here, version 2.10.2 builds with Intel oneAPI. Versions 2.9 and earlier can't be build with Intel oneAPI, because they depend on py-numpy@:1.25 which don't build with Intel oneAPI either.
Version 2.14.2 doesn't build, which is what you are trying to fix. That fix could be much cleaner and contained in a small .patch file.
However, none of this is needed. If you look inside the py-pyresample code, then py-numexpr is used only in one place in _spatial_mp.py, and there is a fallback to numpy if numexpr is not available. Further, numexpr is not constrained by pyresample, means we could use 2.10.2. In the py-resample build scripts, numexpr is only an optional dependency.
My recommendation is to remove the py-numpexpr dependency from py-resample and remove the updates to py-numexpr entirely from this PR.
|
@climbfuji Thanks for your suggestion. I missed it util Alex remind me yesterday. Now, I have revised it according to it, and have it tested in JCSDA/spack-stack#2140 |
climbfuji
left a comment
There was a problem hiding this comment.
Thanks, looks good to me
AlexanderRichert-NOAA
left a comment
There was a problem hiding this comment.
Much cleaner! :)
For JCSDA/spack-stack#1732
(1) pyresample is added into spack spack#6333
(2) Fix the pyresample build issue with intel:
modify py_pyresample/package.py in spack-stack that adds depends_on("c", type="build") and depends_on("cxx", type="build")
modify py_pykdtree/package.py in spack-stack that adds depends_on("c", type="build") and fixes the OpenMP issue for %oneapi (patch -lgomp → -liomp5 or set USE_OMP=0)