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

ci: Continuous Benchmarking #1785

Merged
merged 15 commits into from
Jul 1, 2024
Merged

ci: Continuous Benchmarking #1785

merged 15 commits into from
Jul 1, 2024

Conversation

MarquessV
Copy link
Contributor

Description

closes #1778

@MarquessV
Copy link
Contributor Author

The PR benchmark workflow fails because there are no records for the default branch yet. That shouldn't be an issue after this PR lands and the first set of benchmarks get published.

Copy link

github-actions bot commented Jun 8, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
7214 6326 88% 87% 🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: 61d2599 by action🐍

@MarquessV MarquessV marked this pull request as ready for review June 11, 2024 14:52
@MarquessV MarquessV requested a review from a team as a code owner June 11, 2024 14:52
CONTRIBUTING.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
test/benchmarks/test_program.py Show resolved Hide resolved
MarquessV and others added 2 commits June 11, 2024 10:03
@Shadow53
Copy link
Contributor

Thanks for those fixes!

I'm approving, but want to call out a couple of things:

  1. The CI job is failing because there is no data for master. That is understandable, but I am concerned the job will break differently because that failure is hiding something else. Is it possible to commit some fake master data, e.g. from this branch, and use that for testing? Merging this should overwrite that data anyway, right?
  2. Looking ahead, is there something we can use like CacheGrind to count instructions to avoid the issue of resource contention during benchmarking?

@MarquessV
Copy link
Contributor Author

Thanks for those fixes!

I'm approving, but want to call out a couple of things:

1. The CI job is failing because there is no data for `master`. That is understandable, but I am concerned the job will break differently because that failure is hiding something else. Is it possible to commit some fake `master` data, e.g. from this branch, and use that for testing? Merging this should overwrite that data anyway, right?

2. Looking ahead, is there something we can use like `CacheGrind` to count instructions to avoid the issue of resource contention during benchmarking?
  1. Maybe? bencher has a mock functionality that I might be able to use. I'll give it a try and if it's easy enough I'll go for it.
  2. Not easily. None of the bencher supported adapters for Python support gathering that metric. Timing has its flaws, but my hope is that it will be consistent enough for the benchmarks we need in this project. At the very least, I think it makes sense to try it before investing more time in a custom solution that supports something like that.

Copy link

github-actions bot commented Jun 14, 2024

🐰Bencher

ReportFri, June 14, 2024 at 18:57:46 UTC
Projectpyquil
Branch1778-implement-benchmarks-new
Testbedci-runner-linux
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
test/benchmarks/test_program.py::test_copy_everything_except_instructions✅ (view plot)10,172,871,523.00 (-1.69%)10,840,870,546.41 (93.84%)
test/benchmarks/test_program.py::test_instructions✅ (view plot)3,785,769,392.60 (-3.90%)4,268,328,890.29 (88.69%)
test/benchmarks/test_program.py::test_iteration✅ (view plot)3,812,074,204.60 (-2.79%)4,150,495,761.34 (91.85%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

@jselig-rigetti jselig-rigetti left a comment

Choose a reason for hiding this comment

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

Nice! I

.github/workflows/benchmark_base.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark_pr.yml Show resolved Hide resolved
.github/workflows/benchmark_base.yml Outdated Show resolved Hide resolved
@MarquessV MarquessV merged commit 3ed86cc into master Jul 1, 2024
24 checks passed
@MarquessV MarquessV deleted the 1778-implement-benchmarks-new branch July 1, 2024 21:58
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.

Add benchmarks
4 participants