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

Add some benchmarks #1658

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add some benchmarks #1658

wants to merge 10 commits into from

Conversation

dota17
Copy link
Contributor

@dota17 dota17 commented Sep 8, 2020

major changes:

  • fix timeout failure in benchmark_slicing.py
  • remove unnecessary code in benchmark_slicing.py
  • add benchmarks for group/dataset creation etc.

TODO:

  • add readme.rst in benchmarks

CC: @takluyver

@codecov
Copy link

codecov bot commented Sep 8, 2020

Codecov Report

Merging #1658 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1658   +/-   ##
=======================================
  Coverage   88.51%   88.51%           
=======================================
  Files          17       17           
  Lines        2255     2255           
=======================================
  Hits         1996     1996           
  Misses        259      259           
Impacted Files Coverage Δ
h5py/_hl/group.py 96.80% <0.00%> (ø)
h5py/_hl/dataset.py 93.15% <0.00%> (ø)
h5py/_hl/filters.py 92.78% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e41d3f2...5b78744. Read the comment docs.

benchmarks/README.rst Outdated Show resolved Hide resolved
benchmarks/README.rst Outdated Show resolved Hide resolved
benchmarks/asv.conf.json Outdated Show resolved Hide resolved
benchmarks/benchmarks/benchmark_dataset.py Outdated Show resolved Hide resolved
benchmarks/benchmarks/benchmark_dataset.py Outdated Show resolved Hide resolved
self.f.close()

def time_virtual_dataset(self):
self.f.create_virtual_dataset('vdata', self.layout, fillvalue=-1)
Copy link
Member

Choose a reason for hiding this comment

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

I haven't looked at all of these in detail, but does it make sense to time only this bit, leaving the layout/source bits in setup? For that matter, does it make sense to benchmark creating virtual datasets at all? I can probably make a good case that it's not particularly important for performance.

The more general point is that we need to design benchmarks that can tell us something interesting, which isn't always easy.

benchmarks/benchmarks/benchmark_dataset.py Outdated Show resolved Hide resolved
@aragilar
Copy link
Member

We don't seem to run the benchmarks as part of the CI, I think we should think about how/where/when we want to run these.

@dota17
Copy link
Contributor Author

dota17 commented Sep 11, 2020

We don't seem to run the benchmarks as part of the CI, I think we should think about how/where/when we want to run these.

I probably know how to do it. I'll try it in next days, and maybe we should also add an item in Sphinx documentation.

@takluyver
Copy link
Member

I think benchmarks on CI are kind of tricky - you really want to run them on a consistent system with no other significant work competing for the CPU, memory or disk access, so that one run can be compared to the next. I don't think that's what any of the free CI services will offer, for obvious reasons.

I've got a Raspberry Pi sitting around not doing much. Maybe I can get that set up to run benchmarks regularly.

@dota17
Copy link
Contributor Author

dota17 commented Sep 12, 2020

I think benchmarks on CI are kind of tricky - you really want to run them on a consistent system with no other significant work competing for the CPU, memory or disk access, so that one run can be compared to the next. I don't think that's what any of the free CI services will offer, for obvious reasons.

You are right. I think I should stop moving forward on this, because I realize that asv can't automatically run on CI, when running on a new machine, it has an interactive process to collect various configuration of the machine.

I've got a Raspberry Pi sitting around not doing much. Maybe I can get that set up to run benchmarks regularly.

Raspberry Pi YES 🚀
I have a Raspberry Pi too, it's one of the most fun digital products in the world :)

@dota17 dota17 force-pushed the benchmark branch 2 times, most recently from c95dc62 to 05b0294 Compare September 14, 2020 09:05
@aragilar
Copy link
Member

aragilar commented Oct 2, 2020

I wonder if we want to move the benchmarks to their own repository, as that will make it easier to run the latest set of benchmarks against different commits without having to copy files around?

@takluyver
Copy link
Member

I think asv already takes care of running the same benchmark code against library code from different commits. It seems I can write a new benchmark and then run it against older commits, in any case. But I don't particularly have a good argument for having them in the same repo, if you think a separate repo is better.

@aragilar
Copy link
Member

aragilar commented Oct 2, 2020

Cool, I didn't realise asv could do that.

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

Successfully merging this pull request may close these issues.

None yet

3 participants