Skip to content

[FRAME] Omni bencher fixes #8265

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 36 commits into from
May 19, 2025
Merged

[FRAME] Omni bencher fixes #8265

merged 36 commits into from
May 19, 2025

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Apr 16, 2025 •
edited
Loading

Changes:

  • Add --pallets option to selectively run multiple pallets. In the past we only had --pallet to run a single one.
  • Add --exclude-extrinsics [pallet::extrinsic] to add (Pallet,Extrinsic) tuples that should be excluded.
  • Fix storage overlay reversion before the benchmark runs.
  • Test root hash for V2 benchmarks to be deterministic
  • Changed DB reps to 1 for speedup since it should not be needed to run multiple times. (TODO test)

Checked that it fixes the Kusama benchmark issue when patching to a fixed stable2412 fork:

(before)

The following 5 benchmarks failed:
- pallet_nomination_pools::apply_slash_fail
- pallet_nomination_pools::migrate_delegation
- pallet_nomination_pools::pool_migrate
- pallet_offences::report_offence_babe
- pallet_offences::report_offence_grandpa
Error: Input("5 benchmarks failed")

(after)

The following 1 benchmarks failed:
- pallet_nomination_pools::apply_slash_fail
Error: Input("1 benchmarks failed")

This one needs fixing but is not breaking the other ones anymore.

ggwpez added 7 commits April 16, 2025 19:39
@ggwpez
Add --pallets to run specific benchmark pallets
c2c6e90 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Add --exclude-extrinsics pallet::extrinsic
0c302fe 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
More omni bencher fixes
64ce487 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
fixes
7d4a38b 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
cleanup
74c6b57 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
fmt
1c73938 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Typo
ac2f261 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Copy link
Member Author

ggwpez commented Apr 16, 2025

/cmd bench --runtime westend

@github-actions GitHub Actions
Copy link
Contributor

Command "bench --runtime westend" has started 🚀 See logs here

@github-actions GitHub Actions
Copy link
Contributor

Command "bench --runtime westend" has failed ❌! See logs here

ggwpez added 2 commits April 16, 2025 21:15
@ggwpez
Disable check
c3a9223 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Fix hash
89c6c93 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Copy link
Member Author

ggwpez commented Apr 16, 2025

/cmd bench --runtime westend

@github-actions GitHub Actions
Copy link
Contributor

Command "bench --runtime westend" has started 🚀 See logs here

@github-actions GitHub Actions
Copy link
Contributor

Command "bench --runtime westend" has failed ❌! See logs here

@bkontur bkontur mentioned this pull request Apr 17, 2025
Open
3 tasks
ggwpez added 3 commits April 18, 2025 16:02
@ggwpez
Fix
8294226 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
fmt
99d07b9 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Merge branch 'master' into oty-omni-bencher-fixes
9b49e08
@ggwpez ggwpez marked this pull request as ready for review April 18, 2025 14:13
@ggwpez ggwpez requested a review from a team as a code owner April 18, 2025 14:13
ggwpez added 2 commits April 18, 2025 16:17
@ggwpez
zepter
2d58ef1 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
fmt
31fdbb4 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
bkchr
bkchr reviewed Apr 18, 2025
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Would be nice to get a test that let's some benchmark fail and checks in a follow up benchmark that it can not read the data from the previous benchmark. Basically what we have seen in the fellowship runtimes.

ggwpez added 6 commits May 9, 2025 18:29
@ggwpez
hex -> array_bytes
1e0daff 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Undeploy from Westend
41bf001 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Check Kitchensink storage root
da527b1 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Factor check into excluded_extrinsics
990109c 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Cleanup
5ec79a8 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
fmt
2232bc1 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@athei athei disabled auto-merge May 19, 2025 08:31
athei
athei requested changes May 19, 2025
View reviewed changes
Copy link
Member

@athei athei left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you have a look at the PRdoc? It seems to have major version bumps which are not necessary. And it lacks a version bump for the frame-omni-bencher.

