Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Initial support for session #675

Closed
wants to merge 27 commits into from

Conversation

merav-aharoni
Copy link
Collaborator

@merav-aharoni merav-aharoni commented Jul 20, 2023

Summary

Adding support for Session in qiskit-ibm-provider.

Details and comments

This update introduces duplications between this repo and qiskit-ibm-runtime. They should be addressed after this one is merged. See linked issue below.
Fixes #588.

@coveralls
Copy link

coveralls commented Jul 20, 2023

Pull Request Test Coverage Report for Build 5750015310

  • 57 of 83 (68.67%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 51.39%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_ibm_provider/api/clients/runtime.py 1 2 50.0%
qiskit_ibm_provider/api/rest/runtime.py 2 3 66.67%
qiskit_ibm_provider/api/rest/runtime_session.py 8 11 72.73%
qiskit_ibm_provider/ibm_provider.py 3 6 50.0%
qiskit_ibm_provider/ibm_backend.py 7 11 63.64%
qiskit_ibm_provider/utils/converters.py 9 14 64.29%
qiskit_ibm_provider/session.py 26 35 74.29%
Totals Coverage Status
Change from base Build 5728346612: 0.2%
Covered Lines: 3364
Relevant Lines: 6546

💛 - Coveralls

@merav-aharoni
Copy link
Collaborator Author

merav-aharoni commented Jul 20, 2023

Items that need to be addressed:

  • Duplication between class Session here and in qiskit_ibm_runtime. How do we want to resolve this?
  • The file qiskit_ibm_runtime/api/session.py - is it used anywhere? do we need it in provider?
  • Support Session.close()
  • hms_to_seconds was copied from qiskit_ibm_runtime.utils.converters.py. Probably best to unite the converters.py file here and there, and leave only one - TBD in qiskit-ibm-runtime.
  • Additional tests. Ideas?
  • [x ] Session._circuits_map - do we need this field? seems to me it is not used in qiskit_ibm_runtime. Removed here.
  • Documentation.
  • Do we want to support the default session? I suggest we remove it here.
  • Update the tutorial - I suggest leaving this for a separate PR.

@merav-aharoni
Copy link
Collaborator Author

@kt474 - this PR is not quite ready for review, however it would be great if you could do a preliminary review over what I did until now, just to see if I am in the right direction.
I also wrote a list of items that have yet to be addressed. You are welcome to add your comments there as well.

@kt474
Copy link
Member

kt474 commented Jul 20, 2023

Looks good so far! To answer some of your questions:

  • With duplicated code, we want to have qiskit-ibm-runtime depend on qiskit-ibm-provider so once this is merged we can remove duplicates from qiskit-ibm-runtime.
  • The naming for qiskit_ibm_runtime/api/session.py is a bit confusing - I don't think it's necessary in this provider for now. It's mostly used in runtime for data tracking from the client header.
  • Yes it would be nice to have just one converters.py, uniting all the methods and remove the one in qiskit-ibm-runtime
  • For testing, we can have something similar to qiskit-ibm-runtime. Unit tests for checking values are passed correctly and integration tests to check multiple circuit-runner jobs.
  • Session._circuits_map is not necessary and probably could just be removed from qiskit-ibm-runtime too - it was originally added (9 months ago) for data caching but that ended up creating a lot more problems so that's why it's currently commented out

@merav-aharoni
Copy link
Collaborator Author

See also related issue: Qiskit/qiskit-ibm-runtime#969.

@merav-aharoni merav-aharoni marked this pull request as ready for review July 24, 2023 14:55
@merav-aharoni merav-aharoni requested review from kt474 and jyu00 July 24, 2023 14:55
@merav-aharoni
Copy link
Collaborator Author

@kt474 - I think this is ready for review. Please also see my comments above again, as I made some additions there.

Copy link
Member

@kt474 kt474 left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

Running backend.run() jobs in a session seems to be working correctly but there seems to be an issue with jobs not in sessions

  • It would be good to double check the integration tests are still passing (they seem to be timing out for me right now)
backend = provider.get_backend('ibmq_qasm_simulator')
circuit = ReferenceCircuits.bell()
with Session(backend_name=backend) as session:
    job = backend.run(circuit)
    print(job.result())
    session.close()

job2 = backend.run(circuit) # this is in a new session but should not be in any session


def __init__(
self,
backend_name: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with runtime, we should probably use backend here instead of backend_name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could, but I wanted to stress that here it is only a string and not an actual backend. In runtime, it can be either.

test/integration/test_session.py Outdated Show resolved Hide resolved
@merav-aharoni
Copy link
Collaborator Author

Looks pretty good!

Running backend.run() jobs in a session seems to be working correctly but there seems to be an issue with jobs not in sessions

  • It would be good to double check the integration tests are still passing (they seem to be timing out for me right now)
backend = provider.get_backend('ibmq_qasm_simulator')
circuit = Re

Actually, both jobs here are running without a session. The way I implemented it, Session only receives a backend name, not an actual backend. Session needs to get the actual backend from the provider.
So the real question is if we are happy with the way it is now, or if we want the Session to also be able to receive an IBMBackend in its __init__. If we support this case, then the Session will need to get the IBMProvider from the backend, and will then need to set itself as the IBMProvider._session. This seems rather circular to me, so I didn't like it. @kt474 - what do you think?

@merav-aharoni merav-aharoni mentioned this pull request Aug 6, 2023
@merav-aharoni
Copy link
Collaborator Author

This PR is replaced by #699.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support session
3 participants