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

Benchmark fixes #1971

Merged
merged 9 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 2 additions & 29 deletions .github/workflows/benchmarks-run.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# yaml-language-server: $schema=https://json.schemastore.org/github-workflow.json
name: Benchmarks Run
run-name: Benchmarks Run for ${{github.event.inputs.branch}}
concurrency:
group: ${{github.workflow}}-${{github.ref}}
cancel-in-progress: true
Expand Down Expand Up @@ -32,22 +33,6 @@ jobs:
uses: actions/checkout@v4
with:
ref: ${{github.event.inputs.branch}}
- name: Get Current Job Log URL
id: jobs
uses: Tiryoh/gha-jobid-action@c1d1cf7334b70c29374ae382b91053674c8049a2
with:
github_token: ${{ github.token }}
job_name: "Run Benchmarks"
- name: Set PR Commit Status to Pending
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Commit statuses don't work when triggered by workflow_dispatch

uses: ouzi-dev/commit-status-updater@219d3f932547cad092e384c7a36bf4d963739c35
with:
name: "GithubActions - Run Benchmarks"
status: "pending"
addHoldComment: "true"
pendingComment: >-
:hourglass_flowing_sand: [Running benchmarks](${{steps.jobs.outputs.html_url}})
and calculating weights. DO NOT MERGE! A new commit will be added upon completion...
failComment: "ERROR: Failed to set commit to pending status!"
- name: Install Required Packages
run: |
sudo apt-get update
Expand Down Expand Up @@ -80,17 +65,5 @@ jobs:
id: commit-updated-weights
uses: stefanzweifel/git-auto-commit-action@8756aa072ef5b4a080af5dc8fef36c5d586e521d
with:
commit_message: "Update weights for #${{github.event.inputs.branch}}"
commit_message: "Update weights"
mattheworris marked this conversation as resolved.
Show resolved Hide resolved
file_pattern: "pallets/**/*.rs runtime/common/src/weights/*"

- name: Set Commit Status to Success
uses: ouzi-dev/commit-status-updater@219d3f932547cad092e384c7a36bf4d963739c35
with:
name: "GithubActions - Run Benchmarks"
status: "success"
addHoldComment: "true"
successComment: >-
:white_check_mark: [Finished running benchmarks](${{steps.jobs.outputs.html_url}}).
Updated weights have been committed to this branch in
commit ${{steps.commit-updated-weights.outputs.commit_hash}}.
failComment: "ERROR: Failed to set commit to success status!"
4 changes: 3 additions & 1 deletion .github/workflows/create-issue-dependabot-alert-pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ on:
jobs:
create-issue:
runs-on: ubuntu-22.04
permissions:
contents: write
env:
GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}
GITHUB_TOKEN: ${{ github.token }}
Comment on lines -15 to +17
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Final removal of the custom GITHUB token

PR_TITLE: ${{github.event.pull_request.title}}
PR_NUMBER: ${{github.event.pull_request.number}}
PR_URL: ${{github.event.pull_request.url}}
Expand Down
16 changes: 7 additions & 9 deletions .github/workflows/release.yml
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll need to run a test release once this merges to double check this workflow

Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ on:

permissions:
contents: write
statuses: write

env:
NEW_RELEASE_TAG_FROM_UI: ${{github.event.inputs.release-version}}
Expand Down Expand Up @@ -94,8 +95,8 @@ jobs:
if: steps.is-full-release.outputs.is-full-release == 'true'
# Match installation steps to CI base docker image
run: |
curl https://sh.rustup.rs -sSf | bash -s -- -y
echo "PATH=$HOME/.cargo/bin:$PATH" >> $GITHUB_ENV
curl https://sh.rustup.rs -sSf | HOME=`pwd` bash -s -- -y
echo "PATH=`pwd`/.cargo/bin:$PATH" >> $GITHUB_ENV
Comment on lines -97 to +99
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why the pwd? Because the runner only allows modification inside of the specific folder.

- name: Update Weights for All Pallets
if: steps.is-full-release.outputs.is-full-release == 'true'
run: |
Expand All @@ -108,9 +109,7 @@ jobs:
uses: stefanzweifel/git-auto-commit-action@8756aa072ef5b4a080af5dc8fef36c5d586e521d
with:
commit_message: "Update weights for release ${{env.NEW_RELEASE_TAG}}"
commit_user_name: Frequency CI [bot]
commit_user_email: do-not-reply@users.noreply.github.com
commit_author: Frequency CI [bot] <do-not-reply@users.noreply.github.com>
file_pattern: "pallets/**/*.rs runtime/common/src/weights/*"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Limit to just the weights, and remove the commit user as it will just use the default bot+trigger person


