-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add benchmark checks to CI #5424
Conversation
Apparently time_* functions need to accept the params too, even if they don't use it (same signature as setup). Adding *args is enough to fix, no need to copy the full signature.
Noting here that some of the benchmarks included with |
Thanks, I just merged #5426 and manually started the CI here. We will see if it autostarts on the next commit now... |
That seemed to do the trick, thanks @grlee77! |
FYI, if you are seeing warnings about a deprecated Basically, we changed the API to allow specification of the axis containing "channels" for color images rather than using a boolean |
I'm going to aim for relative performance measurements in GH Actions. I've seen several projects being ok with this approach so at least the catch regressions on PRs. We won't get a history of performance, but that won't be possible without dedicated hardware. I've merged a similar PR in my own fork that will run the benchmarks every 6h for a week for the same two commits. I will then collect the results (uploaded via artifacts) and compare how reliable they are. We should expect the same relative differences (+- some statistical deviation) in all runs if the approach is stable enough. I'll ping you when I have some numbers next week! |
Nice. This will be interesting to see! |
This notebook (attached) will help us assess the stability of the GHA CI for benchmarking purposes. We still need to wait a bit to collect more data points, though, but it looks ok so far! |
During the weekend we have collected 15 independent attempts. See analysis in this gist. Reliability is okayish, but it does give some false positives here and there... Let's see how it behaves over the weekdays instead of the weekend. |
Got it! I'll add some instructions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jaimergp, the new docs are both clear and comprehensive. I had just one suggestion to correct a typo.
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
Done |
Ok, confirmed! The files are there as expected! 🥳 |
Hi @jaimergp, I am seeing many more Azure CI failures after this PR. There seem to be two separate issues related to this commit: Removing the condition on running the gallery examples has resulted in all jobs timing out now, although the timeouts should be resolved if #5446 is merged. The second issue is that a couple of the cases fail to build wheels for (dependencies of) The specific error is
It is probably easiest to just restore the conditional running of the gallery examples, but let me know what you think. |
Oh, wow, that's totally an accident on my end. I shouldn't have removed that conditional. Will open a PR to revert now. Not sure about the 2nd issue. |
Yeah, I am not familiar with that package it is trying to build, but it seems to be something pulled in by sphinx-gallery. It might work if we selected a 64-bit Python interpreter, but I don't know. I think those failure cases will be bypassed by restoring the conditional in any case. |
Description
This PR provides a new CI workflow that runs the current commit against the PR target branch. The different strategies are discussed in the conversation below, which conclusions driven by data points collected in my fork.
It also adds a
asv check
command to the standard CI suite.Checklist
./doc/examples
(new features only)./benchmarks
, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py
.doc/release/release_dev.rst
.