Benchmark fixes #1971

wilwade · 2024-05-15T18:32:20Z

Goal

The goal of this PR is to fix an issue with running benchmarks and update our benchmarks with some tests.

Part of #1853

Issues

When I added the Proxy pallet, the copied weight file worked, but the generated version didn't.
Then when fixing that, the benchmark runners had issues. So updated the benchmark workflow by removing it from the commit commenting system to a manual run. Using that will also automatically trigger verify CI actions. (GitHub actions only run on Manually triggered actions to prevent loops)

Discussion

The additional changes to the templates is from Polkadot-SDK
Actual change is small, as most of this are the generated weights code
Fix issue with the proxy_pallet weights reference
Update the benchmark running system to not use special Github tokens

codecov · 2024-05-15T18:49:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

# Goal The goal of this PR is to create a new flow for benchmarks so that they are easier to run and do not require a separate github token. To test this must be in main as `workflow_dispatch` doesn't work without being in `main`, but here is a run that was triggered using defaults with a few other tweaks: https://github.com/frequency-chain/frequency/actions/runs/9174859575 # Discussion - The PR #1971 will remove the old system from the `.github/workflows/verify-pr-commit.yml` file so that only this one is used - Once PR #1971 is merged, I will also update the wiki with the new instructions

wilwade · 2024-05-21T17:41:59Z

.github/workflows/verify-pr-commit.yml

@@ -750,114 +754,3 @@ jobs:
          echo "Actual genesis state: $actual_genesis_state"
          [ $actual_genesis_state = $expected_genesis_state ] || \
            (echo "ERROR: The actual genesis state does not match the expected" && exit 1)
-


Also removed this from the required list for PRs.

wilwade · 2024-05-21T17:42:24Z

.maintain/frame-weight-template.hbs

