-
Notifications
You must be signed in to change notification settings - Fork 79
Benchmarking #2454
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
Benchmarking #2454
Conversation
|
Looks great! Some unexpected differences here in the e.g. Re running in CI, I guess we just run it to make sure it hasn't broken but don't pay any attention to the numbers? Probably worth spinning into its own Workflow, so we're not running it n-times in the Tests? |
Codecov Report
@@ Coverage Diff @@
## main #2454 +/- ##
==========================================
- Coverage 93.44% 92.51% -0.94%
==========================================
Files 28 28
Lines 27380 27732 +352
Branches 1253 1350 +97
==========================================
+ Hits 25584 25655 +71
- Misses 1762 2040 +278
- Partials 34 37 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Yes, the colouring was a quick hack. I think for those fast ones that is usual variance.
Exactly, mostly run it to check we didn't break it, occasionally look at the numbers if useful.
Yeah, very simple action that will finish before all the other tests, so not making our CI longer in total. |
08fb3a8 to
e9b106a
Compare
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.
Looks great!
The only major thing that's missing from the benchmarks is row-by-row access with/out metadata decoding, but we can add that in later.
|
Wow!!!! This is great! |
0db6de8 to
5efe70b
Compare
|
I've added a test for decoding metadata. I've also added a script that runs the benchmarks across all released versions. I think we should keep an issue open for RAM benching. |
|
BTW, this shows how great the backwards compatibility has been. Same 0.5.2 file for all these benchmarks! |
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.
Awesome!
Few minor suggestions, merge away whenever you're happy.
| if __name__ == "__main__": | ||
| versions = [v for v in versions("tskit") if "a" not in v and "b" not in v] | ||
| for v in tqdm.tqdm(versions): | ||
| os.system(f"pip install tskit=={v}") |
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.
I think it's probably worth automating in a venv here, so that the devs environment doesn't get messed up by running this.
| if __name__ == "__main__": | ||
| versions = [v for v in versions("tskit") if "a" not in v and "b" not in v] | ||
| for v in tqdm.tqdm(versions): | ||
| os.system(f"pip install tskit=={v}") |
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.
subprocess.run(check=True, shell=True) is a better option here
| return ret | ||
|
|
||
|
|
||
| def make_file(): |
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.
Slightly nicer:
benchfile = tskit_dir / "benchmark" / "bench.trees"
if not benchfile.exists():
...
ts.dump(benchfile)
similar patterns below
7c24452 to
1d46672
Compare

WIP #2444
Needs some polish, but probably a good point to get early feedback on the direction. The results are appended to a JSON file that will be committed to the repo, and a very basic HTML report generated:
My thinking is that at under a minute we can add this to CI.