[ci] Support running re-execution benchmark with arbitrary version of Firewood #4650
Conversation
There was a problem hiding this comment.
Pull request overview
This PR establishes infrastructure for tracking Firewood performance over time by enabling C-Chain reexecution benchmarks with custom Firewood builds. The workflow can be triggered from the Firewood repository with either published versions (for quick testing) or branch/commit references (for comprehensive testing with source builds).
Key changes:
- Adds a reusable workflow that accepts Firewood version/branch/commit as input
- Implements intelligent build strategy: uses
go getfor published versions, builds from source for branches/commits - Creates build script for compiling Firewood FFI from source using Nix
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/workflows/c-chain-reexecution-benchmark-firewood.yml |
New workflow that orchestrates benchmark execution with custom Firewood builds and uploads results as artifacts |
graft/coreth/scripts/build_firewood.sh |
Shell script to clone, build, and optionally integrate Firewood FFI from source |
.github/workflows/c-chain-reexecution-benchmark-container.yml |
Removes container configuration (unrelated cleanup) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…/avalanchego into es/enable-firewood-dev-workflow
…ution benchmarks Integrate firewood/libevm dependency overrides into existing workflows using polyrepo, eliminating the need for a separate firewood workflow. - Add LIBEVM_REF/FIREWOOD_REF to reexecute-cchain-range-with-copied-data - Update composite action and workflows to pass inputs - Remove redundant firewood workflow and build_firewood.sh script
…/avalanchego into es/enable-firewood-dev-workflow
|
Functionally this looks ok. Doc question and lint failure will need to be addressed before merge. Also, in addition to the command that you ran to verify that this is working, please include a passing test run that resulted from that command (this is general best practice when manual testing is required). |
|
I guess the 10 input params is an indication that this remote execution can't be much more than a stop-gap pending firewood migration to a monorepo? |
I wasn't aware of the 10 input param limit first time hitting this constraint.
That said, both options are effectively stop-gaps. (1) is temporary until Firewood joins the monorepo, and (2) would still require cross-repo coordination that becomes unnecessary once Firewood is grafted. If you're okay with keeping (1) as a stop-gap for now with a GH issue to track the long-term solution, that would unblock the Firewood team to start collecting performance metrics sooner. |
I'm fine with that, as per my use of the term 'stop-gap' - a temporary solution pending a better one. |
|
Executed: Result: https://github.com/ava-labs/avalanchego/actions/runs/20240259592 |
maru-ava
left a comment
maru-ava
left a comment
There was a problem hiding this comment.
The comments I made on the container workflow also apply to the container one.
| runner: | ||
| description: 'Runner to execute the benchmark. Input to the runs-on field of the job.' | ||
| required: true | ||
| push-post-state: |
There was a problem hiding this comment.
I see that the job definitions have been updated to use a matrix field instead of this import parameter, what are the implications of this change?
There was a problem hiding this comment.
The main implication is that push-post-state can't be set from manual workflow_dispatch anymore you'd have to add it to the JSON config file instead.
Scheduled runs still work fine since they read from the JSON anyway.
We had to make this trade-off because GitHub has a 10 input limit for workflow_dispatch, and we needed room for firewood-ref and libevm-ref. Figured those are more useful for manual runs (testing custom versions), while push-post-state is typically used in scheduled/automated runs anyway.
If you ever need to manually push state after a run, you can still do it with:
task export-dir-to-s3 SRC=/path/to/current-state DST=s3://bucket/destination| push-post-state: ${{ matrix.push-post-state || '' }} | ||
| runner_name: ${{ matrix.runner }} | ||
| - name: Upload benchmark results | ||
| if: inputs.firewood != '' |
There was a problem hiding this comment.
Maybe add a command comment here indicating why a value for this parameter should prevent uploading?
There was a problem hiding this comment.
Added it here:
but forgot to add it to concrete implementations, thanks for pointing it out.…ref and firewood-ref
|
Re: #4650 (comment)
The image was originally used because it was minimal and we didn't want to maintain our own ubuntu base was missing sudo and the initial ARC implementation needed some baseline. SREs and Infra have been iterating on it since then. |
maru-ava
left a comment
maru-ava
left a comment
There was a problem hiding this comment.
I'm ok with this, but I think @aaronbuchwald will need to confirm that these changes are in keeping with his expectations.
Why this should be merged
Enables Firewood to track performance over time by running C-Chain reexecution benchmarks with custom Firewood builds. This establishes the infrastructure for catching performance regressions before they reach production.
ava-labs/firewood#1494
How this works
firewoodandlibevmparametersThe same functionality is available locally for development:
Changes
LIBEVM_REF/FIREWOOD_REFsupport toreexecute-cchain-range-with-copied-datataskc-chain-reexecution-benchmark-firewood.ymlworkflowbuild_firewood.shscript (replaced by polyrepo)How this was tested
gh workflow run "C-Chain Re-Execution Benchmark w/ Container" \ --ref es/enable-firewood-dev-workflow \ -f task=c-chain-reexecution-firewood-101-250k \ -f firewood-ref=v0.0.15 \ -f runner=avalanche-avalanchego-runner-2ti \ -f timeout-minutes=60Need to be documented in RELEASES.md?
No