qiskit-community / qiskit-quantinuum-provider

Qiskit provider for Quantinuum backends.
Apache License 2.0
21 stars 17 forks source link

Update provider to versioned provider interface #14

Closed mtreinish closed 2 years ago

mtreinish commented 3 years ago

Summary

This commit updates the honeywell provider to use the latest versioned abstract interface from terra (which was introduced in 0.16.0). Terra will be deprecating the legacy interface in the near future, so to get in front of that we should update the provider to use the current interface version. At the same time this fixes some compatibility issues that arise when running with the latest version of terra (which is necessary because the minimum version is bumped to ensure the new provider interface exists).

Details and comments

mtreinish commented 3 years ago

I don't have any way to test this, so I assume I missed something. But I don't have any method to run against the honeywell API (and don't know anyone who does) so I'm not sure how to fix that.

dan1pal commented 3 years ago

@mtreinish, we can give you access to our QA environment if it helps and will test this update shortly.

mtreinish commented 3 years ago

@mtreinish, we can give you access to our QA environment if it helps and will test this update shortly.

Yes, that would help a lot and let me locally test these changes to verify everything still works with this PR applied.

We also could follow up and have a post merge CI job that tests the provider against the QA env too, because most of the tests in the unittests are skipped conditional on having credentials to hit the api.

dan1pal commented 3 years ago

Yes, that would help a lot and let me locally test these changes to verify everything still works with this PR applied.

@mtreinish, please reach out via email to alexander.chernoguzov@honeywell.com to obtain login credentials at your convenience.

dlucchetti commented 3 years ago

While you are getting access to our testing infrastructure. The following were found to be the minimum update to pass our regression tests. I have not had, as yet, luck migrating past pyjwt 1.7.1. If it is a requirement that we update to 2.x, then additional updates will be required.

index 97137d4..7a7d62b 100644
--- a/qiskit/providers/honeywell/credentials/credentials.py
+++ b/qiskit/providers/honeywell/credentials/credentials.py
@@ -277,7 +277,7 @@ class Credentials:
             id_token = self._get_token('id_token')

         # check id_token is not expired yet
-        expiration_date = jwt.decode(id_token, verify=False)['exp']
+        expiration_date = jwt.decode(id_token, verify=False, algorithms=["RS256"])['exp']
         if expiration_date < (datetime.datetime.now(datetime.timezone.utc).timestamp()):
             print("Your id token is expired. Refreshing...")

diff --git a/qiskit/providers/honeywell/honeywelljob.py b/qiskit/providers/honeywell/honeywelljob.py
index 147bfdb..6806d71 100644
--- a/qiskit/providers/honeywell/honeywelljob.py
+++ b/qiskit/providers/honeywell/honeywelljob.py
@@ -160,6 +160,7 @@ class HoneywellJob(JobV1):
         self._job_ids = []
         self._experiment_results = []

+        self._qobj_payload = {}
         if circuits:
             if isinstance(circuits, qobj_mod.QasmQobj):
                 self._qobj_payload = circuits.to_dict()
@@ -171,7 +172,6 @@ class HoneywellJob(JobV1):
                 self._experiments = circuits
                 self._job_config = job_config
         else:
-            self._qobj_payload = {}
             self._status = JobStatus.INITIALIZING
             self._job_ids.append(job_id)

diff --git a/requirements.txt b/requirements.txt
index c712420..4844323 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -2,5 +2,5 @@ nest-asyncio>=1.2.0
 qiskit-terra>=0.16.0
 requests>=2.19
 websockets>=7
-pyjwt>=1.7.1
+pyjwt>=1.7.1,<2.0
 keyring>=10.6.0
diff --git a/setup.py b/setup.py
index 618a45d..1fa3fcc 100644
--- a/setup.py
+++ b/setup.py
@@ -35,7 +35,7 @@ requirements = [
     'qiskit-terra>=0.16.0',
     'requests>=2.19',
     'websockets>=7',
-    'pyjwt>=1.7.1',
+    'pyjwt>=1.7.1,<2',
     'keyring>=10.6.0',
 ]

diff --git a/tox.ini b/tox.ini
index 5e0c6eb..6d4a87f 100644
--- a/tox.ini
+++ b/tox.ini
@@ -23,7 +23,7 @@ deps =
   pycodestyle
   pylint
   setuptools>=40.1.0
-  pyjwt>=1.7.1
+  pyjwt>=1.7.1,<2
   keyring>=10.6.0
 commands =
   pycodestyle qiskit/providers/honeywell test/
mtreinish commented 3 years ago

Yeah, I think capping pyjwt is fine, especially against a major version boundary like that. If/when we find a pattern for pyjwt >= 2.0.0 we can do that in a followup. @dlucchetti do you want to open a separate PR with those fixes so we can get the main branch working again and then I'll rebase this on top of that?