Enhance Primitives' interface #8065

ikkoham · 2022-05-16T04:23:55Z

Summary

This PR enhances Primitives (Sampler and Estimator) and provide a more flexible interface.
This comes from the discussion #8035.

The major difference is that the previous PR provided a function to extend the interface, but this one does so by default.

Details and comments

As discussed offline, this PR is API breaking but only for internals.
If needed, I will add a DeprecationWarning, please point it out in the review comments.

TODO:

add the release note.

qiskit-bot · 2022-05-16T04:24:03Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@ajavadia
@ikkoham
@levbishop
@t-imamichi

coveralls · 2022-05-16T04:50:35Z

Pull Request Test Coverage Report for Build 2425813167

100 of 110 (90.91%) changed or added relevant lines in 5 files are covered.
9 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.004%) to 84.395%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/primitives/base_sampler.py	32	36	88.89%
qiskit/primitives/base_estimator.py	50	56	89.29%

Files with Coverage Reduction	New Missed Lines	%
qiskit/primitives/base_estimator.py	1	88.42%
qiskit/primitives/base_sampler.py	1	88.06%
qiskit/primitives/sampler.py	1	92.11%
qiskit/primitives/estimator.py	2	92.31%
qiskit/primitives/utils.py	4	77.78%

Totals
Change from base Build 2409841148:	0.004%
Covered Lines:	54617
Relevant Lines:	64716

💛 - Coveralls

qiskit/primitives/base_sampler.py

HuangJunye · 2022-05-17T11:05:11Z

I think we should have someone (e.g. @rathishcholarajan ) from the runtime provider team to review to get their perspectives.

qiskit/primitives/base_estimator.py

ikkoham · 2022-05-17T15:37:39Z

I would like to summarize the difference between meta-programming and explicitly passing a name/id in this implementation.

Meta-programming
[+] No need to be aware of passing names and IDs on the subclass.
[-] Difficult to understand and maintain

Passing names/ids
[+] Simple
[-] Duplicated codes
[-] Need to understand that the subclass implementer needs to pass names and ids explicitly

ikkoham · 2022-05-18T06:18:02Z

I found that by using __new__ (instead of __init_subclass__) we can stop meta-programming and write code more concisely. I think this made it both easier to implement and maintain.

qiskit/primitives/base_estimator.py

qiskit/primitives/base_sampler.py

test/python/primitives/test_estimator.py

qiskit/primitives/base_sampler.py

test/python/primitives/test_sampler.py

qiskit/primitives/base_sampler.py

qiskit/primitives/base_estimator.py

Co-authored-by: Rathish Cholarajan <rathishc24@gmail.com>

ikkoham · 2022-05-19T15:59:20Z

@rathishcholarajan Thank you for your review comments. Just to be clear, this PR changes the abstract method from __call__ to _call due to type difference. Basically, it should work by simply changing the method name as in the reference implementation. Although this is an API breaking point, is it possible to follow this change?

rathishcholarajan · 2022-05-19T16:57:13Z

@rathishcholarajan Thank you for your review comments. Just to be clear, this PR changes the abstract method from __call__ to _call due to type difference. Basically, it should work by simply changing the method name as in the reference implementation. Although this is an API breaking point, is it possible to follow this change?

Thanks @ikkoham. I can understand it and have made the changes in this PR Qiskit/qiskit-ibm-runtime#331. But have you considered not forcing this validation/conversion in base class __call__ but leaving it upto subclasses to call super().__call__(). That way if someone is just looking at the qiskit-ibm-runtime implementation they know what to pass to __call__.

ikkoham · 2022-05-19T17:24:55Z

@rathishcholarajan Thank you. Your PR looks good.

Yes, the reason why we can't use super().__call__() here is, first, that the argument types of __call__ and _call are different, and also that the return type of __call__ is also different because the return value of super(). __call__ is unknown at the time of BaseClass.
The key point of this new design is that it explicitly simplifies implementation in subclasses by restricting the type of _call.

