frequency-chain / frequency

Frequency: A Polkadot Parachain

https://www.frequency.xyz

Apache License 2.0

48 stars 18 forks source link

Benchmark fixes #1971

Closed wilwade closed 1 month ago

wilwade commented 2 months ago

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[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

enddynayn commented 1 month ago

@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.

wilwade commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

Thanks for updating the Wiki.