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

EstimatorV2 updates #1321

Merged
merged 26 commits into from
Feb 8, 2024
Merged

Conversation

kt474
Copy link
Member

@kt474 kt474 commented Jan 17, 2024

Summary

There are a lot of files copied over from qiskit, they should be replaced after the next qiskit release.

Simple job example:

Screenshot 2024-01-22 at 12 12 05 PM

Details and comments

Fixes #1314

@coveralls
Copy link

coveralls commented Jan 18, 2024

Pull Request Test Coverage Report for Build 7835592117

  • -9 of 59 (84.75%) changed or added relevant lines in 15 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.5%) to 79.982%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_ibm_runtime/utils/estimator_result_decoder.py 1 3 33.33%
qiskit_ibm_runtime/utils/json.py 15 18 83.33%
qiskit_ibm_runtime/sampler.py 5 9 55.56%
Files with Coverage Reduction New Missed Lines %
qiskit_ibm_runtime/utils/estimator_result_decoder.py 2 50.0%
qiskit_ibm_runtime/utils/json.py 3 72.2%
Totals Coverage Status
Change from base Build 7833396730: 0.5%
Covered Lines: 5262
Relevant Lines: 6579

💛 - Coveralls

@ihincks
Copy link
Collaborator

ihincks commented Jan 19, 2024

BaseEstimatorV2 (and all supportive container classes) has now been merged into qiskit's main branch

@kt474 kt474 marked this pull request as ready for review January 22, 2024 17:19
@kt474 kt474 changed the title WIP EstimatorV2 EstimatorV2 updates Jan 22, 2024
@kt474
Copy link
Member Author

kt474 commented Jan 22, 2024

@jyu00 @gadial
Should be ready for initial review, I left a couple questions in about things I wasn't sure about

Copy link
Collaborator

@gadial gadial left a comment

Choose a reason for hiding this comment

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

Generally LGTM. I believe we should have more tests, and my main concern is the large amount of copy-pasting of container data from qiskit, even if we believe it's not subject to change. However, if we have no way around it I have no objections to merging this PR.

@@ -254,8 +253,14 @@ def default(self, obj: Any) -> Any: # pylint: disable=arguments-differ
serializer=lambda buff, data: dump(data, buff), # type: ignore[no-untyped-call]
)
return {"__type__": "Instruction", "__value__": value}
if isinstance(obj, BasePub):
return asdict(obj)
# TODO proper way to do this?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we treat this similarly to other complex data types, e.g. pass a the following dict:

        if isinstance(obj, EstimatorPub):
            return {
                "__type__": "EstimatorPub",
                "__value__": {
                    "circuit": obj.circuit,
                    "observables": obj.observables,
                    "parameter_values": obj.parameter_values,
                    "precision": obj.precision,
                },
            }

However, I think we should delay that until we start using the EstimatorPub from qiskit itself.

from ..ibm_test_case import IBMIntegrationTestCase


class TestEstimatorV2(IBMIntegrationTestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test suite is currently relatively small; perhaps we can use the test suite in qiskit:

https://github.com/Qiskit/qiskit/blob/main/test/python/primitives/test_estimator.py

See the SamplerV2 PR where I do something similar for Sampler:

https://github.com/gadial/qiskit-ibm-runtime/blob/8102831d23fba61fbe294770accbe093f32929a6/test/integration/test_sampler_v2.py

@jyu00
Copy link
Collaborator

jyu00 commented Jan 23, 2024

Now that BaseEstimatorV2 and container classes are all merged into Qiskit main, I think it'd make more sense to just include qiskit main (then rc1 when it's released) in this branch

@kt474
Copy link
Member Author

kt474 commented Jan 23, 2024

Now that BaseEstimatorV2 and container classes are all merged into Qiskit main, I think it'd make more sense to just include qiskit main (then rc1 when it's released) in this branch

Removed all of the copied files and using the main branch of qiskit now. I believe the build is failing because the CI is not using the main branch and the qiskit version is being overwritten by another dependency.

@gadial
Copy link
Collaborator

gadial commented Jan 25, 2024

Now that BaseEstimatorV2 and container classes are all merged into Qiskit main, I think it'd make more sense to just include qiskit main (then rc1 when it's released) in this branch

Removed all of the copied files and using the main branch of qiskit now. I believe the build is failing because the CI is not using the main branch and the qiskit version is being overwritten by another dependency.

This looks like mthree dependency, but I don't see why mthree is included in requirements-dev.txt at all.

@kt474
Copy link
Member Author

kt474 commented Jan 25, 2024

Lint is now failing because qiskit.test was removed from qiskit 1.0 (Have a separate PR to take care of that #1292, too many moving parts 🫠)

Ran unit tests locally - tests are passing except test_parameters_vals_kwvals (which I think is from a bug in BindingsArray.coerce() - fixed Qiskit/qiskit#11645

@jyu00
Copy link
Collaborator

jyu00 commented Jan 29, 2024

This can be a separate PR, but v2 primitives run() method now returns an instance of BasePrimitiveJob, which does not inherit from JobV1. RuntimeJob needs to be updated to inherit this (plus JobV1 to support backend.run still, if the same class is used).

@jyu00
Copy link
Collaborator

jyu00 commented Feb 1, 2024

@kt474 Now that the 2 blockers have been merged, can you update this PR so we can get it merged? We'll need more tests, but that can go into a separate PR.

@jyu00 jyu00 merged commit 8b5ce2a into Qiskit:experimental-0.2 Feb 8, 2024
17 checks passed
@kt474 kt474 deleted the estimatorv2-updates branch November 25, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants