Skip to content
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

Reconfigure CI pipelines for PR and merge queue #11526

Merged
merged 5 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 0 additions & 62 deletions .azure/lint-linux.yml

This file was deleted.

37 changes: 29 additions & 8 deletions .azure/docs-linux.yml → .azure/lint_docs_qpy-linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ parameters:
displayName: "Version of Python to use"

jobs:
- job: 'Docs'
- job: 'Lint_Docs_QPY'
displayName: 'Lint, documentation and QPY'
pool: {vmImage: 'ubuntu-latest'}

variables:
Expand All @@ -20,14 +21,13 @@ jobs:
displayName: 'Use Python ${{ parameters.pythonVersion }}'

- bash: tools/install_ubuntu_docs_dependencies.sh
displayName: 'Install dependencies'
displayName: 'Install docs dependencies'

- bash: |
set -e
tox -e docs
# Clean up Sphinx detritus.
rm -rf docs/_build/html/{.doctrees,.buildinfo}
displayName: 'Run Docs build'
- bash: tox run -e docs,lint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good idea to leverage tox for lint too. Save a lot of config logic here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

laziness is often a good motivator for single-source-of-truth configuration lol

displayName: 'Lint and docs'

- bash: rm -rf docs/_build/html/{.doctrees,.buildinfo}
displayName: 'Clean up docs detritus'

- task: ArchiveFiles@2
inputs:
Expand All @@ -43,3 +43,24 @@ jobs:
artifactName: 'html_docs'
Parallel: true
ParallelCount: 8

- task: Cache@2
inputs:
key: 'qpy | test/qpy_compat/test_qpy.py | "$(Build.BuildNumber)"'
restoreKeys: |
qpy | test/qpy_compat/test_qpy.py
path: qpy_files
displayName: Cache old QPY files

- bash: |
set -e
# Reuse the docs environment to avoid needing to rebuild another
# version of Qiskit.
source .tox/docs/bin/activate
mv qpy_files/* test/qpy_compat || :
pushd test/qpy_compat
./run_tests.sh
popd
mkdir qpy_files || :
mv test/qpy_compat/qpy_* qpy_files/.
displayName: 'Run QPY backwards compat tests'
21 changes: 0 additions & 21 deletions .azure/test-linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@ parameters:
- name: "testRust"
type: boolean

- name: "testQPY"
type: boolean

- name: "testImages"
type: boolean

Expand Down Expand Up @@ -166,24 +163,6 @@ jobs:
displayName: 'Publish images on test failure'
condition: failed()

- ${{ if eq(parameters.testQPY, true) }}:
- task: Cache@2
inputs:
key: 'qpy | test/qpy_compat/test_qpy.py | "$(Build.BuildNumber)"'
restoreKeys: |
qpy | test/qpy_compat/test_qpy.py
path: qpy_files
displayName: cache old qpy
- bash: |
set -e
mv qpy_files/* test/qpy_compat || :
pushd test/qpy_compat
./run_tests.sh
popd
mkdir qpy_files || :
mv test/qpy_compat/qpy_* qpy_files/.
displayName: 'Run QPY backwards compat tests'

- ${{ if eq(parameters.testImages, true) }}:
- bash: |
set -e
Expand Down
76 changes: 59 additions & 17 deletions azure-pipelines.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ pr:
include:
- '*'

# A schedule only runs on branches that match include rules from _both_ `main`
# and the branch itself. On `main`, we blanket include all branches that might
# want to be enabled, then particular branches can override it to exclude
# themselves by removing themselves from the trigger list. For example, old
# stable branches can remove `stable/*` from their copy of this file once they
# reach their end-of-life.
schedules:
- cron: "20 6 * * *"
displayName: "Complete matrix test"
Expand Down Expand Up @@ -82,9 +88,13 @@ parameters:
# - push to a pull request (`PullRequest`)
# For `IndividualCI` (push/merge to a branch/tag on Qiskit/qiskit-terra), you
# need to examine `variables['Build.SourceBranch']` to determine whether it's a
# branch or a tag.
# branch or a tag, and if a branch, then whether it's in the merge queue or a
# push to a "real" branch.
stages:
# Nightly cron job.
#
# For this to run on a branch, the `schedules` trigger up at the top of this
# file needs to match on _both_ `main` and the branch itself.
- ${{ if eq(variables['Build.Reason'], 'Schedule') }}:
- stage: "Nightly"
displayName: "Nightly complete matrix tests"
Expand All @@ -93,7 +103,6 @@ stages:
- template: ".azure/test-linux.yml"
parameters:
pythonVersion: ${{ version }}
testQPY: false
testImages: false
testRust: false
installOptionals: true
Expand Down Expand Up @@ -123,10 +132,10 @@ stages:
id: 7864
comment: Nightly test job failed at commit $(Build.SourceVersion). View the logs at $(System.TeamFoundationCollectionUri)$(System.TeamProject)/_build/results?buildId=$(Build.BuildId).

# Full PR suite. This also needs to apply to the merge queue, which appears
# as a push event. The queue won't get cron triggered because the schedule
# trigger doesn't have it as a target.
- ${{ if or(eq(variables['Build.Reason'], 'PullRequest'), contains(variables['Build.SourceBranch'], 'gh-readonly-queue')) }}:
# Full PR suite. PRs need to pass this pipeline in order to be moved to the
# merge queue, where they'll use the next rule as the branch-protection rule
# for the final merge to the base branch.
- ${{ if eq(variables['Build.Reason'], 'PullRequest') }}:
# The preliminary stage should be small in both total runtime (including
# provisioning) and resources required. About half of PR commits result in
# a CI failure, and over 90% of these are in linting, documention or a test
Expand All @@ -135,14 +144,10 @@ stages:
- stage: "Lint_Docs_Prelim_Tests"
displayName: "Preliminary tests"
jobs:
- template: ".azure/lint-linux.yml"
- template: ".azure/lint_docs_qpy-linux.yml"
parameters:
pythonVersion: ${{ parameters.minimumPythonVersion }}

- template: ".azure/docs-linux.yml"
parameters:
pythonVersion: ${{ parameters.documentationPythonVersion }}

- template: ".azure/test-linux.yml"
parameters:
pythonVersion: ${{ parameters.minimumPythonVersion }}
Expand All @@ -153,13 +158,12 @@ stages:
# to fail fast in the first CI stage.
installOptionals: true
testRust: true
testQPY: true
testImages: true

# The rest of the PR pipeline is to test the oldest and newest supported
# versions of Python. It's very rare for a failure to be specific to an intermediate version of
# Python, so we just catch those in the cron-job pipeline to reduce the
# amount of resources used.
# versions of Python. It's very rare for a failure to be specific to an
# intermediate version of Python, so we just catch those in the cron-job
# pipeline to reduce the amount of resources used.
- stage: "Tests"
displayName: "Main tests"
dependsOn: "Lint_Docs_Prelim_Tests"
Expand All @@ -168,7 +172,6 @@ stages:
parameters:
pythonVersion: ${{ parameters.maximumPythonVersion }}
testRust: false
testQPY: false
testImages: false
installFromSdist: true
installOptionals: false
Expand All @@ -193,6 +196,46 @@ stages:
pythonVersion: ${{ parameters.maximumPythonVersion }}
installOptionals: false

# Merge queue. A PR that reaches here has already passed the more rigorous PR
# suite, so is very likely to pass. The main reasons for failures here are
# flaky VMs timing out (which we can't do much about), or a merge conflict
# with another PR that is also in the merge queue.
#
# There's no reason to have multiple stages in this case, because we're
# expecting it to pass. Having more than one stage frustrates parallel
# throughput in low-contention cases, and guarantees a longer critical path.
- ${{ if and(eq(variables['Build.Reason'], 'IndividualCI'), startsWith(variables['Build.SourceBranch'], 'refs/heads/gh-merge-queue')) }}:
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
- stage: "Merge_Queue"
displayName: "Merge queue"
jobs:
- template: ".azure/lint_docs_qpy-linux.yml"
parameters:
pythonVersion: ${{ parameters.minimumPythonVersion }}

- template: ".azure/test-linux.yml"
parameters:
pythonVersion: ${{ parameters.minimumPythonVersion }}
installOptionals: true
testRust: true
testImages: true

- template: ".azure/test-linux.yml"
parameters:
pythonVersion: ${{ parameters.maximumPythonVersion }}
installOptionals: false
testRust: false
testImages: false

- template: ".azure/test-macos.yml"
parameters:
pythonVersion: ${{ parameters.maximumPythonVersion }}
installOptionals: false

- template: ".azure/test-windows.yml"
parameters:
pythonVersion: ${{ parameters.maximumPythonVersion }}
installOptionals: false

# Push to main or the stable branches. The triggering branches also need to
# be in the triggers at the top of this file.
- ${{ if and(eq(variables['Build.Reason'], 'IndividualCI'), or(startsWith(variables['Build.SourceBranch'], 'refs/heads/main'), startsWith(variables['Build.SourceBranch'], 'refs/heads/stable/'))) }}:
Expand All @@ -202,6 +245,5 @@ stages:
parameters:
pythonVersion: ${{ parameters.branchPushPythonVersion }}
testRust: true
testQPY: true
testImages: true
installOptionals: false
11 changes: 9 additions & 2 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[tox]
minversion = 3.3.0
minversion = 4.0
envlist = py38, py39, py310, py311, py312, lint-incr
isolated_build = true

Expand All @@ -23,9 +23,16 @@ commands =

[testenv:lint]
basepython = python3
# `pylint` will examine the source code, not the version that would otherwise be
# installed in `site-packages`, so we use an editable install to make sure the
# compiled modules are built into a valid place for it to find them.
package = editable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should do this globally in the tox config. I'm constantly hitting this locally when using tox which is why I pushed #11380

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge #11380 in that separate PR after this, maybe? To me, this change is required to fix the lint job (at the moment, tox -e lint only works if you happen to already have built the rust extensions in place yourself, hence the CI failure earlier), but #11380 isn't strictly needed, it's a change to the config to deliberately override some of tox's isolation as a trade-off to get better re-test performance.

allowlist_externals = cargo
commands =
ruff check qiskit test tools examples setup.py
black --check {posargs} qiskit test tools examples setup.py
cargo fmt --check
ruff check qiskit test tools examples setup.py
cargo clippy -- -D warnings
pylint -rn qiskit test tools
# This line is commented out until #6649 merges. We can't run this currently
# via tox because tox doesn't support globbing
Expand Down