issue_comments

Thanks @Illviljan this is great work!

Unit Test Results

6 files           6 suites   1h 0m 16s :stopwatch: 16 226 tests 14 490 :heavy_check_mark: 1 736 :zzz: 0 :x: 90 552 runs  82 372 :heavy_check_mark: 8 180 :zzz: 0 :x:

Results for commit 70cd679e.

:recycle: This comment has been updated with latest results.

Here's how long dataarray_missing.py takes in this workflow with different shapes: 321a761 - shape=(100, 25, 25), 3 minutes 8f262f9 - shape=(365, 50, 50), 3m 28s 0b7b1a0 - shape=(365, 75, 75), 4m 8s 8f08506 - shape=(365, 100, 100), 5m 47s d1b908a - shape=(365, 200, 400), 12m 38s 56556f1 - shape=(3650, 100, 100) 19m 55s and crashes 1eba65c - shape=(3650, 200, 400), 20 minutes and crashes

Changed the shape to shape=(365, 75, 75) as that seems to be around tipping point where it starts slowing down.

Hello @Illviljan! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2021-10-07 18:02:52 UTC

Getting a weird error now.

Might be related to * https://github.com/h5py/h5py/issues/1880 * https://github.com/conda-forge/h5py-feedstock/issues/92

Down to 30 mins now. dataset_io is being skipped however.

I think this is finished and is ready for review.

@dcherian did you have something specific in mind when you linked to https://github.com/jaimergp/scikit-image/blob/main/.github/workflows/benchmarks-cron.yml in #4648? I think I'll remove it otherwise and just focus on

Nothing specific. Just wanted to link their configuration.

Down to 101 minutes now. :)

I thinks it's mainly from the tests that I haven't checked yet. If someone wants to take a stab at reducing the times and removing errors for dataset_io.py and indexing.py that would be great.

Hi @Illviljan — we'd be interesting in seeing whether you wanted to join the core developers group — but we don't have a way of contacting you! So a comment on your PR was the best I could think of. If you'd be interested in discussing this, drop me an email at m at maximilian roos dot com . Thanks!

There we go. I think the workflow works as intended now we'll see in 3-4 hours. Only thing left is to improve the tests which can be done in other PRs and maybe rewrite that README slightly.

@dcherian did you have something specific in mind when you linked to https://github.com/jaimergp/scikit-image/blob/main/.github/workflows/benchmarks-cron.yml in #4648? I think I'll remove it otherwise and just focus on * Benchmark label triggered - from scikit * Benchmark no label triggered - from scikit modded by me.

Do you mean skipping those that require bottleneck rather than relying on asv to handle the crash? If so, sounds good

Hmm, I was just thinking installing all possible dependencies. But I think I've simply misunderstood the tests and why they were crashing.

The Benchmark no label triggered seems to have succeded but still failed? Anyone understands what the error means? ``` [100.00%] ··· unstacking.UnstackingDask.time_unstack_slow 32.7±0.3ms

BENCHMARKS NOT SIGNIFICANTLY CHANGED. + grep 'Traceback \|failed\|PERFORMANCE DECREASED' benchmarks.log + exit 1 ++ '[' 1 = 1 ']' ++ '[' -x /usr/bin/clear_console ']' ++ /usr/bin/clear_console -q Error: Process completed with exit code 1. ```

Edit: I think I understand now. We get a bunch of Traceback errors, that's why it's erroring even though the performance didn't change.

Ah one major problem IIUC is that we run a lot of benchmarks with and without bottleneck even if bottleneck isn't involved in the operation being benchmarked.

Some of the following should be fixed

ImportError: Pandas requires version '0.12.3' or newer of 'xarray' (version '0.0.0' currently installed).

rolling.Rolling.time_rolling_construct
xarray.core.merge.MergeError: conflicting values for variable 'x_coords' on objects to be combined. You can skip this check by specifying compat='override'.

IOWriteNetCDFDaskDistributed.time_write Looks like we're spinning up multiple dask clusters. UserWarning: Port 8787 is already in use. Perhaps you already have a cluster running? Hosting the HTTP server on port 33423 instead warnings.warn(

Long times, +240 minutes

yeah this is a problem.

Maybe we need to go through and mark the slow tests like in the README_ci.md document

In that vein, a new private function is defined at benchmarks.__init__: _skip_slow. This will check if the ASV_SKIP_SLOW environment variable has been defined. If set to 1, it will raise NotImplementedError and skip the test. To implement this behavior in other tests, you can add the following attribute:

Add more required installs? bottleneck for examples crashes tests because it isn't installed. Add all the non-required as well?

Do you mean skipping those that require bottleneck rather than relying on asv to handle the crash? If so, sounds good

Can a normal user add and remove labels?

An alternative approach would be to use something similar to our [skip-ci] and [test-upstream] tags in the commit message. Though I think that tag needs to be on every commit you want benchmarked.

Here's some things besides not getting green ticks @max-sixty: * Long times, +240 minutes. scikit had it down to like 13min somehow. What's the bottleneck? Which tests are super slow? * A lot of printed errors which makes the report very messy, go through each test and fix those. Not necessary for this PR though. * asv.conf.json has been moved. If you have any idea how to trigger this job inside asv_bench that would be nice. * Should we align benchmark folder names? Numpy? * Add more required installs? bottleneck for examples crashes tests because it isn't installed. Add all the non-required as well? * Can a normal user add and remove labels? I set this up so that I don't have to deal with the nightmare of setting up asv locally. Would be nice to avoid having to ask you over and over again to add/remove labels. To trigger it.

Awesome!! Thanks a lot @Illviljan !

On how we run these — I would be fine with the labeled approach that scikit-learn uses.

What else do you think is needed for this? It already looks excellent, thank you.