-
Notifications
You must be signed in to change notification settings - Fork 823
chore: task reexecution #4443
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?
chore: task reexecution #4443
Conversation
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.
Pull Request Overview
This PR refactors the C-Chain re-execution benchmark infrastructure to use externalized configuration and a custom GitHub action that doesn't rely on relative imports. The changes streamline task configuration by moving runtime variables to environment-based configuration and introduce predefined task configurations for common benchmark scenarios.
- Externalized runtime variables to environment configuration rather than explicit task parameters
- Added custom GitHub action for C-Chain re-execution benchmarks with improved dependency management
- Created predefined task configurations for common benchmark scenarios (hashdb, firewood, different block ranges)
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/copy_dir.sh | Enhanced to support S3 object key expansion for simplified source specification |
| scripts/benchmark_cchain_range.sh | Added default values for optional variables and improved environment variable documentation |
| Taskfile.yml | Refactored task configuration to use externalized variables and added predefined benchmark tasks |
| .github/actions/run-monitored-tmpnet-cmd/output-metrics-url.sh | Added warning comment about file duplication |
| .github/actions/c-chain-reexecution-benchmark/output-metrics-url.sh | New duplicate of metrics URL generation script |
| .github/actions/c-chain-reexecution-benchmark/action.yml | New custom GitHub action for C-Chain re-execution benchmarks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/actions/c-chain-reexecution-benchmark/output-metrics-url.sh
Outdated
Show resolved
Hide resolved
…esolving error on compare benchmark results job
- inline set task env - remove unrelated `TMPNET_START_METRICS_COLLECTOR` env var
maru-ava
left a comment
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.
Thank you!
aaronbuchwald
left a comment
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.
Nice work!
Nice improvement to provide well-defined tasks for specific jobs in CI. We should imho continue to support running dynamically defined jobs via workflow dispatch as this has proven to be extremely helpful imo.
…e (custom) approach to task based operations
|
Triggered workflow with custom inputs. Run: https://github.com/ava-labs/avalanchego/actions/runs/19149324486/job/54735171547 |
…ile env making copy_dir.sh reusable for any S3 bucket
| METRICS_COLLECTOR_ENABLED={{.METRICS_COLLECTOR_ENABLED}} \ | ||
| EXECUTION_DATA_DIR={{.EXECUTION_DATA_DIR}} \ | ||
| bash -x ./scripts/benchmark_cchain_range.sh | ||
| # Runtime context variables are read from environment by the script: |
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.
(No action required) Not entirely sure why these details need to be embedded in the taskfile. Or, to be honest, why this task makes sense at all. It might be cleaner to only add tasks for pre-defined test configurations and leave ad-hoc usage to direct script invocation.
Could we update the re-execution README to make sure that instructions to run this via the GH CLI are up to date? Also, does this support both running a named task and granularly defined task via GH CLI? |
| using: composite | ||
| steps: | ||
| - name: Set task env | ||
| - uses: cachix/install-nix-action@02a151ada4993995686f9ed4f1be7cfbb229e56f #v31 |
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.
So we are duplicating the code from run monitored tmpnet command here to avoid the relative import? Is there no better way to share code across these two actions than to duplicate the shared code?
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 alternative is duplicating the custom actions into each repo's .github/actions/ directory. Inlining avoids that while giving us:
- Local reproducibility:
./scripts/run_task.sh <task-name>just works - Zero cross-repo dependencies
- Self-documenting workflows
The logic seems simple enough that duplication beats dependency management here, especially during the polyrepo migration where independence is valuable.
| with: | ||
| tool: 'go' | ||
| output-file-path: ${{ env.BENCHMARK_OUTPUT_FILE }} | ||
| output-file-path: benchmark-output.txt |
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.
What's the reason for this change?
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.
Good catch I used it for testing relative paths on self-hosted environments.
Reverted this change: 0cb04dc
| START_BLOCK=${{ inputs.start-block }} \ | ||
| END_BLOCK=${{ inputs.end-block }} \ | ||
| BENCHMARK_OUTPUT_FILE="${{ env.BENCHMARK_OUTPUT_FILE }}" \ | ||
| TASK_NAME="custom-reexecution" |
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.
Why do we need a task name here?
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.
Without TASK_NAME, parallel local execution of c-chain-reexecution-* tasks would conflict they'd all try to create file with the same name. TASK_NAME ensures each task gets unique context rich identifiers.
This isn't required for CI because each matrix job runs on a separate runner with isolated resources, but it's required for local development where you might run multiple tasks simultaneously on the same machine.
| BENCHMARK_OUTPUT_FILE="${{ env.BENCHMARK_OUTPUT_FILE }}" \ | ||
| TASK_NAME="custom-reexecution" | ||
| fi | ||
| env: |
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.
Why were some env vars removed in this change? It seems like metrics server enabled and labels were dropped here
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.
In current master labels were never set, not even in the parent action for example we can see labels are empty on this run.
Regarding metrics server enabled we consolidated it making it implicitly controlled by whether the collector is enabled.
The benchmark_cchain_range.sh script reads these from the environment using the ${VAR:+--flag} pattern, so they're still available just passed through environment variables instead of explicit task parameters. This makes the workflow cleaner since monitoring config is grouped in the env: section.
| options: | ||
| - ubuntu-latest | ||
| - blacksmith-4vcpu-ubuntu-2404 | ||
| - avalanche-avalanchego-runner-2ti | ||
| - avago-runner-m6i-4xlarge-ebs-fast | ||
| - avago-runner-i4i-4xlarge-local-ssd |
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.
Can we remove this restriction for now?
We are still adding new labels behind ARC on a somewhat regular basis as we test different instance / storage types and this would require merging a code change to test those new labels, whereas right now we can easily trigger manual workflows.
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.
Same comment for the other file that mirrors this one.
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.
Valid, done.
…ing default options
Why this should be merged
This patch is breakdown of #4435
This patch contains a custom action that doesn't rely on relative imports, workflows that externalize reexecute configuration to tasks.