Skip to content
This repository has been archived by the owner on Aug 30, 2024. It is now read-only.

Please revert to VS2017-compatible build for Numpy #145

Closed
matthew-brett opened this issue Jan 22, 2022 · 26 comments
Closed

Please revert to VS2017-compatible build for Numpy #145

matthew-brett opened this issue Jan 22, 2022 · 26 comments

Comments

@matthew-brett
Copy link
Contributor

Summary

The 1.22 series are using the latest VS2019 toolchain (version 142). This makes the built objects and static libraries incompatible with software built with older toolchains. Would you accept a PR to use the older VS2017 version 141 toolchain instead?

Detail

The Numpy 1.22 series wheels are breaking the Scipy MinGW-w64 builds.

I believe this is because y'all have updated from the previous VS2017 toolchain (version 141) to the latest VS2019 toolchain (version 142).

As MS point out, using version 142 to build the static libraries means that anyone linking against those libraries must also be using (at least) this toolchain.

This means that projects like Scipy can't link to Numpy without also updating to version 142.

In fact, for Scipy, it also, and presumably for similar reasons, breaks our ability to link using MinGW-w64:

matthew-brett/dll_investigation#1

I believe, for these reasons, conda-forge has standardized to version 141 for their builds.

Would y'all consider following conda-forge, and using the 141 toolchain for builds, so that more of us can safely
link to Numpy - particularly - npymath?

@isuruf tells me that this just requires the following lines for a Visual Studio 2019 image:

call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Auxiliary\Build\vcvars64.bat" -vcvars_ver=14.16
set DISTUTILS_USE_SDK=1

@isuruf points out that

Last part is to make sure that distutils doesn't try to override that if you are using distutils.

@rgommers @carlkl

@charris
Copy link
Contributor

charris commented Jan 22, 2022

I'm willing to make that change, but eventually there should be a move to more recent versions. Given the downstream impact, we should plan for that move in advance and come up with a way to coordinate.

I'm curious which static libraries are the problem here, it seems that NumPy 1.22 is not self contained, or at least requires libraries that some environments do not have.

@mattip
Copy link
Contributor

mattip commented Jan 22, 2022

I think this is due to using the "windows-latest" Azure image. The place to inject the call I thing is just before building the wheel somewhere around here

I'm curious which static libraries are the problem here

We ship the numpy/core/lib/npymath.a and numpy/random/lib/npyrandom.a libraries in the wheel, which other projects can link to. They expose symbols like npy_sin.

@charris
Copy link
Contributor

charris commented Jan 22, 2022

@mattip Will a program compiled against an older NumPy still run on a newer NumPy compiled with a different version? That would break forward compatibility if we ever change the version.

@charris
Copy link
Contributor

charris commented Jan 22, 2022

According to https://docs.microsoft.com/en-us/cpp/porting/binary-compat-2015-2017?view=msvc-170 it is the linker that needs to be updated. I don't see how that helps here, as most folks will compile and link with the same toolset.

@isuruf
Copy link
Contributor

isuruf commented Jan 22, 2022

Will a program compiled against an older NumPy still run on a newer NumPy compiled with a different version?

Yes.

@matthew-brett
Copy link
Contributor Author

@charris - for the linker - my reading is that if you are linking with 142 libraries, you need the 142 linker. So that means that anyone who is linking against Numpy - npymath.lib in Scipy's case - is forced to upgrade to the 142 toolchain. Whereas, if Numpy drops back to 141, then anyone using 141 or 142 can link correctly.

@mattip
Copy link
Contributor

mattip commented Jan 23, 2022

On the cibuildwheel-based github action in numpy/numpy we are using windows-2019 so this same issue will come up again in a few months.

I see conda-forge uses the vs2017-win2016 image on azure, which will be retired in March 2022. Is there discussion on moving away from Visual 2017 on conda-forge?

Scipy is using windows-latest on azure for CI testing, and "Visual Studio 2017" on appveyor for building wheels.

Perhaps scipy would also consider updating scipy wheel building (perhaps by copying the azure pipeline workflow from CI and dropping appveyor). Unifying the CI workflow and the wheel-building workflow might prevent some subtle bugs in wheels, like this one.

@matthew-brett
Copy link
Contributor Author

Yes - the recipe that @isuruf gave above is for activating the v141 builds on the windows-2019 image. Windows, like Mac, can build for compatibility with earlier toolchains, from up-to-date installs of Visual Studio. So, I don't think there's an immediate threat of being unable to use v141 on modern images.

I'm sure Scipy will update its workflow - but the particular problem here is that we (Scipy) are using Mingw-w64 - and that also runs into trouble linking v142 flavor libraries.

@carlkl
Copy link

carlkl commented Jan 23, 2022

Maybe - this has to be tested - using the flag -d2VolatileMetadata- during numpy build with v142 could help to avoid the inclusion of unwanted .voltbl sections into the static libraries 'npymath.lib' and potentially 'npyrandom.lib'.

