Duplicated CI job runs on pull requests

[backmari]

Description

All pull request CI jobs are run twice, since the triggers are both push and pull_request for any branch. This change reduces the number of jobs run by half. The jobs will still be re-triggered if new commits are pushed.

This change would, however, mean that one has to create a pull request to trigger the CI pipeline, or use the workflow dispatch functionality:

https://github.com/SasView/sasview/actions/workflows/ci.yml

Fixes # (issue/issues)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Review Checklist:

[if using the editor, use [x] in place of [ ] to check a box]

Documentation (check at least one)

• There is nothing that needs documenting
• Documentation changes are in this PR
• There is an issue open for the documentation (link?)

Installers

• There is a chance this will affect the installers, if so
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • Wheels installer (GH artifact) has been tested (installed and worked)

Licensing (untick if necessary)

• The introduced changes comply with SasView license (BSD 3-Clause)

[llimeht]

Being able to run CI in forks and on branches that don't have a PR is needed; adding developer documentation about how to do so from the GUI and with the gh command-line tool is needed, as most projects do not disable this and new developers will reasonably expect that CI runs on their work and they can test things out prior to making a PR.

(From memory it's something like gh workflow run --repo <org>/<repo> --ref <branch> ci.yml, so gh workflow run --repo llimeht/sasview --ref tmp/some-work-in-progress-branch ci.yml )

[backmari]

Is this an acceptable compromise? That is, we provide developer documentation on how to trigger CI on a branch, but the CI no longer runs automatically on branches, only on PR:s?
We need to trigger on PR:s since otherwise the CI won't run on PR:s from forks.
@krzywon @DrPaulSharp Any opinions? 🙂

[DrPaulSharp]

Ok, looking at this I think we should modify our concurrency settings to hopefully satisfy everyone.

At present, the concurrency we have is:

# Stop existing workflows for matching branches and pull requests
concurrency:
  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
  cancel-in-progress: true

As noted in the comment, this code acts to stop the CI if an update is pushed to a branch OR a pull request, with the run of the CI that is currently in progress cancelled in favour of the more recent one.

I propose we extend this to also account for stopping the CI activated on push if a pull request is filed soon after the push. This then means that the CI will be run if there is a push but no PR, but only one version is run if a PR is opened immediately. This also means we don't have to give developers instructions to run CI manually as we won't make any changes to existing runs. The concurrency group I would define is:

# Stop existing workflows for matching branches and pull requests, including pull requests from recently-pushed branches
concurrency:
  group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }}
  cancel-in-progress: true

This then matches the format of the concurrency group for push and PR triggers, and should achieve what we want. See: https://docs.github.com/en/actions/reference/workflows-and-actions/variables

Let me know what you think @backmari @krzywon @llimeht

[DrPaulSharp]

See #2638 for when this was done orginally.

[DrPaulSharp]

Hmm, the concurrency is working as intended - in that the push job was stopped and the pull request job passed, but we have the stopped push job listed in the checks. Does anyone know how we can solve this?

[klytje]

@DrPaulSharp The issue is that concurrency groups seem to cancel the jobs, counting them as failed checks. We could probably achieve the same using if-guards, which will instead make them count as skipped, not failed, meaning the workflow run can still count as passed. I don't think there's any way to completely remove skipped/cancelled jobs from the list. Example from my repo:

[backmari]

@klytje Thank you, I agree that skipped looks better than cancelled, since cancelled is interpreted as failing. Can you please post a snippet or link to the workflow file for the repo? I am not sure I understand what you mean by if-guards.

[klytje]

@backmari Link. All conditional steps depend on check_headers, which only runs conditionally depending on the result of check_draft.

[backmari]

@klytje Ah I see. I think the problem is that a workflow job doesn't have access to information about other running jobs (at least through standard variables etc.). The concurrency feature is as far as I know the only way to coordinate between jobs.

[klytje]