Asking because I think we need to backport this to stable2503 in order to benchmark the fellowship runtimes.

bkontur
bkontur reviewed May 19, 2025
View reviewed changes
@athei
Copy link
Member

athei commented May 19, 2025 •
edited
Loading

We also need a bump for frame-omni-bencher I think. Its a binary crate but we still want the crates.io version to match. So we can just have a minor bump I guess. For binaries semver isn't really important.

Added the bump for the bencher binary.

@athei athei added the A4-backport-stable2503 Pull request must be backported to the stable2503 release branch label May 19, 2025
athei
athei reviewed May 19, 2025
View reviewed changes
@athei athei enabled auto-merge May 19, 2025 08:55
@athei athei added this pull request to the merge queue May 19, 2025
github-merge-queue bot pushed a commit that referenced this pull request May 19, 2025
@ggwpez @github-actions
@bkontur @athei
[FRAME] Omni bencher fixes (#8265)
7387bf9 
Changes:
- Add `--pallets` option to selectively run multiple pallets. In the
past we only had `--pallet` to run a single one.
- Add `--exclude-extrinsics [pallet::extrinsic]` to add
(Pallet,Extrinsic) tuples that should be excluded.
- Fix storage overlay reversion *before* the benchmark runs.
- Test root hash for V2 benchmarks to be deterministic
- <s>Changed DB reps to 1 for speedup since it should not be needed to
run multiple times. (TODO test)</s>

Checked that it fixes the Kusama benchmark issue when
[patching](https://github.com/ggwpez/substrate-scripts/blob/master/diener.py)
to a fixed stable2412 fork:

(before)
```pre
The following 5 benchmarks failed:
- pallet_nomination_pools::apply_slash_fail
- pallet_nomination_pools::migrate_delegation
- pallet_nomination_pools::pool_migrate
- pallet_offences::report_offence_babe
- pallet_offences::report_offence_grandpa
Error: Input("5 benchmarks failed")
```

(after)
```pre
The following 1 benchmarks failed:
- pallet_nomination_pools::apply_slash_fail
Error: Input("1 benchmarks failed")
```
This one needs fixing but is not breaking the other ones anymore.

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Alexander Theißen <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 19, 2025
@ggwpez ggwpez enabled auto-merge May 19, 2025 12:51
@ggwpez ggwpez added this pull request to the merge queue May 19, 2025
github-merge-queue bot pushed a commit that referenced this pull request May 19, 2025
@ggwpez @github-actions
@bkontur @athei
[FRAME] Omni bencher fixes (#8265)
31fad4b 
Changes:
- Add `--pallets` option to selectively run multiple pallets. In the
past we only had `--pallet` to run a single one.
- Add `--exclude-extrinsics [pallet::extrinsic]` to add
(Pallet,Extrinsic) tuples that should be excluded.
- Fix storage overlay reversion *before* the benchmark runs.
- Test root hash for V2 benchmarks to be deterministic
- <s>Changed DB reps to 1 for speedup since it should not be needed to
run multiple times. (TODO test)</s>

Checked that it fixes the Kusama benchmark issue when
[patching](https://github.com/ggwpez/substrate-scripts/blob/master/diener.py)
to a fixed stable2412 fork:

(before)
```pre
The following 5 benchmarks failed:
- pallet_nomination_pools::apply_slash_fail
- pallet_nomination_pools::migrate_delegation
- pallet_nomination_pools::pool_migrate
- pallet_offences::report_offence_babe
- pallet_offences::report_offence_grandpa
Error: Input("5 benchmarks failed")
```

(after)
```pre
The following 1 benchmarks failed:
- pallet_nomination_pools::apply_slash_fail
Error: Input("1 benchmarks failed")
```
This one needs fixing but is not breaking the other ones anymore.

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Alexander Theißen <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 19, 2025
@ggwpez ggwpez added this pull request to the merge queue May 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 19, 2025
@bkontur bkontur added this pull request to the merge queue May 19, 2025
github-merge-queue bot pushed a commit that referenced this pull request May 19, 2025
@ggwpez @github-actions
@bkontur @athei
[FRAME] Omni bencher fixes (#8265)
d33bcfd 
Changes:
- Add `--pallets` option to selectively run multiple pallets. In the
past we only had `--pallet` to run a single one.
- Add `--exclude-extrinsics [pallet::extrinsic]` to add
(Pallet,Extrinsic) tuples that should be excluded.
- Fix storage overlay reversion *before* the benchmark runs.
- Test root hash for V2 benchmarks to be deterministic
- <s>Changed DB reps to 1 for speedup since it should not be needed to
run multiple times. (TODO test)</s>

Checked that it fixes the Kusama benchmark issue when
[patching](https://github.com/ggwpez/substrate-scripts/blob/master/diener.py)
to a fixed stable2412 fork:

(before)
```pre
The following 5 benchmarks failed:
- pallet_nomination_pools::apply_slash_fail
- pallet_nomination_pools::migrate_delegation
- pallet_nomination_pools::pool_migrate
- pallet_offences::report_offence_babe
- pallet_offences::report_offence_grandpa
Error: Input("5 benchmarks failed")
```

(after)
```pre
The following 1 benchmarks failed:
- pallet_nomination_pools::apply_slash_fail
Error: Input("1 benchmarks failed")
```
This one needs fixing but is not breaking the other ones anymore.

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Alexander Theißen <[email protected]>
Merged via the queue into master with commit e000a5c May 19, 2025
240 of 245 checks passed
@bkontur bkontur deleted the oty-omni-bencher-fixes branch May 19, 2025 17:40
paritytech-release-backport-bot bot pushed a commit that referenced this pull request May 19, 2025
@ggwpez @github-actions
@bkontur @athei
[FRAME] Omni bencher fixes (#8265)
233850f 
Changes:
- Add `--pallets` option to selectively run multiple pallets. In the
past we only had `--pallet` to run a single one.
- Add `--exclude-extrinsics [pallet::extrinsic]` to add
(Pallet,Extrinsic) tuples that should be excluded.
- Fix storage overlay reversion *before* the benchmark runs.
- Test root hash for V2 benchmarks to be deterministic
- <s>Changed DB reps to 1 for speedup since it should not be needed to
run multiple times. (TODO test)</s>

Checked that it fixes the Kusama benchmark issue when
[patching](https://github.com/ggwpez/substrate-scripts/blob/master/diener.py)
to a fixed stable2412 fork:

(before)
```pre
The following 5 benchmarks failed:
- pallet_nomination_pools::apply_slash_fail
- pallet_nomination_pools::migrate_delegation
- pallet_nomination_pools::pool_migrate
- pallet_offences::report_offence_babe
- pallet_offences::report_offence_grandpa
Error: Input("5 benchmarks failed")
```

(after)
```pre
The following 1 benchmarks failed:
- pallet_nomination_pools::apply_slash_fail
Error: Input("1 benchmarks failed")
```
This one needs fixing but is not breaking the other ones anymore.

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Alexander Theißen <[email protected]>
(cherry picked from commit e000a5c)
@paritytech-release-backport-bot
Copy link

Successfully created backport PR for stable2503:

@paritytech-release-backport-bot paritytech-release-backport-bot bot mentioned this pull request May 19, 2025
Merged
athei added a commit that referenced this pull request May 19, 2025
@paritytech-release-backport-bot @ggwpez
@github-actions @bkontur @athei
[stable2503] Backport #8265 (#8567)
1eeffac 
Backport #8265 into `stable2503` from ggwpez.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Alexander Theißen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkontur bkontur bkontur approved these changes

@athei athei athei approved these changes

@bkchr bkchr bkchr approved these changes

Assignees
No one assigned
Labels
A4-backport-stable2503 Pull request must be backported to the stable2503 release branch T12-benchmarks This PR/Issue is related to benchmarking and weights.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ggwpez @athei @bkchr @bkontur