Using mingw-w64 for scipy build means that binutils ld linker has to be used for linking. This linker cannot handle these associative sections (.voltbl). Using -d2VolatileMetadata- was proposed here: matthew-brett/dll_investigation#1 (comment) and relies on ocaml/ocaml@0ac7358.

Another approach in the long run (see matthew-brett/dll_investigation#1 (comment)) is to expose npymath.lib and npyrandom.lib in addition as DLLs to be located in numpy's .libs folder. For this case scipy and other 3rd party could link against the DLLs instead of the import libraries.

@rgommers
Copy link
Contributor

Another approach in the long run (see matthew-brett/dll_investigation#1 (comment)) is to expose npymath.lib and npyrandom.lib in addition as DLLs to be located in numpy's .libs folder. For this case scipy and other 3rd party could link against the DLLs instead of the import libraries.

Small tweak on that - they should probably be packaged into the .data directory in a wheel installed into libdir (e.g., $prefix/lib).

This does seem like a good way to go though to me, since shared libraries are more portable than static ones and should prevent trouble like what we're seeing now when trying to build with Mingw.

One thing I don't know is what the reasons were to provide npymath as a static library. I suspect it's because there was no packaging solution back then to reliably provide a shared library, but maybe there were other reasons? @charris or @rkern may remember?

@matthew-brett
Copy link
Contributor Author

Can I propose a new issue to discuss npymath and possible solutions? I still think that it is better, for now, to stick to the 141 toolchain, for the same reasons that conda-forge are doing it. Whether that makes a big difference will depend on whether anyone else is linking to npymath.

@charris
Copy link
Contributor

charris commented Jan 23, 2022

@charris or @rkern may remember

I don't recall. The original decision was npymath and that was over ten years ago when Windows wasn't much used. Later libraries probably followed existing practice. Npysort was for convenience, I don't think it was made public, although that was a possibility.

Scipy is using windows-latest on azure for CI testing, and "Visual Studio 2017" on appveyor for building wheels.

I think the appveyor build uses 1.40 with VS2017.

@matthew-brett
Copy link
Contributor Author

To unpack my reasoning on going back to v141 now, even if, in due course, we refactor npymath somehow - we'd like to be able to rely on past Numpy wheels to build against. If y'all revert to 141 for the next Numpy release, this will mean that people using 141 can build against nearly all Numpy versions (except 1.22.0 and 1.22.1). The ideal would be if y'all are prepared to release build-tag releases of Numpy 1.22.0 and 1.22.1, using v141 - in which case this will continue to be a non-issue for builders into the medium term. I'm happy to do the work there, if y'all agree.

@matthew-brett
Copy link
Contributor Author

@isuruf - can you say something about the discussions about this on Conda-forge?

@isuruf
Copy link
Contributor

isuruf commented Jan 23, 2022

At conda-forge we are moving to windows-2019 image and using VS2019 to activate v141 toolchain.

See https://github.com/conda-forge/numpy-feedstock/blob/master/.azure-pipelines/azure-pipelines-win.yml#L8

@mattip
Copy link
Contributor

mattip commented Jan 23, 2022

So it seems we should do the same here, and in the cibuildwheel numpy/numpy github action

@charris
Copy link
Contributor

charris commented Jan 23, 2022

So it seems we should do the same here

That seems the best short term solution. I wonder what the downstream problems of changing to DLL's would be if we choose that route in the long term.

@carlkl
Copy link

carlkl commented Jan 23, 2022

That seems the best short term solution. I wonder what the downstream problems of changing to DLL's would be if we choose that route in the long term.

Of course one could think about deploying static as well as shared libraries then.

@matthew-brett
Copy link
Contributor Author

Issue to discuss future of Npymath : numpy/numpy#20880

@charris
Copy link
Contributor

charris commented Jan 23, 2022

@matthew-brett Go ahead and make that PR.

@rgommers
Copy link
Contributor

rgommers commented Aug 5, 2022

A comment on this: 1.21.6 was also built with vc142, 1.21.4 with vc141. There's no commit for 1.21.5 in this repo as far as I can tell, not sure what happened there.

@charris
Copy link
Contributor

charris commented Aug 5, 2022

not sure what happened there.

Dec 19 2021 in the maintenance branch. The commit was mislabeled here, cut-and-paste error.

@rgommers
Copy link
Contributor

rgommers commented Aug 5, 2022

Ah thanks, makes sense. 1.21.5 is built with vc141; the changeover happened after that in commit b275730.

@charris
Copy link
Contributor

charris commented Aug 5, 2022

changeover happened after that in commit b275730

Yes, and then vs2019 was made to use 14.1 in the commit after that. This is all getting a bit hazy ...

@rgommers
Copy link
Contributor

rgommers commented Aug 5, 2022

I know, we probably just ran into it when moving SciPy to cibuildwheel, that's why I wanted to document it here. Perhaps not the last time someone needs to know what exact NumPy versions to avoid.

@carlkl
Copy link

carlkl commented Aug 5, 2022

See my comments here: numpy/numpy#20880 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants