-
Notifications
You must be signed in to change notification settings - Fork 5
feat(ci): automate and enhance performance validation, add semantic PR checks, and consolidate build steps #562
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
Changes from 32 commits
394c9a6
2a59ceb
fdeee68
1993586
f718940
a800e57
42a53cb
d66ffe3
752aee8
60c827e
c3576b7
822281c
7527716
a871c51
eac1439
6253322
3539ab4
a3e37d6
fa85571
5cb4e86
8daaad0
8459cf1
523d94c
4bd18e5
ce8a588
90bbf90
24d4820
9861437
5252413
2a27ab2
cc3153d
23500e0
490895c
b908b5c
6b57b5e
6a50f71
d191243
3ca4209
46cc428
5cd10c4
3023beb
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 | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,18 @@ | ||||||||||||||||||||||||||||
| name: Main build | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||||||
| workflow_dispatch: # Allow running the workflow manually from the GitHub UI | ||||||||||||||||||||||||||||
| workflow_dispatch: | ||||||||||||||||||||||||||||
| inputs: | ||||||||||||||||||||||||||||
| run_performance: | ||||||||||||||||||||||||||||
| description: 'Run performance job? (true/false)' | ||||||||||||||||||||||||||||
| required: false | ||||||||||||||||||||||||||||
| default: false | ||||||||||||||||||||||||||||
| type: boolean | ||||||||||||||||||||||||||||
| force_baseline: | ||||||||||||||||||||||||||||
| description: 'Force running baseline benchmarks even if cached (true/false)' | ||||||||||||||||||||||||||||
| required: false | ||||||||||||||||||||||||||||
| default: false | ||||||||||||||||||||||||||||
| type: boolean | ||||||||||||||||||||||||||||
| pull_request: | ||||||||||||||||||||||||||||
| branches: | ||||||||||||||||||||||||||||
| - main | ||||||||||||||||||||||||||||
|
|
@@ -110,19 +121,39 @@ jobs: | |||||||||||||||||||||||||||
| if-no-files-found: error | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| performance: | ||||||||||||||||||||||||||||
| if: github.event_name == 'schedule' && github.ref == 'refs/heads/main' | ||||||||||||||||||||||||||||
| # Run if manually dispatched (with input), on schedule, or if explicitly requested | ||||||||||||||||||||||||||||
| if: | | ||||||||||||||||||||||||||||
| (github.event_name == 'schedule' && github.ref == 'refs/heads/main') || | ||||||||||||||||||||||||||||
| (github.event_name == 'workflow_dispatch' && github.event.inputs.run_performance == 'true') | ||||||||||||||||||||||||||||
|
Comment on lines
+124
to
+127
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.
The condition omits the - (github.event_name == 'workflow_dispatch' && github.event.inputs.run_performance == 'true')
+ (github.event_name == 'workflow_dispatch' && github.event.inputs.run_performance == 'true') ||
+ (github.event_name == 'pull_request')🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| strategy: | ||||||||||||||||||||||||||||
| fail-fast: false | ||||||||||||||||||||||||||||
| matrix: | ||||||||||||||||||||||||||||
| os: [windows-11-arm, ubuntu-24.04-arm, ubuntu-latest, windows-latest, macos-latest] | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| runs-on: ${{ matrix.os }} | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||||
| FORCE_BASELINE: ${{ github.event.inputs.force_baseline }} | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
rjmurillo marked this conversation as resolved.
|
||||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||||
| - uses: actions/checkout@v4 | ||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||
| fetch-depth: 0 | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| - name: Get baseline SHA | ||||||||||||||||||||||||||||
| id: get-baseline-sha | ||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||
| $baseline = Get-Content build/perf/baseline.json | ConvertFrom-Json | ||||||||||||||||||||||||||||
| echo "sha=$($baseline.sha)" >> $env:GITHUB_OUTPUT | ||||||||||||||||||||||||||||
| shell: pwsh | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
rjmurillo marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||
| - name: Download baseline performance cache | ||||||||||||||||||||||||||||
| id: restore-baseline-cache | ||||||||||||||||||||||||||||
| uses: actions/cache/restore@v4 | ||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||
| path: ./artifacts/performance/perfResults/baseline | ||||||||||||||||||||||||||||
| key: baseline-performance-${{ matrix.os }}-${{ steps.get-baseline-sha.outputs.sha }} | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
Comment on lines
+153
to
+159
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. 🛠️ Refactor suggestion Cache key risks hash-collision when baseline SHA changes across branches The cache is keyed on 🤖 Prompt for AI Agents
Owner
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. @coderabbitai create an issue to track this suggestion
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. 🧩 Analysis chainI'll create an issue to track this cache key collision suggestion. @rjmurillo I've created an issue to track the cache key collision suggestion. The issue will help ensure this improvement is properly prioritized and implemented in a future PR. 🏁 Script executed: Length of output: 1305 @rjmurillo Perfect! I've successfully created issue #564 to track the cache key collision risk. The issue includes:
The issue is now available at: #564 🧠 Learnings used |
||||||||||||||||||||||||||||
| - name: Setup, Restore, and Build Solution | ||||||||||||||||||||||||||||
| uses: ./.github/actions/setup-restore-build | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -134,9 +165,45 @@ jobs: | |||||||||||||||||||||||||||
| # These values balance CI cost and reliability, as recommended in issue #553. | ||||||||||||||||||||||||||||
| run: ${{ github.workspace }}/build/scripts/perf/PerfCore.ps1 -v diag -diff -ci | ||||||||||||||||||||||||||||
|
rjmurillo marked this conversation as resolved.
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| - name: Publish baseline performance summaries to GitHub | ||||||||||||||||||||||||||||
| if: success() || failure() | ||||||||||||||||||||||||||||
| shell: pwsh | ||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||
| $resultsDir = "artifacts/performance/perfResults/baseline/results" | ||||||||||||||||||||||||||||
| if (Test-Path $resultsDir) { | ||||||||||||||||||||||||||||
| Add-Content -Path $env:GITHUB_STEP_SUMMARY -Value "### Baseline Performance Results" | ||||||||||||||||||||||||||||
| $files = Get-ChildItem -Path $resultsDir -Filter "*-report-github.md" | Sort-Object Name | ||||||||||||||||||||||||||||
| foreach ($file in $files) { | ||||||||||||||||||||||||||||
| Get-Content $file.FullName | Out-File -Append -FilePath $env:GITHUB_STEP_SUMMARY | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| Add-Content -Path $env:GITHUB_STEP_SUMMARY -Value "---" | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| - name: Publish performance summaries to GitHub | ||||||||||||||||||||||||||||
| if: success() || failure() | ||||||||||||||||||||||||||||
| shell: pwsh | ||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||
| $resultsDir = "artifacts/performance/perfResults/perfTest/results" | ||||||||||||||||||||||||||||
| if (Test-Path $resultsDir) { | ||||||||||||||||||||||||||||
| Add-Content -Path $env:GITHUB_STEP_SUMMARY -Value "### Current Performance Results" | ||||||||||||||||||||||||||||
| $files = Get-ChildItem -Path $resultsDir -Filter "*-report-github.md" | Sort-Object Name | ||||||||||||||||||||||||||||
| foreach ($file in $files) { | ||||||||||||||||||||||||||||
| Get-Content $file.FullName | Out-File -Append -FilePath $env:GITHUB_STEP_SUMMARY | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| Add-Content -Path $env:GITHUB_STEP_SUMMARY -Value "---" | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
rjmurillo marked this conversation as resolved.
Comment on lines
+171
to
+197
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. 🧹 Nitpick (assertive) Duplicate summary-publishing blocks – DRY this via a reusable step The two blocks differ only in directory path and header. Extract into a small composite action or PowerShell function that accepts 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| - name: Upload performance files | ||||||||||||||||||||||||||||
| uses: actions/upload-artifact@v4 | ||||||||||||||||||||||||||||
| if: success() || failure() | ||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||
| name: Performance files (${{ matrix.os }}) | ||||||||||||||||||||||||||||
| path: "./artifacts/performance/**" | ||||||||||||||||||||||||||||
| name: performance-(${{ matrix.os }}) | ||||||||||||||||||||||||||||
| path: | | ||||||||||||||||||||||||||||
| ./artifacts/performance/** | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||
| - name: Upload baseline performance cache | ||||||||||||||||||||||||||||
| if: steps.restore-baseline-cache.outputs.cache-hit != 'true' || env.FORCE_BASELINE == 'true' | ||||||||||||||||||||||||||||
| uses: actions/cache/save@v4 | ||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||
| path: ./artifacts/performance/perfResults/baseline | ||||||||||||||||||||||||||||
| key: baseline-performance-${{ matrix.os }}-${{ steps.get-baseline-sha.outputs.sha }} | ||||||||||||||||||||||||||||
|
Comment on lines
+204
to
+212
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.
When forcing a baseline refresh the current job overrides the previously restored cache key, even if the perf run fails. - if: steps.restore-baseline-cache.outputs.cache-hit != 'true' || env.FORCE_BASELINE == 'true'
+ if: (steps.restore-baseline-cache.outputs.cache-hit != 'true') && (env.FORCE_BASELINE != 'true')…and instead rely on a fresh key (e.g., append a timestamp) when 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.