Skip to content

Refactor environment variables used by test cases#114

Merged
rathishcholarajan merged 26 commits into
Qiskit:mainfrom
daka1510:refactor-test-workflow
Jan 28, 2022
Merged

Refactor environment variables used by test cases#114
rathishcholarajan merged 26 commits into
Qiskit:mainfrom
daka1510:refactor-test-workflow

Conversation

@daka1510
Copy link
Copy Markdown
Contributor

@daka1510 daka1510 commented Jan 20, 2022

Summary

Primary goal
Enable integration test cases to run separately against IBM Quantum ("legacy") and IBM Cloud ("cloud") endpoints. Makes use of Github environment secrets.

Secondary goal
Reduce repetition across workflow definition files, reduce number of parallel integration test runs to help getting around test stability issues.

Details and comments

Fixes #30
Fixes Qiskit/qiskit-ibm-provider#204
Helpful for #109
Helpful for #93

Example Workflow

https://github.com/daka1510/qiskit-ibm-runtime/actions/runs/1728894685
image

Prerequisites

  • Environment secrets set on main repository as described in the contribution guidelines, e.g.
    image
    image

@daka1510 daka1510 marked this pull request as draft January 20, 2022 15:44
Comment thread .github/workflows/ci.yml
# copyright notice, and modified files need to carry a notice indicating
# that they have been altered from the originals.

name: CI
Copy link
Copy Markdown
Contributor Author

@daka1510 daka1510 Jan 21, 2022

Choose a reason for hiding this comment

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

This workflow definitions replaces pr.yml and push.yml that were mostly the same and included duplicated job definitions.

Notable Changes

  • no concurrent workflow runs per branch (previous workflows are cancelled automatically)
  • previous behavior implemented via if conditions
  • rephrased push tests -> integration tests and pr tests -> unit tests
  • added build dependencies to make sure resource expensive integration tests are only run when everything else passes
    image

Let's discuss objections related to the updated CI approach in this thread.

@@ -0,0 +1,55 @@
# This code is part of Qiskit.
Copy link
Copy Markdown
Contributor Author

@daka1510 daka1510 Jan 21, 2022

Choose a reason for hiding this comment

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

This workflow definitions replaces cron-prod.yml and cron-staging.yml that were mostly the same and included duplicated job definitions for different environments.

Notable changes

  • avoid duplicated job definitions by using Github environments (see e.g. legacy-production)
  • reduced the matrix options so that the integration tests are only run using the latest supported Python and ubuntu version
    • version/os specific issues are expected to be uncovered by unit tests
    • helps with stability and decreases test runtime because we're not "performance testing" the APIs with parallel test jobs
  • left out the migration of cron-slow.yml and cron-slow-terra-main.yml for now
    • I believe the primary goal should be to stabilize the main path (integration tests) and then reenable slow tests and discuss if we need to run the same test with the latest versions of qiskit-terra (given we're no longer part of the meta package)

Let's discuss objections related to scheduled test cases in this thread.

Comment thread test/utils/decorators.py
* QISKIT_IBM_STAGING_DEVICE: staging device to use.
* QISKIT_IBM_STAGING_PRIVATE_HGP: staging hub/group/project to use for private jobs.
"""
"""Decorators used by unit tests."""
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In order to introduce support for running the integration tests against different APIs (legacy, Cloud) separately I consolidated the decorators we previously had.

Notable changes

  • best described in code, see below

Let's discuss objections related to decorator changes in this thread.

@daka1510 daka1510 marked this pull request as ready for review January 21, 2022 14:42
@daka1510 daka1510 self-assigned this Jan 21, 2022
@daka1510 daka1510 requested a review from kt474 January 21, 2022 14:56
@daka1510 daka1510 mentioned this pull request Jan 21, 2022
@rathishcholarajan
Copy link
Copy Markdown
Member

I've set the environment secrets in the main repo.

@daka1510
Copy link
Copy Markdown
Contributor Author

Thanks @rathishcholarajan for setting this up. I believe we still need to accept the risk to integrate it in order to see if the pieces work together. Let me know if anything is missing from my side.

Copy link
Copy Markdown
Member

@rathishcholarajan rathishcholarajan left a comment

Choose a reason for hiding this comment

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

LGTM! This PR removes ability to run slow tests. We should handle this using this ticket #127

I see that a lot of tutorial tests are slow tests https://github.com/daka1510/qiskit-ibm-runtime/runs/4969466053?check_suite_focus=true#step:5:2256 and they are skipped. I am hesitant to merge this until we can figure out a way to run these slow tests in this PR or in a separate PR (and I want to merge both PRs together). (Initially I thought it was just one slow test, but now I see a lot after merging the tutorials PR)

@daka1510
Copy link
Copy Markdown
Contributor Author

daka1510 commented Jan 28, 2022

@rathishcholarajan - good catch that the tutorial tests are also "slow tests".

I just checked the logs of the most recent slow-test and found those tests are failing anyway with different errors that do not appear like "intermittent instabilities", e.g.

ImportError: cannot import name 'UserMessenger' from 'qiskit_ibm_runtime' (/Users/runner/work/qiskit-ibm-runtime/qiskit-ibm-runtime/qiskit_ibm_runtime/__init__.py)
ValueError: No default legacy account saved.

I am happy to prioritize #127 right after this PR is integrated and added a second acceptance criteria that covers fixing those tutorial test cases as well.

I don't see much value of reenabling slow tests in this PR as they will definitely continue to fail. Let me know if you feel strongly about it and I will add a workflow definition file that runs the failing slow tests as before (just separately against cloud/legacy).

@rathishcholarajan rathishcholarajan merged commit 96ac16e into Qiskit:main Jan 28, 2022
@daka1510 daka1510 mentioned this pull request Jan 31, 2022
3 tasks
@daka1510 daka1510 deleted the refactor-test-workflow branch January 31, 2022 12:34
blakejohnson pushed a commit to blakejohnson/qiskit-ibm-runtime that referenced this pull request May 26, 2023
* fix counts in tests

* fix qaoa test

* update vqe

* fix cloud programs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor environment variables used by test cases Document env variables to set in contribution guidelines

2 participants