-
Notifications
You must be signed in to change notification settings - Fork 845
[ci] Support running re-execution benchmark with arbitrary version of Firewood #4650
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
base: master
Are you sure you want to change the base?
Changes from 63 commits
d0155dd
e851903
49b7fc3
2feeda4
6f21231
b3cc8ed
e437658
15b52ee
999c3f4
34a331f
cb5e59a
e0e784b
cab333e
c131420
f79c8d5
c4561be
a89c44e
d428155
87b3d4c
6f5502b
6493598
0451870
8cf656d
cfe4960
1416e52
0a7185a
48bdef7
279e120
325e42b
a2d7b8c
73d4057
4c4d3eb
af62634
b81d12f
2ecec16
cd4a9d7
1ef0920
05c1b8d
e1be9e1
5426ee8
2b86559
90fd09a
73c876e
a92e465
81b88b6
6f944d1
8a92625
db41178
f410444
d619a21
954121b
d9b2717
6076c4f
221157a
6aef9df
c28cd33
2ac5157
1f06a14
6ad2f13
e3ab8ad
01da31a
e463b6a
dac5262
a29c22c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,9 @@ on: | |
| timeout-minutes: | ||
| description: 'Timeout in minutes for the job.' | ||
| default: 30 | ||
| with-dependencies: | ||
| description: 'Dependencies to use (e.g., "firewood=abc123" or "firewood=abc,libevm=xyz")' | ||
| default: '' | ||
|
|
||
| # Disabled because scheduled trigger is empty. To enable, uncomment and add at least one vector to the schedule | ||
| # entry in the corresponding JSON file. | ||
|
|
@@ -49,17 +52,22 @@ jobs: | |
| shell: bash -x {0} | ||
| run: | | ||
| if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then | ||
| FIREWOOD_REF="" LIBEVM_REF="" | ||
| [[ "${{ github.event.inputs.with-dependencies }}" =~ firewood=([^,]+) ]] && FIREWOOD_REF="${BASH_REMATCH[1]}" | ||
| [[ "${{ github.event.inputs.with-dependencies }}" =~ libevm=([^,]+) ]] && LIBEVM_REF="${BASH_REMATCH[1]}" | ||
| { | ||
| echo "matrix<<EOF" | ||
| printf '{ "include": [{ "test": "%s", "start-block": "%s", "end-block": "%s", "block-dir-src": "%s", "current-state-dir-src": "%s", "config": "%s", "runner": "%s", "timeout-minutes": %s }] }\n' \ | ||
| printf '{ "include": [{ "test": "%s", "start-block": "%s", "end-block": "%s", "block-dir-src": "%s", "current-state-dir-src": "%s", "config": "%s", "runner": "%s", "timeout-minutes": %s, "firewood-ref": "%s", "libevm-ref": "%s" }] }\n' \ | ||
| "${{ github.event.inputs.test }}" \ | ||
| "${{ github.event.inputs.start-block }}" \ | ||
| "${{ github.event.inputs.end-block }}" \ | ||
| "${{ github.event.inputs.block-dir-src }}" \ | ||
| "${{ github.event.inputs.current-state-dir-src }}" \ | ||
| "${{ github.event.inputs.config }}" \ | ||
| "${{ github.event.inputs.runner }}" \ | ||
| "${{ github.event.inputs.timeout-minutes }}" | ||
| "${{ github.event.inputs.timeout-minutes }}" \ | ||
| "$FIREWOOD_REF" \ | ||
| "$LIBEVM_REF" | ||
| echo EOF | ||
| } >> "$GITHUB_OUTPUT" | ||
| else | ||
|
|
@@ -82,8 +90,6 @@ jobs: | |
| id-token: write | ||
| contents: write | ||
| runs-on: ${{ matrix.runner }} | ||
| container: | ||
Elvis339 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| image: ghcr.io/actions/actions-runner:2.325.0 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Install ARC Dependencies | ||
|
|
@@ -94,6 +100,22 @@ jobs: | |
| sudo apt-get update | ||
| sudo apt-get install -y xz-utils | ||
| fi | ||
| # Nix is required before calling polyrepo | ||
| # c-chain-reexecution-benchmark's Nix install will be skipped; installation is idempotent. | ||
| - name: Install Nix | ||
| if: matrix.libevm-ref != '' || matrix.firewood-ref != '' | ||
| uses: cachix/install-nix-action@02a151ada4993995686f9ed4f1be7cfbb229e56f #v31 | ||
| with: | ||
| github_access_token: ${{ secrets.GITHUB_TOKEN }} | ||
| - name: Configure dependency versions | ||
| if: matrix.libevm-ref != '' || matrix.firewood-ref != '' | ||
| shell: nix develop --command bash {0} | ||
| run: ./scripts/run_task.sh polyrepo -- ${{ matrix.firewood-ref != '' && format('sync firewood@{0}', matrix.firewood-ref) || '' }} | ||
| env: | ||
| LIBEVM_REF: ${{ matrix.libevm-ref }} | ||
| # Required for Nix builds on ARC runners | ||
| TMPDIR: /tmp | ||
| NIX_BUILD_TOP: /tmp | ||
| - name: Run C-Chain Re-Execution Benchmark | ||
| uses: ./.github/actions/c-chain-reexecution-benchmark | ||
| with: | ||
|
|
@@ -107,7 +129,8 @@ jobs: | |
| prometheus-push-url: ${{ secrets.PROMETHEUS_PUSH_URL || '' }} | ||
| prometheus-username: ${{ secrets.PROMETHEUS_USERNAME || '' }} | ||
| prometheus-password: ${{ secrets.PROMETHEUS_PASSWORD || '' }} | ||
| push-github-action-benchmark: ${{ github.event_name == 'schedule' || (github.event_name == 'workflow_dispatch' && github.repository == 'ava-labs/avalanchego' && github.ref_name == 'master') }} | ||
| push-github-action-benchmark: ${{ (github.event_name == 'schedule' || (github.event_name == 'workflow_dispatch' && github.repository == 'ava-labs/avalanchego' && github.ref_name == 'master')) && matrix.firewood-ref == '' }} | ||
| skip-benchmark-comparison: ${{ matrix.firewood-ref != '' || matrix.libevm-ref != '' }} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these values the trigger instead of the dirty tree state?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two separate concerns:
Why input values instead of dirty tree check? I'm okay with adding automatic dirty tree detection inside |
||
| aws-role: ${{ github.event.inputs.push-post-state != '' && secrets.AWS_S3_RW_ROLE || secrets.AWS_S3_READ_ONLY_ROLE }} | ||
| aws-region: 'us-east-2' | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| # Run polyrepo tool for managing local dependencies. | ||
| # | ||
| # Usage: | ||
| # ./scripts/run_polyrepo.sh [polyrepo args...] | ||
| # | ||
| # Environment variables (optional): | ||
| # LIBEVM_REF - Git ref for libevm (runs: go get && go mod tidy) | ||
|
||
| # | ||
| # Examples: | ||
| # ./scripts/run_polyrepo.sh sync firewood@abc123 # sync firewood | ||
| # ./scripts/run_polyrepo.sh status # check status | ||
| # LIBEVM_REF=v1.2.3 ./scripts/run_polyrepo.sh sync firewood@abc123 # both | ||
|
|
||
| POLYREPO_REVISION=6239973c9b | ||
|
|
||
| if [[ -n "${LIBEVM_REF:-}" ]]; then | ||
| echo "Updating libevm to ${LIBEVM_REF}..." | ||
| go get "github.com/ava-labs/libevm@${LIBEVM_REF}" | ||
| go mod tidy | ||
| fi | ||
|
|
||
| if [[ $# -gt 0 ]]; then | ||
| go run github.com/ava-labs/avalanchego/tests/fixture/polyrepo@"${POLYREPO_REVISION}" "${@}" | ||
| fi | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When custom dependencies (firewood-ref or libevm-ref) are used,
go.mod/go.sum are modified. This causes github-action-benchmark
to fail when checking out gh-pages.
New input skips comparison step only when custom deps are present,
preserving summary display for normal PR runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to make this skip automatic with an appropriate warning if the tree is dirty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inputs already tell us when the tree will be dirty if
firewood-reforlibevm-refis provided,polyrepo/go getwill modifygo.mod. A runtimegit statuscheck would just confirm what we already know statically.Putting the detection logic inside the action would also conflict with Aaron's constraint he was okay with the action accepting
skip-benchmark-comparisonas an input, but the decision of when to skip should be made by the caller, not the action itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronbuchwald Why is it desirable to have to pass in a synthesized variable instead of just detecting this condition locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding context on the design. Polyrepo is a composable layer that sits before the benchmark action, setting up deps. The base action runs benchmarks on whatever code is present, then uploads artifacts (Firewood will track performance on their own gh-pages).
Aaron's constraint as I understood it is that the action should run reexecution for the environment that's present not be responsible for in-place modification or inferring dependency state.
This matters because if the action detects dirty tree and decides to skip, it's making decisions about dependency state without knowing why the tree is dirty. The caller (workflow) knows it ran polyrepo → knows the tree is dirty → passes skip-benchmark-comparison: true.
This keeps concerns separated dep setup in workflow, benchmarking in action.
I might be wrong in how I inferred Aaron's constraint happy for him to correct me. Open to doing whatever to progress with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original constraint is that the custom action should not need to know about the special case of firewood or overwriting dependencies. This seems a better separation of responsibilities to me than to pass in firewood-ref, libevm-ref, etc. which imo is better handled as a separate step.
I am not sure why
skip-benchmark-comparisonneeds to be added as opposed to simply settingpush-github-action-benchmarkto false.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that github-action-benchmark fails entirely when the tree is dirty - it tries to checkout gh-pages and errors on the dirty working tree. Setting push-github-action-benchmark: false still runs the step, which fails. We need to skip the step entirely.