Skip to content
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

WIP: try building 1.22.0rc's #248

Closed
wants to merge 9 commits into from
Closed

Conversation

h-vetinari
Copy link
Member

Hot off the press: 1.22.0rc1 - let's try building it. 🙃

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari
Copy link
Member Author

Note that this is currently dropping pypy 3.8, cf. conda-forge/conda-forge-pinning-feedstock#2089

@h-vetinari
Copy link
Member Author

RuntimeError: Broken toolchain: cannot link a simple C++ program. note: A compiler with support for C++11 language features is required.

Now with C++ :)

@h-vetinari
Copy link
Member Author

h-vetinari commented Nov 23, 2021

windows:

  numpy\core\src\npymath\npy_math_internal.h.src(894): error C3861: '__popcnt16': identifier not found
  numpy\core\src\npymath\npy_math_internal.h.src(890): error C3861: '__popcnt': identifier not found
  numpy\core\src\npymath\npy_math_internal.h.src(890): error C3861: '__popcnt': identifier not found
  numpy\core\src\npymath\npy_math_internal.h.src(900): error C3861: '__popcnt64': identifier not found

@h-vetinari
Copy link
Member Author

So it looks like the include-guards of numpy/numpy#19355 don't fully work on windows; I commented upstream.

Rest looks fine (aside from the absence of PyPy).

@h-vetinari
Copy link
Member Author

Rest looks fine (aside from the absence of PyPy).

Ah, well, actually...

=========================== short test summary info ============================
ERROR core/tests/test_mem_policy.py::test_set_policy - RuntimeError: could no...
ERROR core/tests/test_mem_policy.py::test_default_policy_singleton - RuntimeE...
ERROR core/tests/test_mem_policy.py::test_policy_propagation - RuntimeError: ...
ERROR core/tests/test_mem_policy.py::test_context_locality - RuntimeError: co...
ERROR core/tests/test_mem_policy.py::test_thread_locality - RuntimeError: cou...
ERROR core/tests/test_mem_policy.py::test_switch_owner[0] - RuntimeError: cou...
ERROR core/tests/test_mem_policy.py::test_switch_owner[1] - RuntimeError: cou...
ERROR core/tests/test_mem_policy.py::test_switch_owner[None] - RuntimeError: ...
ERROR core/tests/test_mem_policy.py::test_owner_is_base - RuntimeError: could...
= 17308 passed, 813 skipped, 1293 deselected, 19 xfailed, 6 xpassed, 9 errors in 180.78s (0:03:00) =

This yields a passing run; it's"most likely one more of those "actually-needs-a-sys.exit" kind of things...

@h-vetinari
Copy link
Member Author

Ah, and PPC-through-QEMU being its usual charming self,

= 190 failed, 16214 passed, 893 skipped, 1294 deselected, 19 xfailed, 7 xpassed, 104 warnings, 9 errors in 1398.17s (0:23:18) =

with examples such as this one:

E       AssertionError:
E       Arrays are not almost equal to 6 decimals
E        ACTUAL: array([[[[[0.+0.j, 0.+0.j, 0.+0.j],
E                 [0.+0.j, 0.+0.j, 0.+0.j],
E                 [0.+0.j, 0.+0.j, 0.+0.j]],...
E        DESIRED: array([[[[[1., 0., 0.],
E                 [0., 1., 0.],
E                 [0., 0., 1.]],...

This has happened a lot on the scipy feedstock (where it was discussed in depth), as well as the ones of scikit-learn and others, and the only reasonable option is not testing PPC through QEMU (or not building PPC, which was even less popular).

@h-vetinari
Copy link
Member Author

h-vetinari commented Nov 24, 2021

Well, it looks like windows failure of not finding __popcnt are due to vs2017 - with vs2019, things build fine. Given that azure has deprecated vs2017 (and therefore upstream is using vs2019 in their own CI), I doubt that there'll be a way** around bumping to vs2019 here as well...

** with feasible effort and an acceptable maintenance burden

@h-vetinari
Copy link
Member Author

h-vetinari commented Nov 24, 2021

@conda-forge/numpy @rgommers @charris

Just a summary of the findings here (most important is last):

  • numpy has added a new submodule with 1.22 (no issues)
  • build now needs C++ compiler
  • tests were running into the "missing sys.exit-wrapper problem, and misreporting failures as success" - that's on me, mea culpa!
  • this revealed that linux tests need a C compiler since a while (also no issues there)
    • PPC tests fails spectacularly in emulation (as for scipy, scikit-learn, etc.), and have been deactivated
    • similar issue due to emulation as on the scipy feedstock, I have a PR to fix it.
    • one other emulation problem that I think is best to skip
  • There are two failures on linux64 without AVX512 and with CentOS 6: BUG: Test failures with 1.22.0rc1 on non-AVX512 hardware (with old glibc) numpy/numpy#20448
    • Previously, there was little appetite upstream to fix issues for such ancient glibc, but I'll probably try to add a skip upstream like the last time (and we could always skip the tests here of course); OTOH I'd also be happy to move to CentOS 7 (Centos 6 is now EOL for over a year).

Finally, the most actionable point:

  • Windows fails to build with vs2017, but builds with vs2019, see comment above
    • upstream moved to vs2019 due to the deprecation of vs2017 within azure, and I don't think it makes sense to try to bridge this gap, i.e. we need to move to vs2019 here (and I'm aware of the potential knock-on effects if we don't manage the toolchain version, though I think even that would be manageable)

@rgommers
Copy link
Contributor

Asking NumPy to fix up the __popcnt include guards seems quite reasonable.

@h-vetinari
Copy link
Member Author

Asking NumPy to fix up the __popcnt include guards seems quite reasonable.

While I try to keep numpy/scipy cos6 compatible, I feel differently about VS. There I think we should just move on to vs2019 (including the vc142 toolchain), as the effective lower bound is defined by the upstream CI, and I don't think it's reasonable for the feedstock to try to close such gaps that will continue to appear.

Having such native support would also mean that __popcnt compiles down to hardware instructions, rather than go through some fallback code.

@isuruf disagrees about the toolchain bit, but I don't know how to set the toolchain version to vc141, and am not particularly motivated to find out.

@rgommers
Copy link
Contributor

Cc @charris for the question of whether we intentionally dropped VS 2017 in NumPy and are fine keeping it that way.

@charris
Copy link

charris commented Nov 28, 2021

@rgommers The reason for moving to vs2019 was simply that azure will drop vs2017 support next March. I don't know if this causes problems with released Python, but assume that Microsoft tries to keep the vc14.x toolchain series backwards compatible. In general, The MS compilers have lagged behind others in standards compliance and I am happy to go with newer compilers if they work. @h-vetinari is in a better position than I am to make that decision.

@h-vetinari
Copy link
Member Author

@h-vetinari: I think we should just move on to vs2019 (including the vc142 toolchain), as the effective lower bound is defined by the upstream CI, and I don't think it's reasonable for the feedstock to try to close such gaps that will continue to appear.
[...]
@isuruf disagrees about the toolchain bit, but I don't know how to set the toolchain version to vc141, and am not particularly motivated to find out.

Well, I did have a look anyway, and it looks like we need to set this as an argument to vcvarsall.bat, which seems to be set in the vc-activation already. That is a bit outside my league/knowledge though, so I'll have to defer to @conda-forge/vc on that.

@h-vetinari
Copy link
Member Author

@conda-forge/vc @conda-forge/core
Building numpy 1.22 needs VS2019**; I tried looking at @isuruf's suggestion to set the toolset to vc141 (see comment above), but it seems like this is already being done in the activation of the vc-feedstock, and I'm not sure about touching that (or how). I also don't know if things would actually still compile with the old toolset.

In any case, I'm not planning to spend more time chasing old ghosts (especially as upstream now has VS2019 as a lower bound in CI), so I wanted to notify people well in advance that unless there are further comments, I'm going to bump to VS2019 (and implicitly vc142) once numpy 1.22 final drops.

** compile errors with VS2017, I haven't dug further.

@h-vetinari
Copy link
Member Author

h-vetinari commented Dec 7, 2021

Sorry. That's not a decision that one maintainer can take on their own. This needs to be co-ordinated with the community.

Absolutely, which is why I've been trying to involve/inform people. All I said was that if there had been no response for weeks, I would have taken it as tacit consent (and it's not like it wouldn't be possible to bump the build number for a later fix).

@isuruf
Copy link
Member

isuruf commented Dec 7, 2021

What do you mean no response? I mentioned that we should use VS2019 with v141 toolchain in conda-forge/conda-forge.github.io#1538

@h-vetinari
Copy link
Member Author

See my last two comments.

@rgommers
Copy link
Contributor

rgommers commented Dec 7, 2021

What do you mean no response? I mentioned that we should use VS2019 with v141 toolchain in conda-forge/conda-forge.github.io#1538

Can we rephrase this as a request for help, making this comment concrete: "We just need to make sure that the conda compiler activation script works. The effort needed is probably less than the time we have spent on writing on this issue."? It sounds easy, but I admit I have no idea what this actually would take, and apparently nor does @h-vetinari. Right now this isn't urgent yet, but it'd be great if we could build the final 1.22.0 release (I think in a week or two) in your preferred way @isuruf.

@isuruf
Copy link
Member

isuruf commented Dec 7, 2021

See conda-forge/conda-smithy#1552. That should use VS2019 with v141 toolchain.

@h-vetinari
Copy link
Member Author

See conda-forge/conda-smithy#1552. That should use VS2019 with v141 toolchain.

Thanks a lot! I would have gone about this completely the wrong way (had no idea that vs2019 already uses vc141? Apparently I can't deciper the activate.bat in the vc-feedstock, because it still looks to me that it uses vc142)...

@h-vetinari
Copy link
Member Author

That should use VS2019 with v141 toolchain.

@isuruf, this is still using VS2017 despite the newest conda-smithy. Not sure if I'm misunderstanding something or need to add something here, but it could be that it's not yet working as intended? Probably needs an update to the pinning-feedstock?

@isuruf
Copy link
Member

isuruf commented Dec 12, 2021

No, it's not. It's using Visual studio 2019. If you look at the logs you'll see C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.16.27023\include

@h-vetinari
Copy link
Member Author

Hm, I was looking at 04c68e7, where I saw:

 channel_targets:
 - conda-forge main
+cxx_compiler:
+- vs2017
 libblas:
 - 3.8 *netlib

which was what I responded to.

If indeed everything is working as intended, then we have the bigger problem that vc141 is not capable of building numpy, currently. Someone can fix up the include guards, but I fear such issues are going to keep happening (with upstream CI at VS2019@vc142), and I don't have the time/energy to fix them**, but of course I won't stand in the way of someone else doing the work.

** especially as there has been no counter-argument to my impression that the fall-out cost from switching to vc142 would be miniscule to non-existent; without a plausible reason that justifies the cost, it just feels like a massive waste of my time to try to out-support Microsoft.

@isuruf
Copy link
Member

isuruf commented Dec 12, 2021

Sure. I don't expect you to work on that. However we will have to delay the release of 1.22.0 until someone comes up with a patch or until someone champions the move to vs2019 which means getting approval from core to move, announcing the move and giving time for users and then doing the switch itself.

@h-vetinari
Copy link
Member Author

h-vetinari commented Jan 1, 2022

[...] However we will have to delay the release of 1.22.0 until someone comes up with a patch or until someone champions the move to vs2019 which means getting approval from core to move, announcing the move and giving time for users and then doing the switch itself.

@conda-forge/core
Numpy 1.22 was now released (PR) - how do we want to proceed? I think an indefinite hold is not really desirable (maybe release without windows?). I've refrained from championing a move to vs2019 (even though I made a case for it) because doing something similar for cos7 has markedly cooled the interaction with other members and I don't want to repeat that experience.

@h-vetinari
Copy link
Member Author

fixed by #251

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

Successfully merging this pull request may close these issues.

5 participants