+	{{#each benchmarks as |benchmark|}}
+	{{#if (ne benchmark.base_calculated_proof_size "0")}}
+	#[test]
+	fn test_{{benchmark.name~}} () {


Make sure the benchmarks actually work.

wilwade · 2024-05-21T17:43:31Z

.maintain/frame-weight-template.hbs

Minor updates from the polkadot-sdk patterns

wilwade · 2024-05-21T17:44:23Z

pallets/handles/Cargo.toml

Adding common-runtime under dev-dependencies to these so the weights tests work

wilwade · 2024-05-21T17:44:59Z

pallets/capacity/src/weights.rs

I am going ahead and merging the run with the tests.

wilwade · 2024-05-21T17:46:10Z

scripts/run_benchmarks.sh

@@ -224,3 +224,8 @@ then
  echo " "
  ${BENCHMARK} overhead --wasm-execution=compiled --weight-path=runtime/common/src/weights --chain=dev --warmup=10 --repeat=100 --header="./HEADER-APACHE2" || exit_err
 fi
+
+echo "Running tests..."


Run the tests after the benchmarks to make sure the benchmark generated code works.

.github/workflows/benchmarks-run.yml

mattheworris · 2024-05-22T14:19:59Z

runtime/common/src/weights/extrinsic_weights.rs

@@ -1,6 +1,6 @@
-// This file is part of Substrate.
+// This file is part of Frequency.


Nice, always bugged me that it said Substrate. 🤓

enddynayn · 2024-05-22T14:28:15Z

@wilwade Could you please update the commit message to include a description of the issue? It seems to be missing from the ticket as well. Including this information would make it easier to review the PR.

Additionally, as a friendly reminder, please keep PRs scoped to the issue at hand. It looks like there are some additional changes that might be out of scope for this particular fix.

enddynayn

great! 👍

wilwade · 2024-05-22T14:34:56Z

runtime/frequency/src/lib.rs

@@ -1068,7 +1068,7 @@ impl pallet_proxy::Config for Runtime {
 	type CallHasher = BlakeTwo256;
 	type AnnouncementDepositBase = AnnouncementDepositBase;
 	type AnnouncementDepositFactor = AnnouncementDepositFactor;
-	type WeightInfo = weights::pallet_proxy::WeightInfo<Runtime>;
+	type WeightInfo = weights::pallet_proxy::SubstrateWeight<Runtime>;


Fix for the proxy_pallet

wilwade · 2024-05-22T14:39:19Z

@wilwade Could you please update the commit message to include a description of the issue? It seems to be missing from the ticket as well. Including this information would make it easier to review the PR.

Additionally, as a friendly reminder, please keep PRs scoped to the issue at hand. It looks like there are some additional changes that might be out of scope for this particular fix.

Updated the issue and the PR description to make it easier to understand why these were combined. (tldr: Couldn't run the benchmarks to verify the fix without greater changes)

enddynayn · 2024-05-22T14:42:11Z

@wilwade Could you please update the commit message to include a description of the issue? It seems to be missing from the ticket as well. Including this information would make it easier to review the PR.
Additionally, as a friendly reminder, please keep PRs scoped to the issue at hand. It looks like there are some additional changes that might be out of scope for this particular fix.

Updated the issue and the PR description to make it easier to understand why these were combined. (tldr: Couldn't run the benchmarks to verify the fix without greater changes)

Thank you so much!

enddynayn · 2024-05-22T14:45:43Z

Thanks for updating the Wiki.

mattheworris

Read through changes

🚢 it!

shannonwells

Read through the code, read through comments/questions...good to go

wilwade force-pushed the chore/fix-benchmarks branch 19 times, most recently from 3cafc3c to 0ec27a3 Compare May 20, 2024 19:16

github-actions bot pushed a commit that referenced this pull request May 20, 2024

Update weights for PR #1971

65b20f7

wilwade force-pushed the chore/fix-benchmarks branch from 65b20f7 to 2d4b74e Compare May 20, 2024 21:17

github-actions bot pushed a commit that referenced this pull request May 20, 2024

[actions skip] Update weights for PR #1971

d15cc94

wilwade force-pushed the chore/fix-benchmarks branch from d15cc94 to 5ae17eb Compare May 21, 2024 12:40

wilwade mentioned this pull request May 21, 2024

New benchmark workflow #1975

Merged

wilwade added 3 commits May 21, 2024 09:58

Fixing issue with pallet_proxy weights

eb49331

Adding in basic tests for benchmarks

0c1c5f7

Remove old benchmarks workflow

cca5142

wilwade force-pushed the chore/fix-benchmarks branch from c7155b0 to cca5142 Compare May 21, 2024 13:59

wilwade commented May 21, 2024

View reviewed changes

.maintain/frame-weight-template.hbs

Copy link

Collaborator Author

wilwade May 21, 2024

Choose a reason for hiding this comment

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

Minor updates from the polkadot-sdk patterns

wilwade commented May 21, 2024

View reviewed changes

pallets/handles/Cargo.toml

Copy link

Collaborator Author

wilwade May 21, 2024

Choose a reason for hiding this comment

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

Adding common-runtime under dev-dependencies to these so the weights tests work

wilwade commented May 21, 2024

View reviewed changes

pallets/capacity/src/weights.rs

Copy link

Collaborator Author

wilwade May 21, 2024

Choose a reason for hiding this comment

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

I am going ahead and merging the run with the tests.

wilwade commented May 21, 2024

View reviewed changes

wilwade marked this pull request as ready for review May 21, 2024 17:47

wilwade requested a review from demisx as a code owner May 21, 2024 17:47

wilwade requested review from a team, shannonwells, mattheworris, enddynayn, aramikm, claireolmstead and JoeCap08055 and removed request for a team May 21, 2024 17:47

mattheworris reviewed May 22, 2024

View reviewed changes

.github/workflows/benchmarks-run.yml Show resolved Hide resolved

mattheworris reviewed May 22, 2024

View reviewed changes

enddynayn approved these changes May 22, 2024

View reviewed changes

wilwade mentioned this pull request May 22, 2024

Maintenance on benchmarks #1853

Closed

4 tasks

wilwade commented May 22, 2024

View reviewed changes

mattheworris approved these changes May 22, 2024

View reviewed changes

wilwade enabled auto-merge (squash) May 22, 2024 14:58

shannonwells approved these changes May 22, 2024

View reviewed changes

wilwade merged commit 0db0a79 into main May 22, 2024
28 checks passed

wilwade deleted the chore/fix-benchmarks branch May 22, 2024 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Benchmark fixes #1971

Benchmark fixes #1971

wilwade commented May 15, 2024 •

edited

Loading

codecov bot commented May 15, 2024

wilwade May 21, 2024

wilwade May 21, 2024

wilwade May 21, 2024

wilwade May 21, 2024

wilwade May 21, 2024

wilwade May 21, 2024

mattheworris May 22, 2024

enddynayn commented May 22, 2024 •

edited

Loading

enddynayn left a comment

wilwade May 22, 2024

wilwade commented May 22, 2024

enddynayn commented May 22, 2024

enddynayn commented May 22, 2024

mattheworris left a comment

shannonwells left a comment

		@@ -1,6 +1,6 @@
		// This file is part of Substrate.
		// This file is part of Frequency.

Benchmark fixes #1971

Benchmark fixes #1971

Conversation

wilwade commented May 15, 2024 • edited Loading

Goal

Issues

Discussion

codecov bot commented May 15, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enddynayn commented May 22, 2024 • edited Loading

enddynayn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilwade commented May 22, 2024

enddynayn commented May 22, 2024

enddynayn commented May 22, 2024

mattheworris left a comment

Choose a reason for hiding this comment

shannonwells left a comment

Choose a reason for hiding this comment

wilwade commented May 15, 2024 •

edited

Loading

enddynayn commented May 22, 2024 •

edited

Loading