version-code:
needs: run-all-benchmarks
Expand All @@ -136,9 +135,6 @@ jobs:
uses: stefanzweifel/git-auto-commit-action@8756aa072ef5b4a080af5dc8fef36c5d586e521d
with:
commit_message: "Update versions for release ${{env.NEW_RELEASE_TAG}}"
commit_user_name: Frequency CI [bot]
commit_user_email: do-not-reply@users.noreply.github.com
commit_author: Frequency CI [bot] <do-not-reply@users.noreply.github.com>
- name: Update Release Version Tag
uses: rickstaa/action-create-tag@88dbf7ff6fe2405f8e8f6c6fdfd78829bc631f83
with:
Expand Down Expand Up @@ -651,6 +647,8 @@ jobs:
name: Release Built Artifacts
runs-on: ubuntu-22.04
container: ghcr.io/frequency-chain/frequency/ci-base-image:1.3.1
permissions:
contents: write
steps:
- name: Check Out Repo
uses: actions/checkout@v4
Expand Down Expand Up @@ -682,7 +680,7 @@ jobs:
id: build-changelog
uses: mikepenz/release-changelog-builder-action@6fd5cc6eaf7567dbd0f9666061215bb476f012fc
env:
GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}
GITHUB_TOKEN: ${{ github.token }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

contents: write means we can use the github.token

with:
fromTag: ${{env.PREVIOUS_RELEASE_TAG}}
toTag: ${{env.NEW_RELEASE_TAG}}
Expand Down
123 changes: 8 additions & 115 deletions .github/workflows/verify-pr-commit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,18 +56,20 @@ jobs:
clear-metadata-labels:
name: Clear Metadata Labels
runs-on: ubuntu-22.04
permissions:
pull-requests: write
steps:
- name: Clear Metadata Changed Label
if: contains(github.event.pull_request.labels.*.name, env.PR_LABEL_METADATA_CHANGED)
uses: RobinJesba/GitHub-Labeler-Action@2f69380bbf2ee60b2f0893ef0f40582c9a34a64d
with:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GITHUB_TOKEN: ${{ github.token }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Works with pull-requests: write

LABELS_TO_REMOVE: ${{env.PR_LABEL_METADATA_CHANGED}}
- name: Clear Metadata Version Not Incremented Label
if: contains(github.event.pull_request.labels.*.name, env.PR_LABEL_METADATA_VERSION_NOT_INCREMENTED)
uses: RobinJesba/GitHub-Labeler-Action@2f69380bbf2ee60b2f0893ef0f40582c9a34a64d
with:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GITHUB_TOKEN: ${{ github.token }}
LABELS_TO_REMOVE: ${{env.PR_LABEL_METADATA_VERSION_NOT_INCREMENTED}}

# Workaround to handle skipped required check inside matrix
Expand Down Expand Up @@ -541,6 +543,8 @@ jobs:
needs: build-binaries
name: Check Metadata and Spec Version
runs-on: ubuntu-22.04
permissions:
pull-requests: write
steps:
- name: Check Out Repo
uses: actions/checkout@v4
Expand Down Expand Up @@ -583,7 +587,7 @@ jobs:
if: steps.compare-metadata.outputs.metadata_matches != 'true'
uses: RobinJesba/GitHub-Labeler-Action@2f69380bbf2ee60b2f0893ef0f40582c9a34a64d
with:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GITHUB_TOKEN: ${{ github.token }}
LABELS_TO_ADD: ${{env.PR_LABEL_METADATA_CHANGED}}
- name: Check Spec Version
if: steps.compare-metadata.outputs.metadata_matches != 'true'
Expand All @@ -603,7 +607,7 @@ jobs:
(steps.check-spec-version.outputs.metadata_version_incremented != 'true')
uses: RobinJesba/GitHub-Labeler-Action@2f69380bbf2ee60b2f0893ef0f40582c9a34a64d
with:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GITHUB_TOKEN: ${{ github.token }}
LABELS_TO_ADD: ${{env.PR_LABEL_METADATA_VERSION_NOT_INCREMENTED}}
- name: Fail CI
if: |
Expand Down Expand Up @@ -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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also removed this from the required list for PRs.

should-run-benchmarks:
if: github.repository == 'frequency-chain/frequency'
name: Run Benchmarks?
runs-on: ubuntu-22.04
container: ghcr.io/frequency-chain/frequency/ci-base-image:1.3.1
outputs:
should_run_benchmarks: ${{steps.run-benchmarks.outputs.run}}
pallets: ${{steps.run-benchmarks.outputs.pallets}}
steps:
- name: Check Out Repo
uses: actions/checkout@v4
with:
ref: ${{github.event.pull_request.head.sha}}
- name: Run Benchmarks?
id: run-benchmarks
shell: bash
run: |
git config --global --add safe.directory /__w/frequency/frequency
commit_message=$(git show -s --format=%s)
echo "commit_message: $commit_message"
regex='\[run-benchmarks (.*)\]$'
if [[ $commit_message =~ $regex ]]; then
echo "Yes, need to run benchmarks."
pallets="${BASH_REMATCH[1]}"
echo "Pallets: $pallets"
echo "run=true" >> $GITHUB_OUTPUT
echo "pallets=$pallets" >> $GITHUB_OUTPUT
else
echo "No, do not need to run benchmarks."
echo "run=false" >> $GITHUB_OUTPUT
fi

run-benchmarks:
if: github.repository == 'frequency-chain/frequency' &&
needs.should-run-benchmarks.outputs.should_run_benchmarks == 'true'
needs: should-run-benchmarks
name: Run Benchmarks
runs-on: [self-hosted, Linux, X64, benchmark]
steps:
- name: Print Info
run: |
echo "Running benchmarks..."
echo "Pallets: ${{needs.should-run-benchmarks.outputs.pallets}}"
- name: Check Out Repo
uses: actions/checkout@v4
with:
token: ${{secrets.GHA_GIT_COMMIT || github.token}}
ref: ${{github.event.pull_request.head.sha}}
- name: Get Current Job Log URL
id: jobs
uses: Tiryoh/gha-jobid-action@7528cee9716384209bafcbd000d6689e851c5dda
with:
github_token: ${{secrets.GITHUB_TOKEN}}
job_name: "Run Benchmarks"
- name: Set PR Commit Status to Pending
uses: ouzi-dev/commit-status-updater@219d3f932547cad092e384c7a36bf4d963739c35
with:
name: "GithubActions - Run Benchmarks"
status: "pending"
addHoldComment: "true"
pendingComment: >-
:hourglass_flowing_sand: [Running benchmarks](${{steps.jobs.outputs.html_url}})
and calculating weights. DO NOT MERGE! A new commit will be added upon completion...
failComment: "ERROR: Failed to set PR commit to pending status!"
- name: Install Required Packages
run: |
sudo apt-get update
sudo apt install -y protobuf-compiler libclang-dev clang cmake
- name: Install Rust Toolchain
# Match installation steps to CI base docker image
run: |
curl https://sh.rustup.rs -sSf | bash -s -- -y
echo "PATH=$HOME/.cargo/bin:$PATH" >> $GITHUB_ENV
- name: Update Weights
run: |
rustup show
pallets_str="${{needs.should-run-benchmarks.outputs.pallets}}"
echo "Pallets: $pallets_str"
if [ -z "${pallets_str}" -o $pallets_str = 'all' ]; then
echo "Running benchmarks for all pallets..."
make benchmarks
else
IFS=',' read -r -a pallets <<< "$pallets_str"
echo "Running benchmarks for pallets: ${pallets[*]}..."
make benchmarks-multi PALLETS="${pallets[*]}"
echo "Finished benchmarks for pallets: ${pallets[*]}."
fi
- name: Print Updated Weights
run: |
git status
git diff
- name: Commit Updated Weights
id: commit-updated-weights
uses: stefanzweifel/git-auto-commit-action@8756aa072ef5b4a080af5dc8fef36c5d586e521d
with:
commit_message: "Update weights for PR #${{github.event.pull_request.number}}"
commit_user_name: Frequency CI [bot]
commit_user_email: do-not-reply@users.noreply.github.com
commit_author: Frequency CI [bot] <do-not-reply@users.noreply.github.com>
- name: Set PR Commit Status to Success
uses: ouzi-dev/commit-status-updater@219d3f932547cad092e384c7a36bf4d963739c35
with:
name: "GithubActions - Run Benchmarks"
status: "success"
addHoldComment: "true"
successComment: >-
:white_check_mark: [Finished running benchmarks](${{steps.jobs.outputs.html_url}}).
Updated weights have been committed to this PR branch in
commit ${{steps.commit-updated-weights.outputs.commit_hash}}.
failComment: "ERROR: Failed to set PR commit to success status!"
68 changes: 46 additions & 22 deletions .maintain/frame-weight-template.hbs
Copy link
Collaborator Author

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

Original file line number Diff line number Diff line change
@@ -1,27 +1,11 @@
// This file is part of Substrate.

// Copyright (C) 2022 Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//! Autogenerated weights for {{pallet}}
{{header}}
//! Autogenerated weights for `{{pallet}}`
//!
//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION {{version}}
//! DATE: {{date}}, STEPS: `{{cmd.steps}}`, REPEAT: `{{cmd.repeat}}`, LOW RANGE: `{{cmd.lowest_range_values}}`, HIGH RANGE: `{{cmd.highest_range_values}}`
//! WORST CASE MAP SIZE: `{{cmd.worst_case_map_values}}`
//! HOSTNAME: `{{hostname}}`, CPU: `{{cpuname}}`
//! EXECUTION: {{cmd.execution}}, WASM-EXECUTION: {{cmd.wasm_execution}}, CHAIN: {{cmd.chain}}, DB CACHE: {{cmd.db_cache}}
//! WASM-EXECUTION: `{{cmd.wasm_execution}}`, CHAIN: `{{cmd.chain}}`, DB CACHE: `{{cmd.db_cache}}`

// Executed Command:
{{#each args as |arg|}}
Expand All @@ -36,7 +20,7 @@
use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};
use core::marker::PhantomData;

/// Weight functions needed for {{pallet}}.
/// Weight functions needed for `{{pallet}}`.
pub trait WeightInfo {
{{#each benchmarks as |benchmark|}}
fn {{benchmark.name~}}
Expand All @@ -47,7 +31,7 @@ pub trait WeightInfo {
{{/each}}
}

/// Weights for {{pallet}} using the Substrate node and recommended hardware.
/// Weights for `{{pallet}}` using the Substrate node and recommended hardware.
pub struct SubstrateWeight<T>(PhantomData<T>);
{{#if (eq pallet "frame_system")}}
impl<T: crate::Config> WeightInfo for SubstrateWeight<T> {
Expand Down Expand Up @@ -94,7 +78,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
{{/each}}
}

// For backwards compatibility and tests
// For backwards compatibility and tests.
impl WeightInfo for () {
{{#each benchmarks as |benchmark|}}
{{#each benchmark.comments as |comment|}}
Expand Down Expand Up @@ -135,3 +119,43 @@ impl WeightInfo for () {
}
{{/each}}
}


#[cfg(test)]
mod tests {
use frame_support::{traits::Get, weights::Weight, dispatch::DispatchClass};
use common_runtime::constants::{MAXIMUM_BLOCK_WEIGHT, NORMAL_DISPATCH_RATIO};
use common_runtime::weights::extrinsic_weights::ExtrinsicBaseWeight;

struct BlockWeights;
impl Get<frame_system::limits::BlockWeights> for BlockWeights {
fn get() -> frame_system::limits::BlockWeights {
frame_system::limits::BlockWeights::builder()
.base_block(Weight::zero())
.for_class(DispatchClass::all(), |weights| {
weights.base_extrinsic = ExtrinsicBaseWeight::get().into();
})
.for_class(DispatchClass::non_mandatory(), |weights| {
weights.max_total = Some(NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT);
})
.build_or_panic()
}
}

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

Choose a reason for hiding this comment

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

Make sure the benchmarks actually work.

assert!(
BlockWeights::get()
.per_class
.get(frame_support::dispatch::DispatchClass::Normal)
.max_extrinsic
.unwrap_or_else(<Weight as sp_runtime::traits::Bounded>::max_value)
.proof_size()
> {{benchmark.base_calculated_proof_size}}
);
}
{{/if}}
{{/each}}
}
Loading