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

bench all weekly - and fix for pallet_multisig lib #6789

Merged
merged 27 commits into from
Jan 24, 2025
Merged

Conversation

mordamax
Copy link
Contributor

@mordamax mordamax commented Dec 7, 2024

Closes #6196
Closes #7204

Example of PR: #6816

Every sunday 01:00 AM it's going to start to benchmark (with /cmd bench) all runtimes and all pallets
Then diff total will be pushed to a branch and PR open,. I assume review-bot is going assign required reviewers per changed files

I afraid each weeks will be too much to review & merge, but we can adjust later

Bonus: fix for pallet_multisig lib and substrate/.maintain/frame-weight-template.hbs , which didn't let to compile new weights

@mordamax mordamax marked this pull request as ready for review December 9, 2024 23:52
@mordamax mordamax requested review from a team as code owners December 9, 2024 23:52
@bkchr
Copy link
Member

bkchr commented Dec 10, 2024

The pr should be merged automatically if in the difference is not that much more. The cherry on top would be to compare multiple weeks and if it goes up, to create some sort of alarm.

@mordamax
Copy link
Contributor Author

The pr should be merged automatically if in the difference is not that much more. The cherry on top would be to compare multiple weeks and if it goes up, to create some sort of alarm.

There's only auto-merge feature available for automatic merging, but it will still require some (even formal) approvals, to avoid giving too much permissions for bot which would have to overcome at least github minimum requirements if we would like it to merge without approvals. Obviously we don't want it.

Given the fact that this is going to produce a diff in many folders, naturally it will assign too many people/teams for review (via Review-Bot), so I will turn this PR to draft again, and I'm going to think how to make this requirement more loose, as well as how to notify the owners of the weight files, if this went too much up

@mordamax mordamax marked this pull request as draft December 13, 2024 12:09
@mordamax mordamax marked this pull request as ready for review January 17, 2025 18:31
@mordamax mordamax added the R0-silent Changes should not be mentioned in any release notes label Jan 17, 2025
@github-actions github-actions bot deleted a comment from mordamax Jan 22, 2025
@github-actions github-actions bot deleted a comment from mordamax Jan 22, 2025
@github-actions github-actions bot deleted a comment from mordamax Jan 23, 2025
@github-actions github-actions bot deleted a comment from mordamax Jan 23, 2025
@mordamax mordamax changed the title bench all weekly - test bench all weekly - and fix for pallet_multisig lib Jan 23, 2025
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@paritytech-review-bot paritytech-review-bot bot requested review from a team January 23, 2025 21:41
@github-actions github-actions bot deleted a comment from mordamax Jan 23, 2025
@mordamax mordamax enabled auto-merge January 23, 2025 23:27
@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12939954000
Failed job name: check-runtime-migration

1 similar comment
@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12939954000
Failed job name: check-runtime-migration

@mordamax
Copy link
Contributor Author

/cmd bench --pallet pallet_multisig --clean

@github-actions github-actions bot deleted a comment from mordamax Jan 24, 2025
Copy link
Contributor

Command "bench --pallet pallet_multisig --clean" has started 🚀 See logs here

@iulianbarbu
Copy link
Contributor

iulianbarbu commented Jan 24, 2025

LGTM overall! Have a few questions. Disclaimer: I do not have enough context on benchmarking.

  1. Why don't we use bench-omni?

  2. When it comes to reviewing weights changes to all pallets/all runtimes, should we do checks against weights we committed a few weeks back too (e.g. 1/2 months ago)? I am not sure which are the thresholds we'll be accepting for regressions, and I am curious how we'd defend against constant increases below the thresholds week after week.

  3. I am seeing we run subweight compare commits somewhere in the workflow. Can the report be added to the PR - can be done in a follow up too? I am thinking such report might be useful for the comparisons with older weights as I suggested at point 2.

Curious about your thoughts.

@mordamax mordamax added this pull request to the merge queue Jan 24, 2025
Merged via the queue into master with commit f845a9f Jan 24, 2025
190 of 204 checks passed
@mordamax mordamax deleted the mak-weekly-bench branch January 24, 2025 10:09
@mordamax
Copy link
Contributor Author

LGTM overall! Have a few questions. Disclaimer: I do not have enough context on benchmarking.

  1. Why don't we use bench-omni?

we will switch to bench-omni as default bench as soon as the #6797 fix is confirmed 👍

  1. When it comes to reviewing weights changes to all pallets/all runtimes, should we do checks against weights we committed a few weeks back too (e.g. 1/2 months ago)? I am not sure which are the thresholds we'll be accepting for regressions, and I am curious how we'd defend against constant increases below the thresholds week after week.

Yep, we discussed this too, and the way to go could be comparing with the previous stable release tag. But need to explore this more

  1. I am seeing we run subweight compare commits somewhere in the workflow. Can the report be added to the PR - can be done in a follow up too? I am thinking such report might be useful for the comparisons with older weights as I suggested at point 2.

Yes, my plan was to replace this with link to https://weights.tasty.limo/compare - this shows a diff dynamically to a given branch. so we could do 2 links - 1) compare with master, 2) compare with prev release and add this to a body of a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multisig pallet has outdated WeightInfo trait Update pallet weights every week
7 participants