@backmari Do we actually need the pull_request trigger when we already have push? All updates to a PR should already trigger the CI through the push, so can we not simply remove it or make it trigger only when a PR is created? They have this example in the docs:

on:
  pull_request:
    types: [opened, reopened]

Default behaviour also includes synchronize (changes to head of PR branch), which I think should always be covered by the push trigger.

[backmari]

@klytje My initial suggestion was to only trigger on pull_request. But as Stuart pointed out we should in this case provide developer docs for how to trigger workflows manually on branches for developers that want to run the workflow before creating a PR. I am leaning towards this solution.

We need to always trigger on pull_request since some developers work in a fork of the repo before creating a pull request into this repo.

[klytje]

@backmari But if we have:

on:
  push:
  pull_request:
    types: [opened, reopened]

then it will still trigger whenever a PR is opened from a fork, no?

[backmari]

This seems to eliminate some of the duplication. I wonder though if the CI will rerun if a new commit is pushed to a PR opened from a fork or if it will only run when first opened 🤔

[klytje]

@backmari Good question. Probably it will not since the branch does not belong to the repository? To support that case, we could perhaps fully re-enable pull_request but skip the run if the trigger was pull_request and the base repository is sasview (i.e., we are working on an internal branch)? Link. In our case, build-and-test-internal would then lead to everything being skipped.

That seems a little complicated though, so maybe someone else has a better idea...

[backmari]

@klytje Thank you for the suggestion! I think this is the best solution so far. Since all other jobs depend on the ruff job, it is enough for the ruff job to depend on a filtering job that skips jobs triggered by pull requests in the base repo, since for branches in the base repo, the CI has already been triggered by the push event.
The CI jobs run as expected also for a PR from a fork: #3874.
This solution also preserves existing behavior for developers (CI runs on push to any branch in the base repo), but eliminates the duplicated CI jobs.
@llimeht @DrPaulSharp What do you think about this current solution?

[wpotrzebowski]

Being able to run CI in forks and on branches that don't have a PR is needed; adding developer documentation about how to do so from the GUI and with the gh command-line tool is needed, as most projects do not disable this and new developers will reasonably expect that CI runs on their work and they can test things out prior to making a PR.

(From memory it's something like gh workflow run --repo <org>/<repo> --ref <branch> ci.yml, so gh workflow run --repo llimeht/sasview --ref tmp/some-work-in-progress-branch ci.yml )

Sorry, I missed that before. I started documenting it at some point https://github.com/SasView/sasview/wiki/Building_custom_branches_from_CI

[krzywon]

We discussed this at the technical meeting today and the consensus is we need the PR tests and builds to always run when a change is made to a PR. On push, only having a minimal set of checks should be fine, like only running the tests on a handful of python versions, so long as a developer can tell if their changes aren't breaking anything.

This would help minimize the overlap, provide feedback to developers as they are making changes in a branch, and ensure all tests and builds are generated from the as-merged code.

[backmari]

@krzywon Just double checking that I understand, on the push event, the CI should run the tests for some python versions (currently 3.12, 3.13, 3.14) but can skip testing the installer? Can we also skip running the tests on multiple platforms and only run on e.g. ubuntu-latest?

[krzywon]

@backmari, I think you have the basic plan. On the push event, skip building the installers and only run the unit tests on one python version (3.14, probably). I would argue this should be run across all platforms because some cross-platform issues could pop up if you are only running against one OS.

See https://github.com/SasView/sasview/actions/runs/23361600512 for an example where unit tests are successful on Windows, but fail for Ubuntu and MacOS.

[krzywon]

The latest changes look good to me and the CI is running as expected in this PR, where only the py3.14 tests are run on push and everything is run on pull request.

@llimeht, do you have any other suggestions or comments?

[krzywon]

Side note - the failing CI is from the unit test noted in #3904.

[backmari]

@krzywon Ah, thanks for the unit test fix! I have rebased.