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

Support IBMBackend.run() #1138

Merged
merged 70 commits into from
Nov 7, 2023
Merged

Support IBMBackend.run() #1138

merged 70 commits into from
Nov 7, 2023

Conversation

merav-aharoni
Copy link
Contributor

@merav-aharoni merav-aharoni commented Oct 9, 2023

Summary

The backend.run() functionality was copied from qiskit-ibm-provider. I made adjustment as needed. Also copied all the tests that I thought were relevant.
A few open issues:

  • Should we use qiskit-ibm-runtime.Options for IBMBackend?
  • Unite Session from provider with Session here. Currently have 2 separate classes - TBD in Enable Session to contain both primitive.run and backend.run #1180.
  • Do we want to unite test-ibm-job.py with test-job.py? or keep it separate and change the name? - for now keeping both.
  • Do we want to keep all the status values from qiskit-ibm-provider?
  • Unite exceptions from qiskit-ibm-provider with those here - do when the provider is merged into runtime.
  • Are the tests in test_ibm_job.py redundant wrt existing tests here? - for now keeping all tests here.
  • test/fake_account_client.py do we need the methods here or should they be replaced by something already here? - issue Check if there is duplication between test/fake_account_client.py and other test method #1160
  • Do we want to support statuses in Qiskit.RuntimeService.jobs() as in provider.backend.jobs()? See for example test_retrieve_jobs_with_status in the provider. - No, see @jyu00 's comment below.
  • RuntimeJob.test_refresh_job_result, test_job_tags_replace skipped - how do we support refresh in runtime? - issue Support refresh functionality #1159.
  • RuntimeJob.test_queue_info - Do we want to support queue_info in runtime? - issue support queue_info #1161.
  • RuntimeJob.time_per_step - do we want to support it? - on hold for now.
  • transpiler/passmanger directory was copied over to support _deprecate_id_instruction, which is already deprecated. Can we remove? - removed _deprecate_id_instruction.
  • Tutorial - start by writing documentation - issue Migration documentation for backend.run #1181.

Details and comments

Fixes #1117

@coveralls
Copy link

coveralls commented Oct 9, 2023

Pull Request Test Coverage Report for Build 6790823703

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 93 of 1048 (8.87%) changed or added relevant lines in 16 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-14.4%) to 55.975%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_ibm_runtime/runtime_job.py 4 5 80.0%
qiskit_ibm_runtime/utils/utils.py 2 3 66.67%
qiskit_ibm_runtime/transpiler/passes/basis/init.py 0 2 0.0%
qiskit_ibm_runtime/transpiler/passes/init.py 0 5 0.0%
qiskit_ibm_runtime/transpiler/passes/scheduling/init.py 0 6 0.0%
qiskit_ibm_runtime/transpiler/passes/scheduling/pad_delay.py 0 14 0.0%
qiskit_ibm_runtime/transpiler/plugin.py 14 31 45.16%
qiskit_ibm_runtime/ibm_backend.py 68 107 63.55%
qiskit_ibm_runtime/transpiler/passes/basis/convert_id_to_delay.py 0 44 0.0%
qiskit_ibm_runtime/transpiler/passes/scheduling/utils.py 0 134 0.0%
Totals Coverage Status
Change from base Build 6787411813: -14.4%
Covered Lines: 2464
Relevant Lines: 4402

💛 - Coveralls

@merav-aharoni
Copy link
Contributor Author

merav-aharoni commented Oct 16, 2023

@jyu00 , @kt474 - I would appreciate your inputs on the tests for this PR before I continue. My approach has been the following:
I am transferring all the tests that seem relevant from qiskit-ibm-provider and making adjustments as needed. So far, I transferred:

  • all tests from provider/test/integration/test_backend.py to runtime/test/integration/test_backend.py

  • the complete files provider/integration/test_ibm_job.py and test_ibm_job_attributes.py as new integration test files

  • copied tests from provider/test/integration/test_session.py to runtime/test/integration/test_session.py under a new class: TestBackendRunInSession.

  • the complete file provider/test/unit/test_backend.py as a new unit test file

  • all tests from provider/test/unit/test_ibm_job_state.py to runtime/test/unit/test_ibm_job_state.py.

  • Do we want to copy the tests in provider/integration/test_ibm_qasm_simulator.py? They are not specific for backend.run, but I didn't see anything similar in this repo.
    Many of the changes I made in this PR were required to run all these tests.
    I am mostly looking at test files that have backend.run() and transferring those tests, and others that seem relevant (e.g., related to job).

  • Any comments on the above?

  • Any additional tests you can think of? For example, should we do something similar to test_ibm_primitives.py?

@merav-aharoni merav-aharoni marked this pull request as ready for review October 17, 2023 15:14
@merav-aharoni merav-aharoni requested review from jyu00 and kt474 October 17, 2023 15:14
@jyu00
Copy link
Collaborator

jyu00 commented Oct 17, 2023

A few open issues:

  • Should we use qiskit-ibm-runtime.Options for IBMBackend?

No.

  • Do we want to keep all the status values from qiskit-ibm-provider?

Are you referring to job status? Don't they come from Qiskit?

  • test/fake_account_client.py do we need the methods here or should they be replaced by something already here?

Replacing with something already here seems easier?

  • Do we want to support statuses in Qiskit.RuntimeService.jobs() as in provider.backend.jobs()? See for example test_retrieve_jobs_with_status in the provider.

No. Qiskit Runtime API doesn't support filtering by granular statuses, so we had to do some complicated stuff in qiskit-ibm-provider for backward compatibility. Since no one has complained about not having this support in qiskit-ibm-runtime, let's just leave out the complexity.

  • RuntimeJob.test_refresh_job_result, test_job_tags_replace skipped - how do we support refresh in runtime?

I guess we can keep refresh since it was a feature request, and it should be fairly easy to do (e.g. just call the API again).

  • RuntimeJob.test_queue_info - Do we want to support queue_info in runtime?

Yeah, I think people will be more interested in estimated_start_time etc once that becomes more accurate lol.

  • RuntimeJob.time_per_step - do we want to support it?

Let's not do this right now, since it'll change with granular job status.

  • transpiler/passmanger directory was copied over to support _deprecate_id_instruction, which is already deprecated. Can we remove?

I don't understand your comment about the transpiler/passmanger directory, but we can remove _deprecate_id_instruction.

Do we want to copy the tests in provider/integration/test_ibm_qasm_simulator.py?

Yes. They are good to ensure if something goes horribly wrong by verifying the results. It'd be good to have similar tests for real backends as well, but result verification could be difficult.

Any additional tests you can think of?

How about test_ibm_job_attributes? Some of the skipped tests can actually be re-enabled. For example, we can do test_esp_readout_enabled since we can retrieve job inputs from runtime API.

qiskit_ibm_runtime/ibm_backend.py Show resolved Hide resolved
qiskit_ibm_runtime/ibm_backend.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/ibm_backend.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/ibm_backend.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/ibm_backend.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/ibm_backend.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@merav-aharoni
Copy link
Contributor Author

@jyu00 - I addressed all your comments from yesterday. Please have a look.

qiskit_ibm_runtime/ibm_backend.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/ibm_backend.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/ibm_backend.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jyu00 jyu00 left a comment

Choose a reason for hiding this comment

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

One small comment but otherwise LGTM

qiskit_ibm_runtime/ibm_backend.py Outdated Show resolved Hide resolved
@kt474
Copy link
Member

kt474 commented Nov 7, 2023

Thanks @merav-aharoni! 🎉

@kt474 kt474 merged commit 1fccd8e into Qiskit:main Nov 7, 2023
14 checks passed
@kt474 kt474 added the Changelog: New Feature Include in the Added section of the changelog label Nov 7, 2023
@merav-aharoni merav-aharoni deleted the backend_run branch November 8, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the Added section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support backend.run() in IBM Cloud channel
4 participants