8000 BUG: error compiling on MSVC 141 toolchain · Issue #21346 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: error compiling on MSVC 141 toolchain #21346

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
Closed
matthew-brett opened this issue Apr 15, 2022 · 20 comments · Fixed by #21366
Closed

BUG: error compiling on MSVC 141 toolchain #21346

matthew-brett opened this issue Apr 15, 2022 · 20 comments · Fixed by #21366
Labels
00 - Bug

Comments

@matthew-brett
Copy link
Contributor

Describe the issue:

When compiling current Numpy main (ae13307) on MSVC v141, with pip install -v -v ., I get the following compilation error:

C:\repos\numpy\numpy\core\src\npysort\x86-qsort.dispatch.cpp(651): error C3861: '_knot_mask16': identifier not found

This is with:

Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27045 for x64

If I'm using the 142 toolchain, Numpy compiles to completion:

Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30139 for x64

In both cases:

Python 3.9.6 (tags/v3.9.6:db3ff76, Jun 28 2021, 15:26:21) [MSC v.1929 64 bit (AMD64)] on win32

Reproduce the code example:

See above.

Error message:

See above.

NumPy/Python version information:

See above.
@charris
Copy link
Member
charris commented Apr 15, 2022

@serge-sans-paille Thoughts?

@serge-sans-paille
Copy link
Contributor
serge-sans-paille commented Apr 15, 2022 •
edited
Loading

This intrinsic comes with <immintrin.h> (see https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#expand=4295&text=_knot_mask16&ig_expand=4166) which is unconditionally included in numpy/core/src/npysort/x86-qsort.dispatch.cpp

Looks like an MSVC issue: https://godbolt.org/z/nz63xTv8d

@charris
Copy link
Member
charris commented Apr 15, 2022

Hmm, the 1.41 library is five years old, maybe too early.

@matthew-brett matthew-brett mentioned this issue Apr 16, 2022
Open
@h-vetinari
Copy link
Contributor

Hmm, the 1.41 library is five years old, maybe too early.

OTOH, VS2017 (which comes with VC141) is EOL now; as long as things like matthew-brett/dll_investigation#1 can be solved (which looks like it can be done with an extra flag), I think the cost/benefit of dropping VC141 should be considered, especially vis-à-vis how much dev time gets eaten by bugs like this one.

@rgommers
Copy link
Member

Conda-forge is on 14.1, as are our wheel builds. So we should just fix this right? Now that the problem is identified it shouldn't be too hard I'd think.

A little surprising that we didn't notice this elsewhere yet. Maybe we're not using 14.1 in regular CI anywhere yet, and didn't see it in the weekly numpy-wheels run yet (or ignored it there).

@charris
Copy link
Member
charris commented Apr 18, 2022

didn't see it in the weekly numpy-wheels

I checked that they are all being built and uploaded.

I think the cost/benefit of dropping VC141 should be considered

I agree with the sentiment, the question seems to be how to manage that for conda-forge.

@mattip
Copy link
Member
mattip commented Apr 18, 2022

I don't think scipy is running a CI build against the weekly numpy wheels. The PyPI wheels are still built with visual 2017.

@rgommers
Copy link
Member

I don't think scipy is running a CI build against the weekly numpy wheels

Yes we are, two of them I believe. Although one of them may be structurally timing out. But we've done this in SciPy for years.

@charris
Copy link
Member
charris commented Apr 18, 2022

The PyPI wheels are still built with visual 2017.

The 1.22.3+ wheels are built with vs2019 using the 1.41 toolchain. I'm not clear at this point what that all means :)

@mattip
Copy link
Member
mattip commented Apr 18, 2022

The 1.22.3+ wheels are built with vs2019 using the 1.41 toolchain

Right, so either 2017 or the 1.41 toolchain, but not the 1.42 toolchain. PR 151 goes to quite a bit of effort to make sure numpy-wheels produces wheels using the 1.41 toolchain.

Yes we are, two of them I believe

I see the prerelease_deps_coverage_64bit_blas azure job, which is running scipy + numpy weekly on ubuntu. I couldn't find one doing the same for windows

@mattip mattip mentioned this issue Apr 19, 2022
Closed
2 tasks
@h-vetinari
Copy link
Contributor
h-vetinari commented Apr 19, 2022 •
edited
Loading

I think the cost/benefit of dropping VC141 should be considered

I agree with the sentiment, the question seems to be how to manage that for conda-forge.

My (personal) take is that conda-forge could easily make the move to vc142 - not least because we have complete control over the conda-forge-wide toolchain. The discussion already came up in the context of Azure deprecating VS2017, and all that's missing is someone to champion that change.

I've been holding off on doing that because it's not a fun discussion to have, but it's IMO overdue (e.g. LLVM 14 already moved to require VS2019+, and we have followed suit there as well.) - the only concern was for people using conda-environments to develop on windows, but having VS2017 installed locally. I claim (and would like to be challenged on this) that the set of affected users (who develop on windows using conda, aren't already directly or indirectly** affected by CI providers dropping VS2017, and cannot upgrade to a 100% ABI-compatible, 1:1 drop-in & free compiler) is empty, or very very close to it.

** Based on the aggressive removal of VS2017, many open source projects have already moved away (at the very least unintentionally, as numpy did originally for 1.22) from vc141, and that means bitrot & breakage for those using any such project as a dependency while trying to keep on living with an EOL compiler.

@h-vetinari
Copy link
Contributor
h-vetinari commented Apr 19, 2022 •
edited
Loading

PS. I continue to think that we should have a toolchain-equivalent of NEP29. It's IMO not a productive use of everyone's time to keep rehashing many of the same points when it comes to toolchain upgrades. It think it would be worth the effort to come up with a reasonable policy for the various platforms / scenarios, and then stick to that.

I might even be convinced to write such a NEP, but last time I proposed this, the appetite for it was low.

@matthew-brett matthew-brett mentioned this issue Apr 19, 2022
Merged
@mattip
Copy link
Member
mattip commented Apr 19, 2022

If we choose to require MSVC 142 or greater as a solution to this issue, we should document it in the release notes. The code was introduced in PR #20133 and will be part of the upcoming 1.23 release. This means we require conda-forge to update before the release.

I started to try to work around the missing intrinsic in #21361, but the same CI commands that worked to set the toolchain in numpy-wheels did not work for me in that PR. I will see if I can set up a 141 toolchain locally.

@matthew-brett
Copy link
Contributor Author

@mattip - what didn't work - I'd like to know to make sure we're doing it right. Here's what I use locally:

REM set_vc141.bat file.
REM https://docs.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-170
%comspec% /k "C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Auxiliary\Build\vcvarsall.bat" x64 -vcvars_ver=14.16

@mattip
Copy link
Member
mattip commented Apr 19, 2022

what didn't wor 8000 k

The changes in the PATH environment variable are not carried over to the next azure job step, even though I copied the step from the numpy-wheels azure yaml file.

@matthew-brett
Copy link
Contributor Author

@mattip - does the step lack a #vso[task.prependpath] set?

@charris
Copy link
Member
charris commented Apr 20, 2022 •
edited
Loading

You can also use $env: as long as it is in the same powershell execution block. There are several levels of environment variables in powershell, so it gets complicated. See azure-steps-windows.yml for examples.

EDIT: That is for azure-pipelines, but perhaps for github actions as well.

@matthew-brett
Copy link
Contributor Author

Maybe this is the contrast between Azure workflows and Github actions? Setting paths in Github actions is via environment files - e.g. https://github.com/actions/toolkit/blob/main/docs/commands.md#powershell

@mattip
Copy link
Member
mattip commented Apr 20, 2022

Let's continue the CI disussion in #21361, and leave this issue for the compilation error itself

@mattip mattip mentioned this issue Apr 20, 2022
Merged
@h-vetinari h-vetinari mentioned this issue Apr 22, 2022
Closed
@h-vetinari
Copy link
Contributor

@charris: I agree with the sentiment, the question seems to be how to manage that for conda-forge.

@h-vetinari: My (personal) take is that conda-forge could easily make the move to vc142 - not least because we have complete control over the conda-forge-wide toolchain. The discussion already came up in the context of Azure deprecating VS2017, and all that's missing is someone to champion that change.

I opened conda-forge/conda-forge.github.io#1732 to discuss this on the conda-forge side.

@h-vetinari h-vetinari mentioned this issue Aug 15, 2022
Merged
@h-vetinari h-vetinari mentioned this issue Nov 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4CFC
Assignees
No one assigned
Labels
00 - Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

BUG: fix compilation error for VS 141 and earlier mattip/numpy
6 participants
@matthew-brett @charris @rgommers @mattip @serge-sans-paille @h-vetinari
0