8000 BLD: Check Intel compiler when checking complex types in build by lysnikolaou · Pull Request #25044 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BLD: Check Intel compiler when checking complex types in build #25044

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

BLD: Check Intel compiler when checking complex types in build #25044

wants to merge 5 commits into from

Conversation

lysnikolaou
Copy link
Member

Fixes #25034.

@github-actions github-actions bot added the 36 - Build Build related PR label Oct 31, 2023
@lysnikolaou lysnikolaou mentioned this pull request Oct 31, 2023
Closed
seberg
seberg reviewed Nov 1, 2023
View reviewed changes
@lysnikolaou @seberg
BLD: Check Intel compiler when checking complex types in build
0ff966e 
Fixes numpy#25034.

use clearer syntax

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
@seberg seberg force-pushed the check-intel-meson branch from 320cea0 to 0ff966e Compare November 6, 2023 13:43
@seberg
Copy link
Member
seberg commented Nov 6, 2023

Rebased to fix the accidental submodule merge (and squash commits). Since it's a one line change and nobody commented on it, will assume this is a step in the right direction.

Thanks @lysnikolaou.

@seberg seberg mentioned this pull request Nov 6, 2023
Merged
@lysnikolaou
Copy link
Member Author
lysnikolaou commented Nov 6, 2023 •
edited
Loading

I'm not sure about this, but I think I can recall clang-cl using the MSVC types. Should I look into it?

EDIT: Yes, clang-cl CI fails due to this.

@seberg
Copy link
Member
seberg commented Nov 6, 2023

Argg, sorry... Interesting, but that makes me think that more that switching to C11 requires also changing this here in gh-25072.

mattip added 2 commits November 7, 2023 08:21
@mattip
BLD: add needed __INTEL_LLVM_COMPILER check
c9de672
@mattip
< 8000 /div>
BLD: avoid fast-math on intel compilers (taken from scipy)
33849a4 
also add 'std=' flags and avoid a '-utf-8' flag by adding '-source-charset'
@mattip
Copy link
Member
mattip commented Nov 7, 2023

I added a few more changes, especially the one needed for fast math from this comment in the issue and adding a few more std= flags. That together with the workaround for -utf-8 gets me to a pretty clean build with one warning that repeats whenever compiling with cpp and one link-time error:

