-
-
Notifications
You must be signed in to change notification settings - Fork 48
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
Manual migration for PyPy3.7 on windows #238
Conversation
…nda-forge-pinning 2021.06.30.05.52.22
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 ( |
@mattip, this seems to crash in |
bcceb53
to
1ea97a2
Compare
Some good news - all that's separating us from a working PyPy build on windows is that one segfault in
and the hypothesis error can probably be ignored (or might just go away by itself): Detailed log on the remaining failures
|
Thanks for the heads up. I will try to figure out what is broken. |
It looks like the other windows runs are also not happy, the failures seem a bit random. |
Cool, thanks a lot!
Should be fixed by conda-forge/conda-forge-ci-setup-feedstock#156, hopefully |
Just recording another failure here (pypy+linux+ppc) that I haven't seen before:
|
Here is a minimal reproducer of the segfault:
Now to dive in and figure out what is wrong, and why it succeeds on linux |
@mattip, this ran with the new pypy build, but unfortunately still segfaults in that test...
|
So, another option would be to merge this with |
I think releasing a package that crashes in tests is risky. Could we try to require openblas instead of mkl? Maybe there are some latent bugs in mkl on windows that pypy is exposing |
There were no crashes, just the corner case failures of empty arrays we had talked about. Considering that pypy on windows (in conda-forge) has barely taken off yet, I think it would be likely be more useful to continue the migration and actually get to a point where a large(r) number of windows users can try it out - knowing that they're early adopters (most casual users don't know about pypy, from my experience). Which is not to say that the errors shouldn't be fixed, of course. We can try using openblas in CI here, but MKL will still be the default on windows, so we wouldn't really be changing anything. |
I'm confused. #238 (comment) says there's a segfault. |
Nevermind, I should stop working this late. 🤦 My brain had already filed away the segfault as solved, but it isn't, of course. Apologies. Regarding MKL, we can try if their most recent version fixes the issue as soon as conda-forge/blas-feedstock#74 is merged (we could pull this forward - blis & MKL are ready - otherwise we're waiting for some openblas issues around the 0.3.16 release to be resolved first). |
I don't see anything in the mkl 2021.3 bugfixes that catches my eye. I ask about mkl because when I build locally on a avx512-enbled machine with openblas I do not get a segfault, when I build via the recipe I do get a segfault. Although now that I can reproduce the segfault, perhaps I can try to instrument the code to find exactly where the full_args struct is getting zeroed out. |
Is there a way to discover which compiler built the MKL package? |
@h-vetinari could you restart CI here to pick up the latest pypy fix? |
Sure, no problem (this weekend I just have very spotty availability). There's also a magic incantation that you can use on feedstock PRs without having to be maintainer, which is: "@ conda-forge-admin, please restart ci" [remove space after "@"]. |
@mattip, good news, the new pypy build seems to have done the trick. 🥳 |
This also allows to get rid of extra call to show SIMD features
yay |
@conda-forge/numpy, this should now be ready (thanks to the tireless help of @mattip and others)! As mentioned previously, I'm suggesting to add myself as a maintainer. Since the current migrator assumes assumes a 1.19 version of numpy also for pypy-on-windows, I'd offer to try backporting this. My plan would be to create a branch Assuming no opposition, I would proceed along those lines once this PR is merged. |
According to oldest-supported-numpy the oldest numpy supported on pypy3.7 is numpy 1.20. I wouldn't spend time on trying to support 1.19 with pypy3.7 on any platform. |
oldest-supported-numpy is not the reference for this. It comes from the global pinning feedstock (with the migrators layered on top), and you can see in the naming of the CI jobs that pypy3.7 (non-win) runs on numpy 1.19. In fact, since that field was not changed for the windows migrator, all open PRs waiting for numpy would need to be adapted (and the migrator updated). That's still the Plan B, but if a build on 1.19 ends up running through, that might even be the path of least effort. |
I think it's easier to just bump to 1.20 on pypy3.7 windows. |
I don't have a strong preference TBH (well, least amount of work, actually 😅), but whether 1.19 or 1.20, both need a backport. I have the branch for 1.19 ready and could try it out. If it doesn't work, redo for 1.20 - sounds reasonable? |
Ah, I didn't realize we were on 1.21. I thought master was 1.20. |
@isuruf, could you please push the button (unless we'd like to wait for more feedback)? 🙃 |
🎉
You already did in this PR, but for completeness: +1 from me. Thanks for all the work on this! |
Like for pypy37, the migrator seems to have forgotten about numpy - it's nowhere to be found on the status page of the migration. Compare this comment from the pypy37 migration:
Therefore, let's do it manually to progress on the migration.