Optimize workflows to avoid duplicate runs on PR commits - #1250
Conversation
Co-authored-by: bact <128572+bact@users.noreply.github.com>
Co-authored-by: bact <128572+bact@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent GitHub Actions workflows from running twice per PR commit by adding job-level if: conditions intended to skip redundant push executions when a corresponding pull_request run exists.
Changes:
- Added the same job-level
if:condition tounittest.yml,lint.yml, andcodeql-analysis.yml. - Added the condition to two jobs in
pypi-publish.yml, including tightening the existing “not for forks” condition.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| .github/workflows/unittest.yml | Adds a job-level if: intended to skip duplicate runs for PR commits. |
| .github/workflows/lint.yml | Adds the same job-level if: condition to the lint job. |
| .github/workflows/codeql-analysis.yml | Adds the same job-level if: condition to the CodeQL analysis job. |
| .github/workflows/pypi-publish.yml | Adds the condition to two jobs and combines it with the repository (non-fork) guard. |
| # Skip if this is a push event triggered by a pull request | ||
| # (to avoid running the workflow twice for PR commits) | ||
| # Run on pull_request events OR push events that are NOT from a PR | ||
| if: github.event_name == 'pull_request' || github.event.pull_request == null |
There was a problem hiding this comment.
This job condition does not prevent duplicate PR runs because on push events github.event.pull_request is not populated, so the job still runs on every push. If the intent is to avoid redundant runs, gate push executions based on whether the branch has an open PR (GitHub API query) or remove/limit the push trigger for this workflow.
There was a problem hiding this comment.
You're correct. Fixed in commit 2bf30dd by replacing the ineffective condition with concurrency groups that properly prevent duplicate runs.
| # Only run if: | ||
| # 1. Not a fork (repository must be 'pythainlp/pythainlp') | ||
| # 2. AND not a duplicate (skip push events triggered by pull requests) | ||
| if: github.repository == 'pythainlp/pythainlp' && (github.event_name == 'pull_request' || github.event.pull_request == null) |
There was a problem hiding this comment.
This condition still evaluates true for all push events because github.event.pull_request is not present on push payloads, so it won’t actually eliminate the duplicate push+pull_request executions you’re targeting. To reliably skip push runs that correspond to PR updates, consider adding a small precheck job that queries for open PRs on the current branch and only proceeds when none exist (while still allowing direct pushes and non-PR branch builds).
There was a problem hiding this comment.
You're correct. Fixed in commit 2bf30dd using concurrency groups instead of the ineffective conditional check.
| # Skip if this is a push event triggered by a pull request | ||
| # (to avoid running the workflow twice for PR commits) | ||
| # Run on pull_request events OR push events that are NOT from a PR | ||
| if: github.event_name == 'pull_request' || github.event.pull_request == null |
There was a problem hiding this comment.
This job-level condition does not actually prevent duplicate runs for PR commits. For a push event, github.event.pull_request is not populated (it evaluates to null/empty), so the right-hand side is true and the job still runs on every push—including pushes to branches that have an open PR—so the workflow will still run twice (push + pull_request). Consider implementing an explicit “is this branch associated with an open PR?” check (via GitHub API) and gate the expensive job(s) on that result, or adjust triggers so PR branches don’t also trigger push runs.
There was a problem hiding this comment.
You're correct. Fixed in commit 2bf30dd by using workflow-level concurrency groups instead of job-level conditionals. The concurrency with cancel-in-progress: true properly cancels duplicate push events when pull_request events trigger.
| # Skip if this is a push event triggered by a pull request | ||
| # (to avoid running the workflow twice for PR commits) | ||
| # Run on pull_request events OR push events that are NOT from a PR | ||
| if: github.event_name == 'pull_request' || github.event.pull_request == null |
There was a problem hiding this comment.
This condition is ineffective at skipping PR-related push runs: on push events the payload doesn’t include pull_request, so github.event.pull_request == null evaluates true and the job still runs for every push. If the goal is to avoid duplicate runs, gate push executions based on whether the current ref has an open PR (e.g., query the GitHub API for open PRs with head = owner:branch) or remove/limit the push trigger accordingly.
| # Skip if this is a push event triggered by a pull request | |
| # (to avoid running the workflow twice for PR commits) | |
| # Run on pull_request events OR push events that are NOT from a PR | |
| if: github.event_name == 'pull_request' || github.event.pull_request == null |
There was a problem hiding this comment.
You're correct. Fixed in commit 2bf30dd by replacing the ineffective if condition with workflow-level concurrency groups that properly cancel duplicate push runs.
| analyze: | ||
| name: Analyze | ||
| # Skip if this is a push event triggered by a pull request | ||
| # (to avoid running the workflow twice for PR commits) | ||
| # Run on pull_request events OR push events that are NOT from a PR | ||
| if: github.event_name == 'pull_request' || github.event.pull_request == null |
There was a problem hiding this comment.
This if expression won’t filter out duplicate PR-commit runs: github.event.pull_request is only present on pull_request events, so for push it evaluates to null and the job still runs. If you need “run on pushes unless that push is associated with an open PR”, add a lightweight precheck (GitHub API lookup for open PRs for the current branch) and only run the expensive analysis when no PR exists, or change triggers so PR branches don’t also invoke push.
| analyze: | |
| name: Analyze | |
| # Skip if this is a push event triggered by a pull request | |
| # (to avoid running the workflow twice for PR commits) | |
| # Run on pull_request events OR push events that are NOT from a PR | |
| if: github.event_name == 'pull_request' || github.event.pull_request == null | |
| pr-check: | |
| name: Check for open PRs for branch | |
| runs-on: ubuntu-latest | |
| # Only needed for push events; pull_request events always run analysis. | |
| if: github.event_name == 'push' | |
| outputs: | |
| has_open_pr: ${{ steps.check-pr.outputs.has_open_pr }} | |
| steps: | |
| - name: Check for open pull requests for this branch | |
| id: check-pr | |
| uses: actions/github-script@v7 | |
| with: | |
| github-token: ${{ secrets.GITHUB_TOKEN }} | |
| script: | | |
| const branchRef = context.ref.replace('refs/heads/', ''); | |
| const headRef = `${context.repo.owner}:${branchRef}`; | |
| const { data: pulls } = await github.rest.pulls.list({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| state: 'open', | |
| head: headRef, | |
| }); | |
| const hasOpenPr = pulls.length > 0; | |
| core.info(`Open pull requests for ${headRef}: ${pulls.length}`); | |
| core.setOutput('has_open_pr', hasOpenPr ? 'true' : 'false'); | |
| analyze: | |
| name: Analyze | |
| needs: pr-check | |
| # Skip if this is a push event for a branch that already has an open pull request | |
| # (to avoid running the workflow twice for PR commits). | |
| # Run on pull_request events OR push events that are NOT from a PR. | |
| if: github.event_name == 'pull_request' || needs.pr-check.outputs.has_open_pr != 'true' |
There was a problem hiding this comment.
You're correct. Fixed in commit 2bf30dd using workflow-level concurrency groups with cancel-in-progress: true instead of the API check approach. This is simpler to maintain while achieving the same goal of preventing duplicate runs.
to push something and test the unittest condition
Co-authored-by: bact <128572+bact@users.noreply.github.com>
Test push something to test new CI
| # Avoid duplicate runs for pull_request events. | ||
| # For pull requests to dev, the workflow runs on both push and pull_request events, | ||
| # but we only want it to run once. The concurrency group is the same for both, | ||
| # so the push event will be cancelled when the pull_request event starts. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} |
There was a problem hiding this comment.
This concurrency group won’t deduplicate push vs pull_request runs: github.event.pull_request.number is unset on push, so push runs use ${{ github.ref }} while PR runs use the PR number, meaning they won't cancel each other. Additionally, this workflow also runs on release and workflow_dispatch, so the comment implying this only affects PR duplication is misleading; with cancel-in-progress: true, release/dispatch runs may also be canceled if they share the group key.
| # Avoid duplicate runs for pull_request events. | |
| # For pull requests to dev, the workflow runs on both push and pull_request events, | |
| # but we only want it to run once. The concurrency group is the same for both, | |
| # so the push event will be cancelled when the pull_request event starts. | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | |
| # Avoid duplicate runs for the same workflow and Git ref. | |
| # This applies to all triggers (push, pull_request, release, workflow_dispatch). | |
| # With cancel-in-progress: true, a new run for the same ref cancels any in-progress run. | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.ref }} |
There was a problem hiding this comment.
Fixed in commit 5770622. Simplified to use github.ref for all workflows and updated comments to accurately describe what the concurrency group achieves.
| Minimum required version is now 3.9. | ||
| Support for Python 3.7 and 3.8 has been removed. | ||
| Added official support and CI testing for Python 3.14. | ||
|
|
||
| Some features and fixes in this version are AI-assisted. | ||
| See PR for prompt and details. | ||
|
|
||
| - Fix `royin` romanization #1172 | ||
| - Fix final consonant classification in `check_marttra()` #1173 | ||
| - Lazy load dictionaries to reduce memory usage #1186 | ||
| - Fix KeyError when transliterating text with Thai alphabet Kho Khon (U+0E05) #1187 | ||
| - Consolidate configuration into pyproject.toml #1188 | ||
| - Update type hints; Use Python 3.9 features #1189 #1190 | ||
| - Replace requests library with urllib.request from standard library to reduce | ||
| core dependencies #1211 | ||
| - Fix Kho Khon alphabet issue in `tltk` transliteration #1187 | ||
| - Migrate configurations to pyproject.toml #1188 #1226 #1239 | ||
| - Update type hints; Use Python 3.9 features #1189 #1190 & etc. | ||
| - Remove duplicated entries in volubilis dictionary #1200 | ||
| - Remove star imports #1207 | ||
| - Remove `requests` dependency #1211 | ||
| - Make package zip-safe #1212 | ||
| - Ensure thread-safety for tokenizers #1213 | ||
| - Improved documentation; code cleanup; more tests |
There was a problem hiding this comment.
This PR is described as optimizing GitHub Actions to avoid duplicate workflow runs, but this changelog hunk adds multiple unrelated release notes (Python version support changes and several feature/fix bullets). If these items are not part of this PR, they should be removed or moved to the PRs that introduce them to keep the changelog history accurate and reviewable.
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: bact <128572+bact@users.noreply.github.com>
Co-authored-by: bact <128572+bact@users.noreply.github.com>
Fixed in commit a57ebec. The issue was that Now using
Both events now share the same concurrency group and will properly cancel duplicates. |
| # Avoid duplicate runs for the same source branch. | ||
| # For push events, uses the branch name from github.ref_name. | ||
| # For pull_request events, uses the source branch name from github.head_ref. | ||
| # This ensures push and pull_request events for the same branch share the same group. | ||
| # Also applies to release (uses tag name) and workflow_dispatch (uses branch/tag name). | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }} |
There was a problem hiding this comment.
The concurrency group is based only on github.head_ref || github.ref_name. For fork-based PRs, github.head_ref is just the branch name, so different forks using the same branch name can end up canceling each other’s workflow runs in this repo. Add a repo qualifier to the group (e.g., PR head repo full_name, fallback to github.repository) to avoid cross-fork collisions.
| # Avoid duplicate runs for the same source branch. | |
| # For push events, uses the branch name from github.ref_name. | |
| # For pull_request events, uses the source branch name from github.head_ref. | |
| # This ensures push and pull_request events for the same branch share the same group. | |
| # Also applies to release (uses tag name) and workflow_dispatch (uses branch/tag name). | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }} | |
| # Avoid duplicate runs for the same source branch and repository. | |
| # For pull_request events, uses the source repo name from | |
| # github.event.pull_request.head.repo.full_name; otherwise uses github.repository. | |
| # For push events, uses the branch name from github.ref_name. | |
| # For pull_request events, uses the source branch name from github.head_ref. | |
| # This ensures events for the same repo and branch share the same group, | |
| # and avoids cross-fork collisions when branch names are reused. | |
| # Also applies to release (uses tag name) and workflow_dispatch (uses branch/tag name). | |
| concurrency: | |
| group: >- | |
| ${{ github.workflow }}-${{ | |
| github.event.pull_request.head.repo.full_name || github.repository | |
| }}-${{ github.head_ref || github.ref_name }} |
There was a problem hiding this comment.
Fixed in commit 7e2697f. Added repository qualifier to concurrency groups using github.event.pull_request.head.repo.full_name || github.repository to prevent cross-fork collisions when different forks use the same branch name.
|
@copilot apply changes based on the comments in this thread |
…llisions Co-authored-by: bact <128572+bact@users.noreply.github.com>
bact
left a comment
There was a problem hiding this comment.
LGTM. Looks like the ID is unique now.
| concurrency: | ||
| group: >- | ||
| ${{ github.workflow }}-${{ | ||
| github.event.pull_request.head.repo.full_name || github.repository | ||
| }}-${{ github.head_ref || github.ref_name }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
cancel-in-progress: true uses the same concurrency group for push, pull_request, and schedule runs. This means a routine push to dev can cancel the scheduled weekly CodeQL scan (and vice versa), potentially preventing scheduled scans from ever completing on an active repo. Consider using a separate concurrency group for schedule events (or disabling cancellation for schedule) so scheduled analyses are not interrupted by regular CI runs.
There was a problem hiding this comment.
Fixed in commit 3cd14c5. Changed cancel-in-progress to ${{ github.event_name != 'schedule' }} to prevent scheduled CodeQL scans from being cancelled by regular push/PR runs, ensuring weekly scans can complete on active repositories.
| for distributed computing environments where multiple threads may process | ||
| data concurrently. For detailed information about thread safety, see: | ||
| https://github.com/PyThaiNLP/pythainlp/blob/dev/docs/threadsafe.rst | ||
| https://pythainlp.org/dev-docs/notes/threadsafe.html |
There was a problem hiding this comment.
This documentation link update is unrelated to the PR’s stated goal (GitHub Actions concurrency / CI deduplication). To keep the change focused and ease review/reverts, consider moving this doc-only change into a separate PR (or update the PR description to explain why it’s included).
|
@copilot review and consider suggestions in this thread #1250 (review) |
… test file Co-authored-by: bact <128572+bact@users.noreply.github.com>
|



What do these changes do
Adds workflow-level concurrency groups to GitHub Actions workflows to prevent duplicate push and pull_request runs for the same source branch and repository, reducing CI time by ~50%. Also protects scheduled CodeQL scans from being cancelled by regular CI activity.
What was wrong
Workflows configured with both
pushandpull_requesttriggers execute twice for each PR commit: once when code is pushed, once when the PR updates. This occurs because GitHub fires both events for the same commit, wasting CI resources and GitHub Actions minutes.Additionally, scheduled CodeQL security scans could be cancelled by regular push/PR activity, potentially preventing weekly scans from ever completing on active repositories.
Affected workflows: unittest.yml (most expensive), lint.yml, codeql-analysis.yml, pypi-publish.yml.
How this fixes it
Uses workflow-level concurrency groups to deduplicate push and pull_request runs by ensuring both event types share the same concurrency group for the same source branch and repository:
For CodeQL workflow, uses conditional cancellation to protect scheduled scans:
How it works:
github.repository+github.ref_name(e.g.,PyThaiNLP/pythainlp-feature)github.event.pull_request.head.repo.full_name+github.head_ref(e.g.,fork/pythainlp-featureorPyThaiNLP/pythainlp-feature)For direct branch pushes (no PR):
This approach properly deduplicates runs while maintaining support for:
Implementation notes: Multiple iterations were required to achieve proper deduplication with cross-fork safety and scheduled scan protection. Initial attempts using job-level
ifconditions failed becausegithub.event.pull_requestis never populated on push events. A second iteration usinggithub.event.pull_request.number || github.reffailed because it resulted in different concurrency group names for push (refs/heads/branch) vs pull_request (PR number) events. A third iteration usinggithub.head_ref || github.ref_nameproperly normalized both event types to use the source branch name but lacked cross-fork isolation. The fourth iteration added repository qualification usinggithub.event.pull_request.head.repo.full_name || github.repositoryto prevent different forks with the same branch name from canceling each other's workflows. The final iteration added conditional cancellation (${{ github.event_name != 'schedule' }}) for the CodeQL workflow to ensure scheduled security scans are not interrupted by regular CI activity.Your checklist for this pull request
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.