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

Choose correct backend within a session #628

Merged
merged 15 commits into from
Feb 22, 2023
Merged

Conversation

kt474
Copy link
Member

@kt474 kt474 commented Nov 19, 2022

Summary

Details and comments

Fixes #627

@coveralls
Copy link

coveralls commented Nov 19, 2022

Pull Request Test Coverage Report for Build 4246568333

  • 6 of 7 (85.71%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 66.673%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_ibm_runtime/session.py 6 7 85.71%
Files with Coverage Reduction New Missed Lines %
qiskit_ibm_runtime/api/clients/base.py 4 87.29%
Totals Coverage Status
Change from base Build 4244410013: 0.02%
Covered Lines: 3275
Relevant Lines: 4912

💛 - Coveralls

@kt474 kt474 marked this pull request as ready for review November 28, 2022 22:58
@kt474 kt474 requested a review from jyu00 November 29, 2022 00:07
@mriedem
Copy link
Contributor

mriedem commented Nov 29, 2022

Test for this? It's not clear to me how this resolves the issue without a test.

@kt474 kt474 marked this pull request as draft November 30, 2022 19:58
@kt474 kt474 marked this pull request as ready for review December 19, 2022 17:32
with Session(service=service, backend="statevector_simulator") as session:
job = session.run(program_id="hello-world", options=options, inputs=inputs)
self.assertEqual(backend, job.backend().name)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jyu00 my understanding is that in a scenario like this the backend passed in through options should be honored?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, since the docstring says options can be anything in RuntimeOptions (which includes backend). We should change that though since you can't have a session cross multiple backends.

Comment on lines 144 to 145
if "backend" not in options:
options["backend"] = self._backend
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you raise an exception if a session is already active, has a backend, and the backend passed in options doesn't match.

qiskit_ibm_runtime/session.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/session.py Outdated Show resolved Hide resolved
3,
)
if (
self._session_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to compare session id here? If I do

session = Session(backend="foo")
session.run(options={"backend": "bar"})

^ this should fail because it's ambiguous which backend I really want.

qiskit_ibm_runtime/session.py Show resolved Hide resolved
kt474 and others added 4 commits January 5, 2023 19:31
Co-authored-by: Jessie Yu <jessieyu@us.ibm.com>
Co-authored-by: Jessie Yu <jessieyu@us.ibm.com>
@kt474 kt474 requested a review from jyu00 January 6, 2023 01:41
@kt474 kt474 added this to the 0.9.0 milestone Feb 6, 2023
@kt474 kt474 modified the milestones: 0.9.0 , 0.9.1 Feb 16, 2023
@kt474 kt474 added the Changelog: Bugfix Include in the Fixed section of the changelog label Feb 22, 2023
@kt474 kt474 merged commit 222461b into Qiskit:main Feb 22, 2023
Comment on lines +162 to +163
if self._backend and options["backend"] != self._backend:
raise IBMInputValueError(
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no test coverage for this is there?

Copy link
Member Author

Choose a reason for hiding this comment

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

added test case here #763

kt474 added a commit to kt474/qiskit-ibm-runtime that referenced this pull request Mar 23, 2023
kt474 added a commit that referenced this pull request Mar 24, 2023
@kt474 kt474 deleted the correct-backend branch August 11, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the Fixed section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backend specified on job submitted as part of a session is not honored
4 participants