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

run_batch method for IBMBackendWrapper class #246

Merged
merged 7 commits into from
Jun 13, 2023

Conversation

TheGupta2012
Copy link
Collaborator

@TheGupta2012 TheGupta2012 commented May 28, 2023

Issue tags : devices api unitaryhack

Changes:

Fixes #231

Adds a new method run_batch to the IBMBackendWrapper class and updates the init_job method in qbraid/api/job_api.py

Details or comments:

The run_batch method has almost similar logic as the run method, just extended to a list.
Moreover, the init_job method now accepts a list of circuits. The major change is in the circuitDepth and circuitNumQubits parameters of the request body. The values are now a list instead of an int.

I wanted to first confirm if this change in init_body is not a breaking change for posting requests to the qBraid platform.

Checklist before merge

  • Is change to init_job breaking? Not really, needed to add different fields
  • Add unit tests

General

  • I have read the CONTRIBUTING doc.

  • ALL new features must include unit test to the test directory, codecov will check for any missing test code exist.

  • Ensure that all the test passes.

@ryanhill1
Copy link
Member

@TheGupta2012 These changes seem good so far! For next steps, we look like to make sure that the propagation of the run_batch method to the IBMJobWrapper and IBMResultWrapper are each working as expected. For example, in the README Devices & Jobs example, if you were to replace the ibm_device.run(circuit) method with ibm_device.run([circuit1, circuit2, ...]) would ibm_job.status() and ibm_result.measurement_counts() still work? There may be further modifications needed in these classes to account for multiple jobs/results. And finally, we will want tests to verify that all of these steps work together nicely.

@TheGupta2012
Copy link
Collaborator Author

Hi @ryanhill1 , I have added the necessary changes to the IBMResultWrapper and the general ResultWrapper class along with the relevant unit tests.

Also, the get_status method worked correctly since qiskit uses a single job id even if multiple circuits were passed in.

I tested these changes using the dummy job ids instead of calling the init_job method as my error is still not resolved.

I also saw that the CI pipeline skips the test_devices.py file during the build (which contains the added unit tests) thus it would be great if you could test this once and let me know what changes are required. Thanks!

@TheGupta2012 TheGupta2012 marked this pull request as ready for review June 8, 2023 12:09
Copy link
Member

@ryanhill1 ryanhill1 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! I looked into it, and you are correct that there is an authentication issue with the API. It seems that the init_job route was not yet added to list of routes that accepts the api-key. So we are making that update now it should be fixed within the next day!

I ran your new device tests with the API updates and typing fixes locally and got errors on the following two tests:

FAILED qbraid/devices/tests/test_devices.py::test_cancel_completed_batch_error[ibm_q_simulator_statevector] - qiskit_ibm_provider.job.exceptions.IBMJobInvalidStateError: 'Job cannot be cancelled: \'409 Client Error: Conflict for url: https://...
FAILED qbraid/devices/tests/test_devices.py::test_result_wrapper_measurements[ibm_q_simulator_statevector] - qiskit_ibm_provider.job.exceptions.IBMJobFailureError: 'Job failed: Error preprocessing job. Try again or contact support.'

The first looks like an error where it tried to cancel a job that was already complete. We've added fixes for that, so if you pull the latest changes from main, that should be resolved. The second one, I'm not as sure about.

And to address you comment about the test_devices.py being skipped: yes, you are correct. This is because those tests require IBM tokens, which we don't want to make visible through our GitHub actions. But eventually, we do want to be able to run IBM device tests without the need for extra tokens. One of the ways to do that is via local simulators and/or mock devices. Adding this would also speed up the runtime of these tests by a lot, because we wouldn't have to wait in the queue for jobs to finish just to run tests. If you're interested, you can take a look at #251, but it's definitely not required.

I will be back in touch with you about the api-token authentication within the next day or so, and hopefully have an answer for the init_jobs data array vs number types as well! Thanks for your patience while we make these API updates.

qbraid/api/job_api.py Outdated Show resolved Hide resolved
qbraid/devices/tests/test_devices.py Outdated Show resolved Hide resolved
@TheGupta2012
Copy link
Collaborator Author

