Feature: Measuring the airspeed velocity of an unladen signac #629
Conversation
bdice
requested review from
b-butler and
SchoeniPhlippsn
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## master #629 +/- ##
==========================================
+ Coverage 77.92% 78.31% +0.39%
==========================================
Files 65 65
Lines 7079 7079
Branches 1310 1310
==========================================
+ Hits 5516 5544 +28
+ Misses 1249 1227 -22
+ Partials 314 308 -6
Continue to review full report at Codecov.
|
|
That's really cool! The only thing that bothers is me is the lack of axis labels. 😄 I think we can get rid of the old benchmark tests as long as it is super easy to still run benchmarks on your computer and as part of the CI. |
You can get axis labels if you click through into the full chart, just not on the overview. Example: https://glotzerlab.github.io/signac/#benchmarks.ProjectBench.time_iterate The overview chart is called a sparkline: https://en.wikipedia.org/wiki/Sparkline 😄 |
| def time_iterate_load_sp(self, *params): | ||
| for _ in range(10): | ||
| [job.sp() for job in self.project] |
There was a problem hiding this comment.
Given lazy loading it would be interesting to see single pass speeds as well, unless this does it every pass (I am not too familiar with the loading that takes place here.
There was a problem hiding this comment.
I'd like to leave the scope of what we benchmark the same in this PR as we have currently implemented in benchmark.py. I am unsure how lazy loading / caching would work if we added another test that loads statepoints here. Ideally we want to have an empty cache when each test executes, but I can't remember if the setup/teardown are run for each test method or each test class. (It's been a few weeks since I worked on this.)
|
@SchoeniPhlippsn Do you think you will be able to have a look at this soon? |
Yes! The next action item is to move some additional options like document size, statepoint key/value sizes, etc. into the new benchmark script. I think we can ignore the ability to set those via command line and just edit the script (and rerun asv) if those values need to be set for a particular benchmark. |
|
@b-butler @csadorf What do you think we should do with |
| raise TypeError("N must be an integer!") | ||
|
|
||
| temp_dir = TemporaryDirectory() | ||
| project = signac.init_project(f"benchmark-N={N}", root=temp_dir.name) |
There was a problem hiding this comment.
Any reason to not use (a potentially extended/adapted variant of)
signac/signac/contrib/project.py
Line 2412 in e60bfbc
Line 8 in 8a804d0
There was a problem hiding this comment.
The TemporaryProject yields a project when used as a context manager. The benchmark script requires something we can store (like self.project and self.temp_dir) in setup and then destroy during the teardown method. I'm not aware of a clean way to use that context manager here.
There was a problem hiding this comment.
Well, even if you don't use TemporaryProject, you could still use the testing.init_jobs() function?
There was a problem hiding this comment.
Hmm, I had forgotten about that function. It looks like it is currently designed for tests of projects with complex schemas, not tests of performance with varying data sizes. We would probably have to change that function to support the arguments like num_keys, num_doc_keys, data_size, data_std, etc., and that feels out of scope for this PR. I'll finalize and merge as-is, since you indicated this is a non-blocking issue.
I think it's fine to leave it for now, but add an in-code comment where appropriate about the purpose of each method and about the motivation to keep both. |
|
@csadorf Feel free to give this a final pass of review if you'd like. Otherwise it's ready to merge. I added docs here: https://signac--629.org.readthedocs.build/projects/core/en/629/support.html#benchmarking |
| raise TypeError("N must be an integer!") | ||
|
|
||
| temp_dir = TemporaryDirectory() | ||
| project = signac.init_project(f"benchmark-N={N}", root=temp_dir.name) |
There was a problem hiding this comment.
Well, even if you don't use TemporaryProject, you could still use the testing.init_jobs() function?
My mistake, I meant to push that fix directly to master and didn't notice that it was on this branch. |
Description
This PR uses asv (airspeed velocity) to measure performance of the signac package. It could replace our existing
benchmark.pyscript. I haven't yet tried all the features of asv, but it does a lot of things we care about and is a very powerful / helpful tool:asv run v1.3.0..mastertests every commit from v1.3.0 to master (with the current commit's set of benchmarks, so it doesn't require any git trickery!)asv publishasv previewasv devasv profile 'benchmarks.ProjectBench.time_iterate_load_sp(.*)' --gui=snakevizasv comparePreview of asv results from v1.3.0 to master (this link may be removed in the future): https://glotzerlab.github.io/signac/
To-do:
benchmark.py(see below)Motivation and Context
I have always wanted to try asv and I think this is a great tool for understanding the project's longitudinal improvements. From a reporting/visualization/profiling perspective, asv seems like a massive step forward. Especially since releases 1.6.0 and 1.7.0 were very carefully tested for performance, it would be nice to add this tool to ensure that we keep high performance into signac 2.0 and beyond.
We can separately discuss whether to remove
benchmark.py. For now, I'm happy to keep both. To get rid ofbenchmark.pywe'll want to decide how to incorporate all the flags frombenchmark.py, like those controlling document sizes, number of state point keys, etc. I think I would recommend that we just add those as parameters, and then let users alter the settings of the benchmark file if they need to test particular configurations (e.g. large documents).Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist:
If necessary: