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

Add mode parameter #1584

Merged
merged 55 commits into from
Apr 18, 2024
Merged

Add mode parameter #1584

merged 55 commits into from
Apr 18, 2024

Conversation

ptristan3
Copy link
Collaborator

@ptristan3 ptristan3 commented Apr 3, 2024

Summary

  • Add mode param to Sampler and Estimator. the new param can be: Backend, Session, Batch, or None
  • Stop supporting a backend name as a valid execution mode.
  • Deprecate Session and Backend params

Details and comments

Fixes #1556

@ptristan3 ptristan3 added the Changelog: Deprecation Include in Deprecated section of changelog label Apr 5, 2024
@ptristan3 ptristan3 added the Changelog: New Feature Include in the Added section of the changelog label Apr 5, 2024
@ptristan3 ptristan3 marked this pull request as ready for review April 5, 2024 17:25
@coveralls
Copy link

coveralls commented Apr 5, 2024

Pull Request Test Coverage Report for Build 8739878057

Details

  • 47 of 50 (94.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 83.649%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_ibm_runtime/base_primitive.py 23 26 88.46%
Totals Coverage Status
Change from base Build 8738780942: 0.03%
Covered Lines: 6272
Relevant Lines: 7498

💛 - Coveralls

@ptristan3 ptristan3 requested review from kt474 and jyu00 April 5, 2024 17:25
qiskit_ibm_runtime/base_primitive.py Outdated Show resolved Hide resolved
Comment on lines +171 to +172
def mode(self) -> Optional[Session | Batch]:
"""Return the execution mode used by this primitive.
Copy link
Collaborator

@jyu00 jyu00 Apr 5, 2024

Choose a reason for hiding this comment

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

We can't just remove the session attribute without deprecating it first.

qiskit_ibm_runtime/estimator.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/estimator.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/estimator.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/estimator.py Outdated Show resolved Hide resolved
"The session argument is going to be consolidated into the mode param.",
)
if isinstance(mode, str) or isinstance(backend, str):
raise ValueError(
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned earlier, this needs to be deprecated first.

Comment on lines 255 to 256
mode: Optional[Union[IBMBackend, Session, Batch]] = None,
backend: Optional[IBMBackend] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't actually need to change V1 primitive since we are deprecating it already

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.

Some minor comments but overall looks good!

qiskit_ibm_runtime/estimator.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/sampler.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/base_primitive.py Outdated Show resolved Hide resolved
Comment on lines 160 to 166
def test_init_with_session_backend_str(self, primitive):
"""Test initializing a primitive with a backend name using session."""
backend_name = "ibm_gotham"

with patch("qiskit_ibm_runtime.base_primitive.QiskitRuntimeService"):
with self.assertRaises(ValueError) as exc:
inst = primitive(session=backend_name)
self.assertIsNone(inst.session)
self.assertIn("session must be of type Session or None", str(exc.exception))
inst = primitive(session=backend_name)
self.assertIsNone(inst.mode)
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 see the point of having this test now. It was added to ensure one couldn't specify a backend name as a session (which was allowed way back). Now that it no longer raises an error but violates type hint, I'm not sure what the test is trying to cover.

test/unit/test_ibm_primitives_v2.py Outdated Show resolved Hide resolved
@kt474 kt474 merged commit bc4d01c into Qiskit:main Apr 18, 2024
20 checks passed
@garrison
Copy link
Member

This was merged after qiskit-ibm-runtime 0.23.0 was released. Since this change first appears in 0.24.0, the deprecation warnings should refer to 0.24.0, not 0.23.0

@kt474
Copy link
Member

kt474 commented Jun 12, 2024

This was merged after qiskit-ibm-runtime 0.23.0 was released. Since this change first appears in 0.24.0, the deprecation warnings should refer to 0.24.0, not 0.23.0

Sorry we missed this, we'll fix in a patch release

garrison added a commit to Qiskit/qiskit-addon-cutting that referenced this pull request Jun 12, 2024
This is necessary to prevent a `DeprecationWarning` under
qiskit-ibm-runtime 0.24, which is currently visible in the
[first](https://qiskit-extensions.github.io/circuit-knitting-toolbox/circuit_cutting/tutorials/01_gate_cutting_to_reduce_circuit_width.html)
and third tutorials and is due to the merging of
Qiskit/qiskit-ibm-runtime#1584.

qiskit-ibm-runtime 0.24 depends on qiskit>=1.1, so I had to
bump the qiskit version as well.

Technically, this version bump is only required by the notebooks,
not the actual CKT library, but the "minimum version tests" aren't
equipped to deal with situations where the notebooks have a
different minimum version than the library.
garrison added a commit to Qiskit/qiskit-addon-cutting that referenced this pull request Jun 17, 2024
This is necessary to prevent a `DeprecationWarning` under
qiskit-ibm-runtime 0.24, which is currently visible in the
[first](https://qiskit-extensions.github.io/circuit-knitting-toolbox/circuit_cutting/tutorials/01_gate_cutting_to_reduce_circuit_width.html)
and third tutorials and is due to the merging of
Qiskit/qiskit-ibm-runtime#1584.

qiskit-ibm-runtime 0.24 depends on qiskit>=1.1, so I had to
bump the qiskit version as well.

Technically, this version bump is only required by the notebooks,
not the actual CKT library, but the "minimum version tests" aren't
equipped to deal with situations where the notebooks have a
different minimum version than the library.
garrison added a commit to Qiskit/qiskit-addon-cutting that referenced this pull request Jun 17, 2024
This is necessary to prevent a `DeprecationWarning` under
qiskit-ibm-runtime 0.24, which is currently visible in the
[first](https://qiskit-extensions.github.io/circuit-knitting-toolbox/circuit_cutting/tutorials/01_gate_cutting_to_reduce_circuit_width.html)
and third tutorials and is due to the merging of
Qiskit/qiskit-ibm-runtime#1584.

This means that the notebooks now depend on
qiskit-ibm-runtime 0.24, which depends on qiskit>=1.1.

The "minimum version tests" aren't equipped to deal with situations
where the notebooks have a different minimum version than the
library.  For this reason I've disabled the "minimum version tests"
on the current stable branch going forward for the notebooks only.
I don't want to bump the minimum version of any library on the
stable branch, but I _do_ want to update our code examples
to the latest version of qiskit-ibm-runtime.
garrison added a commit to Qiskit/qiskit-addon-cutting that referenced this pull request Jul 8, 2024
…628)

This is necessary to prevent a `DeprecationWarning` under
qiskit-ibm-runtime 0.24, which is currently visible in the
[first](https://qiskit-extensions.github.io/circuit-knitting-toolbox/circuit_cutting/tutorials/01_gate_cutting_to_reduce_circuit_width.html)
and third tutorials and is due to the merging of
Qiskit/qiskit-ibm-runtime#1584.

This means that the notebooks now depend on
qiskit-ibm-runtime 0.24, which depends on qiskit>=1.1.

The "minimum version tests" aren't equipped to deal with situations
where the notebooks have a different minimum version than the
library.  For this reason I've disabled the "minimum version tests"
on the current stable branch going forward for the notebooks only.
I don't want to bump the minimum version of any library on the
stable branch, but I _do_ want to update our code examples
to the latest version of qiskit-ibm-runtime.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Deprecation Include in Deprecated section of changelog Changelog: New Feature Include in the Added section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename session parameter in primitives
5 participants