Hi, thanks for the reply @ryanhill1! I merged the main branch into the current one and ran the wrapper measurement tests. I seem to be getting different errors concerned with the init_job endpoint -

FAILED qbraid/devices/tests/test_results.py::test_result_wrapper_measurements[ibm_q_simulator_statevector] - qbraid.api.exceptions.RequestsApiError: HTTPSConnectionPool(host='api.qbraid.com', port=443): Max retries exceeded with url: /api/init-job
FAILED qbraid/devices/tests/test_results.py::test_result_wrapper_batch_measurements[ibm_q_qasm_simulator] - qbraid.api.exceptions.RequestsApiError: HTTPSConnectionPool(host='api.qbraid.com', port=443): Max retries exceeded with url: /api/init-job
FAILED qbraid/devices/tests/test_results.py::test_result_wrapper_batch_measurements[ibm_q_simulator_statevector] - qbraid.api.exceptions.RequestsApiError: HTTPSConnectionPool(host='api.qbraid.com', port=443): Max retries exceeded with url: /api/init-job

When I tried to run these tests with dummy job IDs and did not use init_job, these tests did pass successfully.

Being not entirely sure about this error, I have added a @pytest.skip decorator for these tests till the error gets resolved in the init_job endpoint. Still, I think we would need to skip these tests on the CI side as they require the simulator backends and thus the service API tokens.

@ryanhill1
Copy link
Member

@TheGupta2012 a few updates!

  • The api-key issue should now be resolved! So the init_job and other requests that weren't working before should go through, barring any other problems
  • To bridge the gap for the typing update, I've added two more fields called circuitBatchNumQubits and circuitBatchDepth to the API that each accept type Array (list). So you can use those for the batched jobs data until our next update where we can change the type of the regular circuitNumQubits and circuitDepth fields. Or, maybe it might just be easier to differentiate and keep them separate?
  • I might have identified one other reason you might be experiencing API request errors, which is the required email field in the init_job() function. Namely, this line:
init_data["email"] = os.getenv("JUPYTERHUB_USER")

This environment variable is present on qBraid Lab by default, but if it is not present on your local computer, that could be causing issues. You can actually replace it with this:

init_data["email"] = session.user_email

from the QbraidSession object declared earlier in the function, and that should fix that if it was an issue.

  • And yes, as you mentioned the IBM simulator backends do require the service API tokens. So for now we do have to skip any tests that involve those. What I was suggesting in my last comment was instead using Mock IBM devices for tests, which don't require API tokens. That's what we would like to accomplish in issue Testing with Mock IBM Provider/Backend #251 . But once again, that's not directly related to this issue, so can be saved for later.

Let me know if the update and changes I suggested fix any of the API issues! And thanks for your patience as we've made these API fixes :)

@TheGupta2012
Copy link
Collaborator Author

TheGupta2012 commented Jun 9, 2023

Hi @ryanhill1 , the changes on the API side seem to be working well!

I was able to successfully use the init_job endpoint with the new fields. I have also added a change to use -1 for the circuitNumQubits and circuitDepth if a batch was passed in. In the single circuit case, both fields would be having actual values (I reckoned a single circuit is still a batch of size 1)

To address your question about the fields, I think one field per circuit property would be a cleaner way to implement it as we will have two fewer fields in the json to worry about :)

You'll be happy to know that the failing tests have passed! Let me know if you have any problems running the tests after my latest commit.

Besides that, I think the mock provider implementation would take some time so I've retained the skip test decorator for now.

Thanks a lot for catering to the API changes and all the helpful suggestions!

Copy link
Member

@ryanhill1 ryanhill1 left a comment

Choose a reason for hiding this comment

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

Just two very minor fixes to make sure tests pass/run once merged, and then we should be good to go!

qbraid/api/job_api.py Outdated Show resolved Hide resolved
qbraid/devices/tests/test_results.py Outdated Show resolved Hide resolved
@ryanhill1 ryanhill1 merged commit eb0717c into qBraid:main Jun 13, 2023
4 checks passed
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.

Implement IBMBackendWrapper.run_batch method
2 participants