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

Deprecate v1 primitives #12497

Closed
jyu00 opened this issue Jun 3, 2024 · 7 comments · Fixed by #12575
Closed

Deprecate v1 primitives #12497

jyu00 opened this issue Jun 3, 2024 · 7 comments · Fixed by #12575
Labels
type: feature request New feature or request
Milestone

Comments

@jyu00
Copy link
Contributor

jyu00 commented Jun 3, 2024

What should we add?

We should have deprecated V1 primitives in Qiskit 1.1, given its replacement (v2) had co-existed for 1 minor release. We should make sure it's deprecated in 1.2.

cc @t-imamichi, @ihincks

@jyu00 jyu00 added the type: feature request New feature or request label Jun 3, 2024
@mtreinish mtreinish added this to the 1.2.0 milestone Jun 3, 2024
@1ucian0
Copy link
Member

1ucian0 commented Jun 4, 2024

related to #11490

@1ucian0
Copy link
Member

1ucian0 commented Jul 11, 2024

I'm putting this PR on hold as there are still open conversations on if or how are we going to do this.

@1ucian0 1ucian0 added on hold Can not fix yet and removed on hold Can not fix yet labels Jul 11, 2024
@1ucian0
Copy link
Member

1ucian0 commented Jul 15, 2024

After some off line conversations, we agree too:

  • Deprecate the Interface type aliases qiskit.primitives.BaseEstimator (while leaving qiskit.primitives.BaseEstimatorV1). The recommendation is to use qiskit.primitives.BaseEstimatorV2 directly.
  • Deprecate the Interface type aliases qiskit.primitives.BaseSampler (while leaving qiskit.primitives.BaseSamplerV1). The recommendation is to use qiskit.primitives.BaseEstimatorV2 directly.
  • Deprecate qiskit.primitives.Estimator as reference implementation. The recommendation is to use qiskit.primitives.EstimatorV2 directly.
  • Deprecate qiskit.primitives.Sampler as reference implementation. The recommendation is to use qiskit.primitives.SamplerV2 directly.
  • Deprecate qiskit.primitives.backend_estimator.BackendEstimator as a tool. The recommendation is to use qiskit.primitives.BackendEstimatorV2 directly.
  • Deprecate qiskit.primitives.backend_estimator.BackendSampler as a tool. The recommendation is to use qiskit.primitives.BackendSamplerV2 directly.

@t-imamichi
Copy link
Member

t-imamichi commented Jul 24, 2024

How about other items in the PR #12575 (comment), e.g., BasePrimitivesResult , EstimatorResult, and SamplerResult? They are used for only EstimatorV1 and SamplerV1 while V2 uses PubResult and SamplerPubResult.

BasePrimitives is a base class of BaseSamplerV1 and BaseEstimatorV1.

t-imamichi added a commit to LeanderCS/Qiskit that referenced this issue Jul 24, 2024
- Revert BaseSamplerV1 and BaseEstimatorV1
- Deprecate BaseSampler and BaseEstimator

Qiskit#12497 (comment)
@ElePT
Copy link
Contributor

ElePT commented Jul 24, 2024

Given that BasePrimitives is not part of the deprecation (according to @1ucian0's message), I think it would make sense not to deprecate BasePrimitivesResult.

For EstimatorResult and SamplerResult, they are documented as the result types for the run method of BaserSamplerV1/BaseEstimatorV1, so I see a point for keeping them. Especially if we want to avoid breaking external implementations of the V1 interface (which I think is why the Base interface should be kept). However, I see how the names can lead to confusion. Would it make sense to "rename" them to SamplerV1Result or SamplerResultV1/ EstimatorV1Result or EstimatorResultV1 and deprecate the non-V1 name? In practice, we could deprecate the class and introduce a new type alias that doesn't raise a warning. This is however, this is still a breaking change, so I am not 100% sure it's the right way to go. The safest way would be keeping and not renaming them.

@t-imamichi
Copy link
Member

t-imamichi commented Jul 24, 2024

Sorry, I misunderstood BasePrimitiveResult status. It's already deprecated #11054 at 0.46 (renamed as a private class _BasePrimitiveResult) and BasePrimitiveResult was removed #11576 at 1.0. We need to decide when we remove _BasePrimitiveResult.

@t-imamichi
Copy link
Member

I have a concern to rename SamplerResult and EstimatorResult because it's a breaking change. I updated the docstrings to clarify that they are for V1 0b66c6a. How about this approach?

github-merge-queue bot pushed a commit that referenced this issue Jul 25, 2024
* Deprecate V1 Primitives and their utils

* Fix tests

* Fix yaml error

* Fix build

* Fix error after mc

* Fix error after mc

* Apply comments

* Use correct deprecate version for warning message

* Update deprecation messages

* Add missed ``

* update releasenote

* Deprecate SamplerResult and EstimatorResult

* fix deprecation warning for SamplerResult and EstimatorResult

* apply review comments

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

* Applying the agreement of deprecations.

- Revert BaseSamplerV1 and BaseEstimatorV1
- Deprecate BaseSampler and BaseEstimator

#12497 (comment)

* revert SamplerResult, EstimatorResult, and BasePrimitiveResult

* fix test_backend_sampler

* revert tox.ini

* revise deprecation warning for BaseSampler and BaseEstimator

* update reno

* revert BaseSampler and BaseEstimator

---------

Co-authored-by: Takashi Imamichi <imamichi@jp.ibm.com>
Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this issue Aug 1, 2024
* Deprecate V1 Primitives and their utils

* Fix tests

* Fix yaml error

* Fix build

* Fix error after mc

* Fix error after mc

* Apply comments

* Use correct deprecate version for warning message

* Update deprecation messages

* Add missed ``

* update releasenote

* Deprecate SamplerResult and EstimatorResult

* fix deprecation warning for SamplerResult and EstimatorResult

* apply review comments

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

* Applying the agreement of deprecations.

- Revert BaseSamplerV1 and BaseEstimatorV1
- Deprecate BaseSampler and BaseEstimator

Qiskit#12497 (comment)

* revert SamplerResult, EstimatorResult, and BasePrimitiveResult

* fix test_backend_sampler

* revert tox.ini

* revise deprecation warning for BaseSampler and BaseEstimator

* update reno

* revert BaseSampler and BaseEstimator

---------

Co-authored-by: Takashi Imamichi <imamichi@jp.ibm.com>
Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants