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

Deprecate auth parameter and add new channel parameter #226

Merged
merged 31 commits into from
Mar 29, 2022

Conversation

rathishcholarajan
Copy link
Member

@rathishcholarajan rathishcholarajan commented Mar 21, 2022

Summary

Deprecate existing auth parameter to IBMRuntimeService and introduce new channel parameter to better accommodate the different channels that may be available in the future.

channel="ibm_quantum"
channel="ibm_cloud"

Details and comments

Fixes #217

TODO

  • Add channel parameter and replace auth everywhere with channel to make channel work (auth gets broken at the end of this step)
  • Deprecate auth parameter and fix code now to make auth also to work alongside channel
    • Save account (save_account)
    • Delete account (delete_account)
    • List saved accounts (saved_accounts)
    • Enable account (__init__)
  • Add new environments ibm-quantum-production, ibm-quantum-staging, ibm-cloud-production and ibm-cloud-staging.
  • Duplicate some channel tests and make them work using auth flow
    • Add unit tests for save account
    • Add unit tests for listing saved accounts
    • Add unit tests for deleting account

auth parameter and the tests related to it will be removed in a later release as per Qiskit deprecation policy.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@coveralls
Copy link

coveralls commented Mar 25, 2022

Pull Request Test Coverage Report for Build 2042278316

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 77 of 89 (86.52%) changed or added relevant lines in 8 files are covered.
  • 111 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.9%) to 69.963%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_ibm_runtime/accounts/management.py 31 32 96.88%
qiskit_ibm_runtime/api/auth.py 2 3 66.67%
qiskit_ibm_runtime/api/clients/auth.py 1 2 50.0%
qiskit_ibm_runtime/accounts/account.py 15 17 88.24%
qiskit_ibm_runtime/ibm_runtime_service.py 24 31 77.42%
Files with Coverage Reduction New Missed Lines %
qiskit_ibm_runtime/accounts/management.py 2 96.34%
qiskit_ibm_runtime/runtime_options.py 2 89.47%
qiskit_ibm_runtime/accounts/account.py 4 93.33%
qiskit_ibm_runtime/accounts/storage.py 5 88.24%
qiskit_ibm_runtime/utils/utils.py 17 54.55%
qiskit_ibm_runtime/ibm_runtime_service.py 81 75.92%
Totals Coverage Status
Change from base Build 2039951690: 0.9%
Covered Lines: 2080
Relevant Lines: 2973

💛 - Coveralls

@rathishcholarajan rathishcholarajan marked this pull request as ready for review March 26, 2022 01:43
@rathishcholarajan rathishcholarajan added the Changelog: API Change Include in the Changed section of the changelog label Mar 26, 2022
Copy link
Collaborator

@daka1510 daka1510 left a comment

Choose a reason for hiding this comment

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

@rathishcholarajan carefully reviewed the changes and left a few comments. Let me know if you disagree or anything is unclear.

docs/max_time.rst Outdated Show resolved Hide resolved
docs/release_notes.rst Show resolved Hide resolved
docs/tutorials/04_account_management.ipynb Outdated Show resolved Hide resolved
qiskit_ibm_runtime/ibm_runtime_service.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/accounts/account.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/accounts/storage.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/accounts/management.py Show resolved Hide resolved
Copy link
Collaborator

@daka1510 daka1510 left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks!! I find the new terminology much clearer.

Comment on lines +183 to +219
def migrate(cls) -> None:
"""Migrate accounts on disk by removing `auth` and adding `channel`."""
data = read_config(filename=_DEFAULT_ACCOUNT_CONFIG_JSON_FILE)
for key, value in data.items():
if key == _DEFAULT_ACCOUNT_NAME_CLOUD:
value.pop("auth", None)
value.update(channel="ibm_cloud")
delete_config(filename=_DEFAULT_ACCOUNT_CONFIG_JSON_FILE, name=key)
save_config(
filename=_DEFAULT_ACCOUNT_CONFIG_JSON_FILE,
name=_DEFAULT_ACCOUNT_NAME_IBM_CLOUD,
config=value,
overwrite=False,
)
elif key == _DEFAULT_ACCOUNT_NAME_LEGACY:
value.pop("auth", None)
value.update(channel="ibm_quantum")
delete_config(filename=_DEFAULT_ACCOUNT_CONFIG_JSON_FILE, name=key)
save_config(
filename=_DEFAULT_ACCOUNT_CONFIG_JSON_FILE,
name=_DEFAULT_ACCOUNT_NAME_IBM_QUANTUM,
config=value,
overwrite=False,
)
else:
if hasattr(value, "auth"):
if value["auth"] == "cloud":
value.update(channel="ibm_cloud")
elif value["auth"] == "legacy":
value.update(channel="ibm_quantum")
value.pop("auth", None)
save_config(
filename=_DEFAULT_ACCOUNT_CONFIG_JSON_FILE,
name=key,
config=value,
overwrite=True,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this a lot easier to understand than the previous approach, thanks for updating the PR accordingly!

Instead of persisting the changes for every stored account, we could also make all updates in-memory and overwrite the file with a single write operation. Given that this is temporary migration code and I don't expect users to have hundreds of account I'm personally fine with this. I don't think we have to optimize at this level of detail.

@daka1510
Copy link
Collaborator

No objections from my side if the CI checks pass...

@rathishcholarajan rathishcholarajan merged commit 0dd4747 into Qiskit:main Mar 29, 2022
@rathishcholarajan rathishcholarajan deleted the 217-channel-parameter branch March 29, 2022 16:56
@rathishcholarajan rathishcholarajan mentioned this pull request Mar 31, 2022
blakejohnson pushed a commit to blakejohnson/qiskit-ibm-runtime that referenced this pull request May 26, 2023
* Add error handling of separate cregs

* do not raise an error unless error mitigation is enabled

* always check unused classical bits

* revert a test name

* avoid applying correction to classical bits not used for measurements

* update an error messgage

* isort

* Update test/unit/test_sampler.py

Co-authored-by: Mariana Bernagozzi <[email protected]>

* add MidcircuitMeasurementError

Co-authored-by: Rathish Cholarajan <[email protected]>
Co-authored-by: JESSIE YU <[email protected]>
Co-authored-by: Mariana Bernagozzi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the Changed section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate auth parameter and introduce new channel parameter
3 participants