8000 DEV, BUILD: add pypy3 to azure CI by mattip · Pull Request #12594 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

DEV, BUILD: add pypy3 to azure CI #12594

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

Merged
merged 4 commits into from
Apr 22, 2019
Merged

Conversation

mattip
Copy link
Member
@mattip mattip commented Dec 19, 2018

This PR adds testing for PyPy to our CI. I chose to use the latest nightly rather than a stable release since PyPy will be fixing test failures between releases.

We should also add a benchmark run at some point.

Things that are known not to work on PyPY

I have forced the test script to return 0 so CI does not fail even though there are failing tests

Edit: PyPy latests nightly HEAD passes all tests except the ones that modify docstrings in C

@mattip
Copy link
Member Author
mattip commented Dec 19, 2018

The azure pipeline succeeded, but tests failed. This is the expected outcome, at least until PyPy passes 100% of the tests for a while.

@mhvk
Copy link
Contributor
mhvk commented Dec 20, 2018

Was the plan to merge this now, or just to have a place where we a quick restart shows whether there are problems?

If the plan is to merge, I think this should not run on every PR - that is just a waste of electricity as long as there are known failures.

@charris
Copy link
Member
charris commented Dec 20, 2018

Shouldn't these test be run on the PyPy side?

@mhvk
Copy link
Contributor
mhvk commented Dec 20, 2018

Shouldn't these test be run on the PyPy side?

Once pypy is working, I think it is not unreasonable to run a test here, so that we don't accidentally break it.

@charris
Copy link
Member
charris commented Dec 20, 2018

@mattip I'm happy if NumPy is made more PyPy compatible, but I don't see PyPy as a blocker for anything. CPython is our primary platform.

@rgommers
Copy link
Member

@mattip I'm happy if NumPy is made more PyPy compatible, but I don't see PyPy as a blocker for anything. CPython is our primary platform.

Well, I think it would be useful to add to our CI and keep it green. We can add @skipifs for anything that is a PyPy problem. If we see new breakages, they could well be due to bugs in our usage of Python's C API.

Disclaimer: SciPy's PyPy CI has been fairly consistently red which is annoying.

@shoyer
Copy link
Member
shoyer commented Dec 22, 2018

Well, I think it would be useful to add to our CI and keep it green. We can add @skipifs for anything that is a PyPy problem. If we see new breakages, they could well be due to bugs in our usage of Python's C API.

Yes, this is my preferred approach. @pytest.mark.xfail is probably a better option than skipif, for cases where we are skipping genuine bugs.

@tylerjereddy
Copy link
Contributor

I'm +1 on this. I thought there was still some interest in "swappable back-ends" in the ecosystem too, so staying in touch with the JIT community via some measure of cross-testing seems like a good idea. The electrical footprint of becoming completely incompatible with the JIT community & later trying to remedy a massive divergence could also be quite high.

That said, I do have to agree that the maintenance burden recently observed for PyPy with SciPy CI has been a little scarier recently.

@rgommers
Copy link
Member

Re CI issues, codecov is red yet again. This passed my annoyance threshold a long time ago, I'm starting to favor nuking it completely. Do we want to have one more go at fixing it?

@tylerjereddy
Copy link
Contributor

I'm starting to favor nuking it completely

-1 on my end for removing codecov until there's a viable replacement in the ecosystem; there's a PR open that demonstrates it can be green if we use the SciPy config verbatim as Pauli suggested -- see #12549, but that won't alert us to PRs that have poor test coverage in any way.

If there's a strong preference among devs to have the CI green & expect reviewers to manually check coverage vs. CI often questionably red but more explicitly reporting patch diff %, perhaps this should be expressed clearly in that PR & I'll clean it up / it can be merged.

@shoyer
Copy link
Member
shoyer commented Dec 30, 2018

I am also -1 on removing codecov entirely unless we find an alternative. It's an important check to have easy access to.

@mhvk
Copy link
Contributor
mhvk commented Dec 30, 2018

