Updated Estimator interface #51
Conversation
|
|
There was a problem hiding this comment.
Approving from the lens of the desired interface, including:
- tasks (we can discuss name of this elsewhere)
- broadcasted shapes
ObservablesArrayBindingArray- bumping up error bars into the required part of the
TaskResult
The issue of migration path is important, but not in scope of my review.
Co-authored-by: Jim Garrison <jim@garrison.cc>
|
Please, let's close on this in the next 24 hours. |
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: Toshinari Itoko <15028342+itoko@users.noreply.github.com>
Co-authored-by: Toshinari Itoko <15028342+itoko@users.noreply.github.com>
Co-authored-by: Toshinari Itoko <15028342+itoko@users.noreply.github.com>
|
Thanks everyone for the thoughtful discussion about whether to version the interfaces, or whether to migrate the current signature. We have, in the end, decided that the pros of versioning the interface win. |
There was a problem hiding this comment.
With a decision being made to use explicit versioning on the public interface as a migration strategy I'm happy with the proposal now. Thanks for all the hard work sorting through everyone's comments to come up with a concrete and workable plan.
I left 2 small comments inline about some of the migration path. Neither are critical for this RFC, the converter class I think is a good idea and some we can add independently after BaseEstimatorV2 has been defined. The other is more of a question for clarification and idle musings on migration strategies for implementers of Estimators (which is largely out of scope for this document, but useful to discuss while formulating this plan).
|
|
||
| In summary, we propose the following strategy: explicitly version the `Estimator` interface (i.e. add a `BaseEstimatorV2` as described in this proposal) and document both the differences and the end user migration path for consumers of estimators. The user can determine which version of the estimator to use, and existing implementations can be left intact. | ||
|
|
||
| Similar to `Backend`, we will have a `BaseEstimator`, `BaseEstimatorV1` equal to the existing `BaseEstimator`, and `BaseEstimatorV2`, as described in this proposal. |
There was a problem hiding this comment.
I like this direction, thanks for outlining it clearly. One thing we've found with the Backend interface is that the converter class to wrap the old interface in the new semantics of V2 has proven quite useful for end users writing their own libraries on top of backends: https://qiskit.org/documentation/stubs/qiskit.providers.BackendV2Converter.html accounting for something like that as part of this versioning strategy would be useful. This will enable end users to more quickly standardize on using BaseEstimatorV2 and if a particular estimator implementation is still v1 they can just wrap it in the converter class.
| In summary, we propose the following strategy: explicitly version the `Estimator` interface (i.e. add a `BaseEstimatorV2` as described in this proposal) and document both the differences and the end user migration path for consumers of estimators. The user can determine which version of the estimator to use, and existing implementations can be left intact. | ||
|
|
||
| Similar to `Backend`, we will have a `BaseEstimator`, `BaseEstimatorV1` equal to the existing `BaseEstimator`, and `BaseEstimatorV2`, as described in this proposal. | ||
| We will maintain backward compatibility by defaulting the import of `Estimator` to `EstimatorV1` (i.e. existing) for the duration of the deprecation period. |
There was a problem hiding this comment.
Which Estimator are you talking about here? I assume qiskit.primitives.Estimator, but I know of at least three classes named Estimator that implement BaseEstimator in different repositories.
Regardless of which one is being referred to, one other option here that might make it easier for users, is basically what qiskit-ibm-provider does for its BackendV2 implementation. The backends from qiskit-ibm-provider implement the BackendV2 interface but also provide a compatible interface with most of BackendV1 (the notable exception being backend.name which is an attribute on V2 and a method on V1). So users have the freedom to use both interfaces for an existing implementation. It also makes future deprecation a little easier because it's easier to warn on legacy access patterns and inputs. But it's also totally manageable with parallel implementations, just using a FutureWarning when Estimator is instantiated and you can use a __future__ import to change Estimator over to EstimatorV2.
|
I think the only two open discussion are: Let’s move those conversation to their issues. |
No description provided.