Migrate the library to Qiskit 2.0 #973
Conversation
Siddharthgolecha
requested review from
woodsp-ibm,
adekusar-drl,
smens,
edoaltamura,
oscar-wallis,
OkuyanBoga and
Benjamin-Symons
as code owners
|
|
|
An overall comment. The V1 primitives usage was deprecated in 0.8 release and marked for removal in 0.9. So in that regards that aspect of this PR fits with the plan as the next main release to happen would be 0.9. However the removal of the Blueprint circuit support - this was deprecated in this PR #945 which was intended to go out in 0.9 with the removal in a subsequent version. Qiskit has deprecated these Blueprint based circuits with the plan to remove them in |
| from qiskit.quantum_info.operators.base_operator import BaseOperator | ||
| from qiskit.transpiler.passmanager import BasePassManager | ||
| from qiskit_aer.primitives.sampler import _circuit_key |
There was a problem hiding this comment.
This (line 26 there above) adds a dependence to qiskit_aer which currently ML does not have. Sure it's supported but it's down to the user if they optionally install/use it or not. This import/use of the function will require it. What was done in qiskit algorithms was just to copy over the former circuit key function https://github.com/qiskit-community/qiskit-algorithms/blob/d84bd401bdcba99b8a1c07a8a9e8859097ec3abf/qiskit_algorithms/utils/circuit_key.py#L45 to the repo and use that instead. (Yes it was discussed to remove usage of this too if that were possible/practical...)
There was a problem hiding this comment.
But would rewriting the logic in every library makes sense? Wouldn't it be a better choice to import from one single source?
There was a problem hiding this comment.
Yes, its better not to have a local copy - but with it being dropped from qiskit I could see it being dropped from qiskit_aer too, and as I said the ML library does not have a dependency on aer so its not installed by default - CI installs it here so it will work here as tests are done with aer, But for an end user its optional whether they choose to want to install/use aer and without it that circuit key import will fail. Ideally code should move away from using it at all since circuits are mutable - but maybe for what its used for here its ok.
There was a problem hiding this comment.
Ok, I'll reimplement this function in the library itself. I think that solves our concerns for now.
| numpy>=2.0 | ||
| scipy>=1.4,<1.16 | ||
| scipy>=1.16 |
There was a problem hiding this comment.
Note there is another PR that was fixing tests to work with 1.16 i.e. #966. Maybe this fixes thing too - I do not know. Either way I made a comment in that PR when it was changed like this to start with #966 (comment)
This cannot stay like this, > 1.4 I think was ok as in Python 3.9/3/10 1.16 does not exist (has a >= 3.11 requirement). If you want to bump the min to say the prior version that still works with 3.9/3.10 that should be fine.
There was a problem hiding this comment.
I see. Thanks for letting me know. I didn't realize that the min py version for 1.16 is 3.11. It makes sense if we don't want to bump the min py version of the library as a whole. Make these many changes in one single lib version would not be a great idea. I'll keep this back to 1.14.
There was a problem hiding this comment.
The 1.16 was also excluded as you can see as there are failures that occur with that which another PR was trying to sort out. 1.4 was the minimum version that ML is known to work with - there is no reason to require a user, that might already have scipy installed to force them to a newer version unless ML really requires that e.g. uses some newly introduced function in which case updating this would be wanted. But not now I think. The only thing that we would like to happen is the < 1.16 is removed to allow even the very latest versions once the issues with it are taken care of by that PR I referenced above.
There was a problem hiding this comment.
Yup, I'll revert back this change 👍
Hi, do you mean that since |
Yes they were deprecated in Qiskit 2.0 and planned to be removed in 3.0 (my bad I typed 0.2 and 0.3 above - I edited the original comment. As ML has not been released with the update that deprecates their use itself we do not want to remove Blueprint circuits from being used at this time - rather in a future version to allow users to change/update any existing code they may have that uses these rather than it just breaking if their support was removed. The next version of ML to be released, i.e. 0.9, would include their deprecation as was added by #945. So far that code only exists here in the repo until such time as a new release is made that includes it in the official version (i.e 0,9). Is that clearer. Removing the V1 primitive support is ok, V2 support was added in the last release (0,8) so both were supported by ML but V1 usage was deprecated - giving time for people to switch their code to V2. So the plan was to remove V1 support for the 0.9 release. So you see the pattern, support both old way, but deprecated, and the new way for a period of time to allow users time to changeover without breaking their code, |
I agree, we don't want to break the user experience for the users. But what I am trying to say is that even if we remove these, it won't break the user experience. Let's take one example, the function |
Summary
Qiskit Machine Learning library currently uses Qiskit >=1.0 and <2.0 making it incompatible with the latest Qiskit Version and changes. I have attempted to migrate the library to use the latest Qiskit version.
Fixes #897 and #934
🤖 ✨ Made with the help of the Qiskit Code Assistant
Details and comments
qiskit.primitives.BaseSamplertoqiskit.primitives.BaseSamplerV2qiskit.primitives.BaseSamplerV1toqiskit.primitives.BaseSamplerV2qiskit.primitives.Samplertoqiskit.primitives.StatevectorSamplerqiskit.primitives.BaseEstimatortoqiskit.primitives.BaseEstimatorV2qiskit.primitives.BaseEstimatorV1toqiskit.primitives.BaseEstimatorV2qiskit.primitives.Estimatortoqiskit.primitives.StatevectorEstimatorqiskit.circuit.library.ZZFeatureMaptoqiskit.circuit.library.zz_feature_mapqiskit.circuit.library.ZFeatureMaptoqiskit.circuit.library.z_feature_mapqiskit.circuit.library.RealAmplitudestoqiskit.circuit.library.real_amplitudesqiskit.circuit.library.EfficientSU2toqiskit.circuit.library.efficient_su2qiskit.circuit.library.PauliFeatureMaptoqiskit.circuit.library.pauli_feature_mapuse_methods=Truein VQC as it is not required anymore.