rathishcholarajan

LGTM! Thanks a lot @ikkoham! (I pulled in this code to provider and all tests are passing Qiskit/qiskit-ibm-runtime#331)

(Approving and hoping it won't auto merge without the label and since I am not a code owner here, thereby others get enough time to review)

blakejohnson

LGTM.

Cryoris

LGTM, but I'll keep the automerge label off in case someone else (like @levbishop or @ajavadia?) want to have another look 🙂

qiskit/primitives/base_estimator.py

HuangJunye

I can't comment on the technicality of the implementation but I left some comments about the documentation. One major point I want to ask if whether we should also rename parameter_values to parameters now that we are renaming circuit_indices and observable_indices to circuits and observables. I am not sure why the current implementation has parameter_values as argument in __call__ method but parameters in the __init__ method. I think it would be good to unify among the methods.

qiskit/primitives/base_estimator.py

qiskit/primitives/base_sampler.py

qiskit/primitives/base_estimator.py

releasenotes/notes/primitive-interface-408b91ed338a5bc4.yaml

Co-authored-by: Junye Huang <h.jun.ye@gmail.com> Co-authored-by: Rathish Cholarajan <rathishc24@gmail.com>

qiskit/primitives/base_estimator.py

Co-authored-by: Rathish Cholarajan <rathishc24@gmail.com>

HuangJunye

Looks great! Thanks for adopting the suggestions.

… observables (Qiskit#165) * Adapt to base class changes Qiskit/qiskit#8065 * Adapt sampler to base class changes in terra * Fix black * Fix lint * Fix parameter names * Use Ikko's branch from terra temporarily for testing base class changes * Fix some failing tests * Fix couple more tests * Fix all test failures * Install rust * Install rust compiler * Fix disabling prompt when installing rust * Run cargo * Fix parameter name * Fix black * Update requirements.txt * Apply suggestions from code review Co-authored-by: IKKO HAMAMURA <Ikko.Hamamura1@ibm.com> * Match _call signature with base classes and remove unnecessary casts * Try with terra pre pre-release * Double equals needed * Fix lint * qiskit-terra 0.21.0 released, so use that no longer need to install rust since we are not installing terra from source anymore qiskit-aer is really a dev dependency for this repo and use the main branch code for now since PR Qiskit/qiskit-aer#1521 is not released yet * Stop inheriting QiskitTestCase so tests do not fail on deprecation warnings Co-authored-by: IKKO HAMAMURA <Ikko.Hamamura1@ibm.com>

Enhance Primitives' interface

4999259

ikkoham added Changelog: New Feature Include in the "Added" section of the changelog mod: primitives Related to the Primitives module labels May 16, 2022

ikkoham requested review from blakejohnson, HuangJunye, ajavadia and levbishop May 16, 2022 04:23

ikkoham added 2 commits May 16, 2022 14:09

use QuantumCircuit.name instead of id

be5c08d

fix lint

3e5334a

Cryoris reviewed May 16, 2022

View reviewed changes

qiskit/primitives/base_sampler.py Outdated Show resolved Hide resolved

rename *_indices

cd2126d

blakejohnson reviewed May 17, 2022

View reviewed changes

qiskit/primitives/base_estimator.py Outdated Show resolved Hide resolved

ikkoham added 2 commits May 18, 2022 11:12

use __new__ instead of __init_subclass__

7d1261d

refactoring

8b5f62a

ikkoham marked this pull request as ready for review May 18, 2022 06:18

ikkoham requested review from a team and t-imamichi as code owners May 18, 2022 06:18

rathishcholarajan suggested changes May 18, 2022

View reviewed changes

rathishcholarajan mentioned this pull request May 19, 2022

Deprecate circuit_indices and observable_indices and add circuits and observables Qiskit/qiskit-ibm-runtime#331

Merged

1 task

rathishcholarajan reviewed May 19, 2022

View reviewed changes

qiskit/primitives/base_estimator.py Show resolved Hide resolved

ikkoham and others added 2 commits May 19, 2022 23:12

Apply suggestions from code review

2c837be

Co-authored-by: Rathish Cholarajan <rathishc24@gmail.com>

Reflect review comments

6f9f469

rathishcholarajan previously approved these changes May 26, 2022

View reviewed changes

Merge branch 'main' into primitives/enhance-interface

94d5ab4

blakejohnson previously approved these changes May 27, 2022

View reviewed changes

ikkoham added this to the 0.21 milestone May 28, 2022

Cryoris previously approved these changes May 30, 2022

View reviewed changes

rathishcholarajan reviewed Jun 1, 2022

View reviewed changes

qiskit/primitives/base_estimator.py Outdated Show resolved Hide resolved

rathishcholarajan reviewed Jun 1, 2022

View reviewed changes

qiskit/primitives/base_estimator.py Outdated Show resolved Hide resolved

HuangJunye reviewed Jun 1, 2022

View reviewed changes

Apply suggestions from code review

bb0918d

Co-authored-by: Junye Huang <h.jun.ye@gmail.com> Co-authored-by: Rathish Cholarajan <rathishc24@gmail.com>

ikkoham dismissed stale reviews from Cryoris, blakejohnson, and rathishcholarajan via bb0918d June 1, 2022 14:36

rathishcholarajan reviewed Jun 1, 2022

View reviewed changes

qiskit/primitives/base_estimator.py Show resolved Hide resolved

rathishcholarajan reviewed Jun 1, 2022

View reviewed changes

qiskit/primitives/base_estimator.py Show resolved Hide resolved

ikkoham and others added 5 commits June 2, 2022 09:51

Merge branch 'main' into primitives/enhance-interface

e0186e3

Update qiskit/primitives/base_estimator.py

e1260ef

Co-authored-by: Rathish Cholarajan <rathishc24@gmail.com>

Update qiskit/primitives/base_estimator.py

8300785

Co-authored-by: Rathish Cholarajan <rathishc24@gmail.com>

releasenote

0dc5fd9

rm dup and add comments

fc499f0

ikkoham requested review from HuangJunye, blakejohnson and Cryoris June 2, 2022 03:47

HuangJunye approved these changes Jun 2, 2022

View reviewed changes

Cryoris approved these changes Jun 2, 2022

View reviewed changes

Cryoris merged commit df54ae2 into Qiskit:main Jun 2, 2022

rathishcholarajan mentioned this pull request Jun 3, 2022

check circuit_indices vs length of circuits prior to running Qiskit/qiskit-ibm-runtime#313

Closed

ikkoham deleted the primitives/enhance-interface branch June 6, 2022 00:31

t-imamichi mentioned this pull request Jul 5, 2022

Need to fix the inconsistent part between the base classes and the reference implementations of Primitives #7836

Closed

ikkoham mentioned this pull request Sep 1, 2022

Deprecate circuit_indices and observable_indices and replace with circuits and observables #8045

Closed

Enhance Primitives' interface #8065

Enhance Primitives' interface #8065

Uh oh!

Conversation

ikkoham commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

qiskit-bot commented May 16, 2022

Uh oh!

coveralls commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2425813167

💛 - Coveralls

Uh oh!

Uh oh!

HuangJunye commented May 17, 2022

Uh oh!

Uh oh!

ikkoham commented May 17, 2022

Uh oh!

ikkoham commented May 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ikkoham commented May 19, 2022

Uh oh!

rathishcholarajan commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikkoham commented May 19, 2022

Uh oh!

rathishcholarajan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blakejohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Cryoris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

HuangJunye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HuangJunye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ikkoham commented May 16, 2022 •

edited

Loading

coveralls commented May 16, 2022 •

edited

Loading

rathishcholarajan commented May 19, 2022 •

edited

Loading

rathishcholarajan left a comment •

edited

Loading