d:\Intel\oneAPI\compiler\latest\windows\bin-llvm\..\compiler\include\complex.h(37,14): warning: "The /Qstd=c99 compilation option is required to enable C99 support for C programs" [-W#warnings]
            #warning "The /Qstd=c99 compilation option is required to enable C99 support for C programs"
             ^
1 warning generated.
[421/496] Linking target numpy/_core/_multiarray_umath.cp310-win_amd64.pyd
FAILED: numpy/_core/_multiarray_umath.cp310-win_amd64.pyd numpy/_core/_multiarray_umath.cp310-win_amd64.pdb
"xilink.exe" @numpy/_core/_multiarray_umath.cp310-win_amd64.pyd.rsp
libmmd.lib(libmmd.dll) : error LNK2005: ldexpf already defined in meson-generated_arraytypes.c.obj
   Creating library numpy\_core\_multiarray_umath.cp310-win_amd64.lib and object numpy\_core\_multiarray_umath.cp310-win_amd64.exp
numpy\_core\_multiarray_umath.cp310-win_amd64.pyd : fatal error LNK1169: one or more multiply defined symbols found

@mattip
Copy link
Member
mattip commented Nov 7, 2023

Hmm. it seems clang-cl needs the msvc complex definitions, but the intel clang-based compilers need the other ones.

@lysnikolaou
Copy link
Member Author

Hmm. it seems clang-cl needs the msvc complex definitions, but the intel clang-based compilers need the other ones.

Yup, that's right. I think if cc.get_argument_syntax() == 'msvc' and not cc.get_id().startswith('intel') should work in the meson.build file, right?

@mattip
Copy link
Member
mattip commented Nov 7, 2023

This is what we want, right?

get_id() result
msvc True
clang-cl True
intel-llvm-cl False

So `cc.get_id() == 'msvc' or cc.get_id() == 'clang-cl'

@mattip
Copy link
Member
mattip commented Nov 10, 2023

CI is passing

@lysnikolaou
Copy link
Member Author

I don't understand the Intel/fastmath related change well enough to comment on it, but the rest looks good!

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Nov 10, 2023
rgommers
rgommers reviewed Nov 13, 2023
View reviewed changes
Copy link
Member
@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good, thanks Matti and Lysandros. One question in a comment inline. Second question: there are quite a few more __INTEL_COMPILER usages in the code base, did you audit those, or only changed the 3 in this PR in response to build/test failures?

meson.build
_intel_c99_flag = cc.get_supported_arguments('/Qstd=c99')
add_project_arguments(_intel_c99_flag, language: 'c')
_intel_c17 = cpp.get_supported_arguments('/Qstd=c++17')
add_project_arguments(_intel_c17, language: 'cpp')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole if block looks incorrect, because it hardcodes flags that should be set already by the default options higher up. And that may be a problem soon because I already have a PR open to upgrade from C99 to C11. Did you add this to work around a problem with the default flags Meson is adding? If so, let's add a comment and link the upstream issue for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was to solve the same problem as in #25072: hundreds of warnings about needed language standards. I am happy to wait for that PR to land first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, you added the /Qstd=c99 warning higher up. That should already be added, because we have c_std=c99' in default_options at the top of meson.build. If it's indeed not added, can you open a Meson issue for it?

To check if it's added:

cd build
python ../vendored-meson/meson/meson.py introspect --targets -i | grep c99

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On main of course the build fails when checking for complex, which what started this whole exercise in the first place. But if I run the command on the broken build, I do not see any c99. Maybe I need to create a minimal reproducer for a proper meson issue.

@mattip
Copy link
Member
mattip commented Nov 15, 2023

I only added __INTEL_LLVM_COMPILER next to __INTEL_COMPILER where it was required for compilation.

This PR is still incomplete: linking fails. To recap (from a comment in the issue), to test this PR one needs to

  • install the Intel oneAPI windows compiler from the intel download site

  • Set some environment variables:

    <install path>\setvars.bat intel64
set CC=icx.exe
set CXX=icx.exe

  • install packages for the build (standard stuff)

    pip install scipy-openblas64 -r build_requirements.txt -r test_requirements.txt REM use scipy-openblas wheels to provide OpenBLAS pip install scipy-openblas64 set PKG_CONFIG_PATH=<path>\.openblas

  • try to build NumPy with pip install . --no-build-isolation

If we decide this is important, there is an example github repo with a workflow that downloads and caches the compiler.

@rgommers
Copy link
Member

If we decide this is important, there is an example github repo with a workflow that downloads and caches the compiler.

I tried this before in scipy/scipy#16957. It's a real pain to work with. I'm hoping the Intel compilers before available in conda-forge (we had some discussions with Intel engineers around that), then it'll be much easier. I wouldn't bother now.

@charris
Copy link
Member
charris commented Jan 4, 2024

Needs rebase.

@charris
Copy link
Member
charris commented Feb 5, 2024

Be good to finish this.

@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Feb 5, 2024
@lysnikolaou
Copy link
Member Author

@rgommers @mattip I rebased this PR, so that it's easier to be reviewed/merged, in case we want to do so before 2.0.

@mattip
Copy link
Member
mattip commented Mar 4, 2024

There is so much already in 2.0 I didn't want to add more. Also

This PR is still incomplete: linking fails.

and I have not gotten it to work yet

@lysnikolaou
Copy link
Member Author

Got it. I only wanted to remind you all of it, just in case it was missed.

@mattip mattip mentioned this pull request Apr 3, 2024
Closed
@mattip mattip mentioned this pull request Apr 10, 2024
Open
@rgommers
Copy link
Member

@mattip perhaps you could retry with latest Intel Fortran? We have had working CI jobs with those compilers in SciPy for a while now, and gh-25034 notes that an issue with 2023.1.0 is fixed in 2024.2.1

@ashish-2022
Copy link
ashish-2022 commented Nov 25, 2024 •
edited
Loading

With new version of visual studio professional following is the status:
(I used tar.gz source files from PyPI for build)
Numpy=1.26.4
Visual Studio version 17.10.7
intelOneAPI 2024.2.1

Build command: python -m pip install . -Cbuild-dir=build -Csetup-args=-Dallow-noblas=false -Csetup-args=-Dblas-order=mkl -Csetup-args=-Dlapack-order=mkl -Csetup-args=-Dblas=mkl -Csetup-args=-Dlapack=mkl

Selecting Microsoft compilers with MKL PASSES:
set CC=cl
set CXX=cl

set CC=clang-cl
set CXX=clang-cl

Setting intel icx fails with error npy_clongdouble definition is not compatible with C99 complex definition !
set CC=icx
set CXX=icx

By default numpy selected cl compiler + MKL during build with new intelOneAPI. icx is still a problem. same error.
So the Microsoft visual studio LLVM compilers are fixed, and also the default Microsoft visual studio cl compiler works.

For numpy 2.1.3 meson fails to detect icx:

File "D:\Python_Packages\numpy-2.1.3\vendored-meson\meson\mesonbuild\interpreter\interpreter.py", line 1522, in add_languages_for
          comp = compilers.detect_compiler_for(self.environment, lang, for_machine, skip_sanity_check, self.subproject)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "D:\Python_Packages\numpy-2.1.3\vendored-meson\meson\mesonbuild\compilers\detect.py", line 107, in detect_compiler_for
          env.coredata.process_compiler_options(lang, comp, env, subproject)
        File "D:\Python_Packages\numpy-2.1.3\vendored-meson\meson\mesonbuild\coredata.py", line 740, in process_compiler_options
          self.add_compiler_options(comp.get_options(), lang, comp.for_machine, env, subproject)
                                    ^^^^^^^^^^^^^^^^^^
        File "D:\Python_Packages\numpy-2.1.3\vendored-meson\meson\mesonbuild\compilers\c.py", line 547, in get_options
          raise RuntimeError('This is a transitory issue that should not happen. Please report with full backtrace.')
      RuntimeError: This is a transitory issue that should not happen. Please report with full backtrace.
      The Meson build system
      Version: 1.5.2
      Source dir: D:\Python_Packages\numpy-2.1.3
      Build dir: D:\Python_Packages\numpy-2.1.3\build
      Build type: native build
      Project name: NumPy
      Project version: 2.1.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seberg seberg seberg left review comments

@rgommers rgommers rgommers left review comments

@mattip mattip mattip left review comments

Assignees
No one assigned
Labels
36 - Build Build related PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

npy_cdouble related error when compiling 1.26.1 with Intel compilers on Windows
6 participants
@lysnikolaou @seberg @mattip @rgommers @charris @ashish-2022
0