Yes, do keep code coverage. At astropy we use coveralls.io - might that be better?

@mhvk
Copy link
Contributor
mhvk commented Dec 30, 2018

Actually, never mind, we seem to be using codecov as well at astropy.

@rgommers
Copy link
Member

there's a PR open that demonstrates it can be green if we use the SciPy config verbatim as Pauli suggested -- see #12549, but that won't alert us to PRs that have poor test coverage in any way.

thanks, will move the comment over there.

don't get me wrong, having coverage reports nicely formatted at hand is very nice, I'm just tired of useless CI failures. looking at the PR list, it's mostly red. for various other reasons besides codecov, travisci and circleci have timeouts for example. but the current state is simply NOK, and codecov is the easiest to fix and offers least value of all the flaky CI we have.

@charris
Copy link
Member
charris commented Dec 31, 2018

As this does not report failure, how is it intended to be used?

@tylerjereddy
Copy link
Contributor

As this does not report failure, how is it intended to be used?

I would guess the intention is to keep an eye on it and start gradually reducing the failure count until it can be turned on.

I suppose one could grep out the current failure count and fail if it increases in new PRs & as Matti closes down the issues the threshold (i.e., currently 25 failures allowed) can be adjusted accordingly.

@shoyer
Copy link
Member
shoyer commented Dec 31, 2018 via email

@mattip mattip changed the title DEV: add pypy3 to azure CI WIP, DEV: add pypy3 to azure CI Jan 1, 2019
@mattip
Copy link
Member Author
mattip commented Jan 1, 2019

Marked this as WIP. We could xfail the missing pypy support for creating pep3118 buffers from ctype structures, but I think PyPy should fix that before we move forward with this PR.

@mattip mattip force-pushed the pypy3-testing branch 2 times, most recently from 26e3d66 to c233900 Compare April 16, 2019 17:29
@mattip mattip changed the title WIP, DEV: add pypy3 to azure CI DEV, BUILD: add pypy3 to azure CI Apr 17, 2019
@mattip
Copy link
Member Author
mattip commented Apr 17, 2019

This is ready, so I took off the WIP and the "always pass" check at the end of tests. PyPy (latest HEAD) has fixed everything except the tp_doc overwriting, which now is marked xfail, just like running python -OO

@mattip
Copy link
Member Author
mattip commented Apr 18, 2019

On windows, from test.support import gc_collect is failing. Does the azure pipelines VS2017-Win2016 image not include a complete stdlib? Strange.

@mattip
Copy link
Member Author
mattip commented Apr 18, 2019

xref Microsoft/azure-pipelines-image-generation#871

@shoyer
Copy link
Member
shoyer commented Apr 18, 2019

As I noted over in Microsoft/azure-pipelines-image-generation#871, the test module is meant for internal use by Python only. We shouldn't rely upon it downstream in NumPy.

@mattip
Copy link
Member Author
mattip commented Apr 18, 2019

If test.support.gc_collect is not officially blessed, should I go back to break_cycles?

@shoyer
Copy link
Member
shoyer commented Apr 18, 2019 via email

@mattip
Copy link
Member Author
mattip commented Apr 19, 2019

added a break_cycles function to testing. CI is passing.

Copy link
Contributor
@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

Seems useful to me--added some minor comments. It is the third fastest job in Azure for us it seems, and we still have two free parallel slots left, so seems reasonable.

@mattip mattip force-pushed the pypy3-testing branch 3 times, most recently from 44bfdf2 to 5daa9d4 Compare April 20, 2019 20:29
@eric-wieser
Copy link
Member

Looks good to me, but I'll let someone with more CI background approve/merge

@tylerjereddy
Copy link
Contributor

All review discussions are resolved, and I double checked the CI logs for the new PyPy Azure job & looks great.

Also, looks like Eric, Stephan, and I are in favor here.

In it goes, thanks Matti & reviewers.

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

Successfully merging this pull request may close these issues.

8 participants
0