Add initial airspeed velocity (asv) framework #3137
Conversation
|
So I'm thinking about generating the benchmarks instead of writing each of them. I see several possibilities for that:
What do you think? |
|
@emmanuelle yes this is a very good plan, but now the autogeneration should somehow fit into the asv benchmark format, this is why I pushed things here. =) |
Codecov Report
@@ Coverage Diff @@
## master #3137 +/- ##
=======================================
Coverage 86.08% 86.08%
=======================================
Files 338 338
Lines 27234 27234
=======================================
Hits 23445 23445
Misses 3789 3789Continue to review full report at Codecov.
|
|
Once this is merged, we'll add an entry to https://github.com/TomAugspurger/asv-runner/blob/master/tests/full.yml and it'll start running automatically. |
|
@scikit-image/core I suggest merging this and allowing (/forcing) subsequent PRs to add benchmarks. Perhaps we should add a section to the development guide about writing asv benchmarks? At any rate I'm excited to have this merged! ;) |
|
@TomAugspurger what happens when benchmarks themselves are added/removed/changed? Can one re-run a whole range of commits on benchmarks committed at the end? |
|
I'm not sure offhand, but I think ASV tracks the most recent commit, and
only runs newer ones.
It's not ideal, but I can manually run additional commits if you want to
build up a history.
…On Tue, Jun 5, 2018 at 1:27 AM, Juan Nunez-Iglesias < ***@***.***> wrote:
@TomAugspurger <https://github.com/TomAugspurger> what happens when
benchmarks themselves are added/removed/changed? Can one re-run a whole
range of commits on benchmarks committed at the end?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3137 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIgyb4un1-AGTHEgryfER9URx78lbks5t5iTBgaJpZM4UVxWA>
.
|
|
@TomAugspurger You are asking for alot. I'm not sure it is possible to build anything older than 0.14 and have the tests pass ;) |
|
@hmaarrfk tests don't need to pass, only benchmarks. |
|
They'll show up at pandas.pydata.org/speed/
They're run nightly (Eastern time), but if this is merged in the next
couple hours I'll kick off a build now :)
…On Thu, Jun 7, 2018 at 2:50 PM, Emmanuelle Gouillart < ***@***.***> wrote:
***@***.**** commented on this pull request.
+1 for merging. Where will we see the results of the benchmarks?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3137 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIoClNg7yw6Xs6ozlfiyeKcVZT4JTks5t6YP3gaJpZM4UVxWA>
.
|
|
@jni could you, please, add a link from the comment above to somewhere (including the |
|
@emmanuelle @soupault done! =) @TomAugspurger how does asv work with PRs? The specific feature that I'm interested in is the ability to run even a new benchmark (provided by the PR) on the master branch and the PR tip (or the merge commit) before merging. Is this easy enough to do? |
|
Unfortunately, that’s not doable yet :/
It principle we could have a github bot respond to requests, but someone would have to write that bot :)
…________________________________
From: Juan Nunez-Iglesias <notifications@github.com>
Sent: Monday, June 11, 2018 8:14:49 PM
To: scikit-image/scikit-image
Cc: Tom Augspurger; Mention
Subject: Re: [scikit-image/scikit-image] Add initial airspeed velocity (asv) framework (#3137)
@emmanuelle<https://github.com/emmanuelle> @soupault<https://github.com/soupault> done! =)
@TomAugspurger<https://github.com/TomAugspurger> how does asv work with PRs? The specific feature that I'm interested in is the ability to run even a new benchmark (provided by the PR) on the master branch and the PR tip (or the merge commit) before merging. Is this easy enough to do?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#3137 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIucz4Ez6R7tfda_sd_1TAg7FeBYpks5t7xYJgaJpZM4UVxWA>.
|
|
@TomAugspurger that sounds like a fun challenge. And @Carreau has done the hard work with https://github.com/MeeseeksBox/MeeseeksDev. Can you give me a bit of background about what the bot would need to do? ie if I have a head commit and the master branch it split from, how do I:
? |
|
@jni As far as running goes, yes I think you've got it. You should be able to clone the PR and run I think the main concern is scheduling these so that we aren't running these on-demand runs with a.) eachother The benchmark machine is using Airflow right now to do the scheduling. Airflow supports external triggers, so this should be possible. I can help out with the airflow stuff if you want. |
|
@emmanuelle if we merge this soon we can beat the 4th anniversary of #1061. =P |
|
@hmaarrfk I know 😂 😭 😂 |
| @@ -7,6 +7,8 @@ | |||
| - [ ] Clean style in [the spirit of PEP8](https://www.python.org/dev/peps/pep-0008/) | |||
| - [ ] [Docstrings for all functions](https://github.com/numpy/numpy/blob/master/doc/example.py) | |||
| - [ ] Gallery example in `./doc/examples` (new features only) | |||
| - [ ] Benchmark in `./benchmarks`, if your changes aren't covered by an | |||
There was a problem hiding this comment.
Do we really want a benchmark for every new piece of functionality?
There was a problem hiding this comment.
@stefanv as with all of these tick marks, they are at the reviewers' discretion. =) The idea should certainly be considered for all PRs. And the idea is to build a suite over time. So anything that touches e.g. dilation, gaussian filters, watershed, etc should add a benchmark, and that benchmark should be run before and after the change, so that skimage moves towards speed as a whole.
|
Thank you, Juan! |
|
@jni good job on the asv PR. For those looking to install asv locally, we need to wait either for 0.2.2 or install with pip and git |
|
@TomAugspurger none of the benchmarks added after this PR appear on the ASV server! Any ideas why? I looked at |
|
I think my 2 year old hit the power button on the benchmark machine, and I didn't notice till last night 😆 It should be catching up now, and we've had a talk about touching Daddy's stuff. |
|
@TomAugspurger I feel like this is a longer term problem of not downloading from master. New benchmarks were added weeks ago... |
|
Oh although I just reread your statement that you think it's a longer term problem on your end also. =P Ok let's see how we go... =) |
|
Found the problem. When we initially set this up, I manually made the change adding a |
|
http://pandas.pydata.org/speed/scikit-image/ seems to be up to date now.
…On Fri, Aug 17, 2018 at 4:31 AM Juan Nunez-Iglesias < ***@***.***> wrote:
Oh although I just reread your statement that you think it's a longer term
problem on your end also. =P Ok let's see how we go... =)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3137 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIsp98zpVyMyqPOES45suwRPStK_1ks5uRo1tgaJpZM4UVxWA>
.
|
|
🎉 thanks @TomAugspurger! |
EDIT: Benchmarks at http://pandas.pydata.org/speed/
Description
In order to prevent performance regressions, we should set up asv to benchmark our slowest functions. This branch exists to work on that goal.
Closes #1061
For reviewers
later.
__init__.py.doc/release/release_dev.rst.