-
Notifications
You must be signed in to change notification settings - Fork 954
Add PR and Release benchmark with new changes in framework #2871
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
base: unstable
Are you sure you want to change the base?
Conversation
Signed-off-by: Roshan Khatri <[email protected]>
a7f8b5c to
e4d129c
Compare
Signed-off-by: Roshan Khatri <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #2871 +/- ##
============================================
- Coverage 72.44% 72.43% -0.01%
============================================
Files 128 128
Lines 70415 70439 +24
============================================
+ Hits 51011 51026 +15
- Misses 19404 19413 +9 🚀 New features to boost your workflow:
|
|
@roshkhatri looks like we are adding x86 as well? I am not sure if we should add x86 runs if we are not confident on the stability and numbers yet. |
rainsupreme
left a comment
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.
LGTM! 👍
I wrote a few notes but you don't have to fix them in this PR.
| "io-threads": [1,9], | ||
| "benchmark-threads": 90, | ||
| "server_cpu_range": "0-8", | ||
| "client_cpu_range": "144-191,48-95" |
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.
curious why the range is split. Is it something to do with NUMA nodes..?
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.
Yes for X86 the NUMA nodes have cores split
| python-version: "3.10" | ||
| cache: "pip" | ||
|
|
||
| - name: Install dependencies |
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 feel like I've seen this checkout and dependency setup stuff in multiple places. Is there some way we could deduplicate it and avoid the possibility of bugs from having accidental differences between them over time?
| with: | ||
| path: artifacts | ||
|
|
||
| - name: Combine results and create comprehensive report |
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.
for the record, I'm not a fan of putting "business logic" in yml files like this. I'd prefer this to be in a script that we call, but I'm not going to make a big fuss in this PR. It's not worse than what you're replacing.
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.
Thats true, I was thinking the other way, where I didnt want to add another script which will be used just for one simple purpose 😅
Yes, we would still like to get the benchmark numbers for X86, while doing the releases. The PR only used ARM64 though |
Signed-off-by: Roshan Khatri <[email protected]>
rainsupreme
left a comment
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.
Updates look fine 👍
This adds the workflow improvements for PR and Release benchmark where it runs on
c8g.metal-48xlforARM64andc7i.metal-48xlforX86c8g.metal-48xl Spec: https://aws.amazon.com/ec2/instance-types/c8g/
c7i.metal.48xl Spec: https://aws.amazon.com/ec2/instance-types/c7i/
PR benchmarking will be executed on ARM64 machine as it has been seen to be more consistent.
Additionally, it runs 5 iterations for each tests and posts the average and other statistical metrics like
Note: Values with (n=X, σ=Y, CV=Z%, CI99%=±W%, PI99%=±V%) indicate averages from X runs with standard deviation Y, coefficient of variation Z%, 99% confidence interval margin of error ±W% of the mean, and 99% prediction interval margin of error ±V% of the mean. CI bounds [A, B] and PI bounds [C, D] show the actual interval ranges.
For comparing between versions, it adds a workflow which runs on both ARM64 and X86 machine. It will also post the comparison between the versions like this: #2580 (comment)