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

TST: mark tests as slow, fix missing random seed #9210

Merged
merged 2 commits into from
Sep 3, 2018

Conversation

rgommers
Copy link
Member

@rgommers rgommers commented Sep 2, 2018

Closes gh-7513. Gives about a 25% speed improvement for scipy.test().

This also fixes the one failure for running tests in parallel with pytest-xdist and scipy.test(extra_argv=['-n', 4])". That doesn't scale linearly (there seems to be a lot of scheduling/communication overhead), but for up to 4 cores there's a significant gain. Timings for fast test suite run:

  • current master: 280 sec
  • this PR: 180 sec
  • this PR on 4 cores: 65 sec
  • this PR on 16 cores: 50 sec

@rgommers rgommers added the maintenance Items related to regular maintenance tasks label Sep 2, 2018
@rgommers rgommers added this to the 1.2.0 milestone Sep 2, 2018
@rgommers
Copy link
Member Author

rgommers commented Sep 2, 2018

Appveyor has been horribly slow again, builds weren't even started 2 hours after PR submission. I've just tried disabling re-testing after merges - that's not very useful anyway. What I did:

  1. Log in on Appveyor and go to https://ci.appveyor.com/project/scipy/scipy/settings
  2. Change Branches to build from "All branches" to "All except branches specified below" and then specified master.

@rgommers
Copy link
Member Author

rgommers commented Sep 2, 2018

Can't tell if that Appveyor change worked until the scipy-wheels build completes - that will take about 5 hours it looks like (10 builds x 30 min/build). Note that if this doesn't work as expected, we can try https://www.appveyor.com/docs/how-to/filtering-commits/#skip-commits (filtering out /Merge pull request * from */).

@andyfaff
Copy link
Contributor

andyfaff commented Sep 3, 2018

Appveyor times for each matrix entry look to be around 30 mins at the moment. What proportion of that is build, and what proportion test? Is there anything we can do so speed the build up?

@andyfaff
Copy link
Contributor

andyfaff commented Sep 3, 2018

e.g. a bit of searching leads to clcache, but I'm not sure how one would go around integrating that with the build process/appveyor.

@rgommers
Copy link
Member Author

rgommers commented Sep 3, 2018

Appveyor times for each matrix entry look to be around 30 mins at the moment. What proportion of that is build, and what proportion test?

Not sure, but IIRC we optimized that fairly well. We skip 4 of the 8 configs unless there's a tag attached too; the total for PR builds is below 1.5 hrs (e.g. https://ci.appveyor.com/project/scipy/scipy/build/1.0.3689). Without concurrent builds I don't think we can do a whole lot better, and 1.5 hrs per PR is enough most of the time (although faster would be very nice). Problems are:

  1. the double CI runs on PR merge (trying to fix that now).
  2. any activity on the scipy-wheels repo
  3. particularly active days

For (2) and (3) I think we live with it until we get concurrent builds, which hopefully will be centrally organized/negotiated by NumFOCUS in the near future.

@charris
Copy link
Member

charris commented Sep 3, 2018

Hmm. @rgommers I've been looking to getting the NumPy project on appveyor working, but even when notified, it queues the tests but never runs them. AFAICT, the project is correctly configured, but something is wrong, I suspect the mail address may not have worked. In any case, I'd be curious as to how you went about setting up the project for SciPy.

@pv
Copy link
Member

pv commented Sep 3, 2018

I think xoviat tried clcache, but it didn't speed up so much. We also don't necessarily want to use it for scipy-wheels (doesn't help so much with infrequent builds).

I'm not sure why scipy-wheels appveyor is that slow. I don't think the tests are slower there.

@pv
Copy link
Member

pv commented Sep 3, 2018

@charris: For asv I just put in "airspeed-velocity admin" as "Your name" and used my own email address (with +stuff added before @ --- my email provider has subaddressing). After that I added collaborators as admins. The "scipy" account I think someone (@larsoner?) created with their own email, and then added collaborators enabled the github team support.

@larsoner
Copy link
Member

larsoner commented Sep 3, 2018

In any case, I'd be curious as to how you went about setting up the project for SciPy.

IIRC I followed what at the time were the AppVeyor "organization account" methods, something like:

  1. ensured any existing scipy AppVeyor webhooks were deleted on GitHub
  2. made an "organizational account" by creating a user named scipy with my-gmail-address+scipy@gmail.com (because gmail allows you to add whatever you want after the +)
  3. logged in with this scipy account
  4. added SciPy devs that I had email addresses for as admins on the account (under Team in the scipy user's settings)
  5. logged out of the scipy account on AppVeyor
  6. logged into AppVeyor using GitHub auth, and when it asked which user I wanted to act as I selected scipy
  7. added public repo github.com/scipy/scipy.git
  8. pushed some code / PR to see if the webhook was delivered (I think under the defaults it built PRs, but it's worth checking the org settings)

I did this for probably half a dozen repos. There were one or two where something went amiss with this process and I had to delete the webhooks and reenable them. Eventually they worked somehow. I might have actually had to email / post a support question, not sure.

@rgommers
Copy link
Member Author

rgommers commented Sep 3, 2018

this is safe to merge without waiting for Appveyor by the way. average of fast build matrix entries on TravisCI went down from ~16.2 min to ~13.8 min.

@pv pv merged commit 6683b5f into scipy:master Sep 3, 2018
@pv
Copy link
Member

pv commented Sep 3, 2018

thanks, in it goes

@rgommers rgommers deleted the mark-slow-tests branch September 3, 2018 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Further test suite speedups
5 participants