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

Make V2 primitive input container classes private #11678

Merged
merged 4 commits into from
Jan 31, 2024

Conversation

chriseclectic
Copy link
Member

Summary

Make the V2 primitives input container and pubs classes private and removes them from public API Docs

Details and comments

This removes the EstimatorPub, SamplerPub, BindingsArray, ObservablesArray classes from API docs to make them internal and not subject to qiskits deprecation policies for public classes. This is because we have not had sufficient time to fully finalize these classes before the 1.0 release and anticipate making possibly breaking changes to them in future qiskit releases.

Note that we still intend for the <class>Like type hints to be in the API docs, though im not entirely sure how to get these to build and render in sphinx with propert doc strings and without unrolling all the composite type hints.

@chriseclectic chriseclectic added the mod: primitives Related to the Primitives module label Jan 30, 2024
@chriseclectic chriseclectic added this to the 1.0.0 milestone Jan 30, 2024
@chriseclectic chriseclectic requested review from a team as code owners January 30, 2024 20:03
@qiskit-bot
Copy link
Collaborator

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

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

@mtreinish mtreinish added the Changelog: None Do not include in changelog label Jan 30, 2024
@chriseclectic chriseclectic changed the title Private containers Make V2 primitive input container classes private Jan 30, 2024
@coveralls
Copy link

coveralls commented Jan 30, 2024

Pull Request Test Coverage Report for Build 7731026313

  • -3 of 48 (93.75%) changed or added relevant lines in 8 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.02%) to 89.606%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/primitives/containers/observables_array.py 27 30 90.0%
Files with Coverage Reduction New Missed Lines %
qiskit/primitives/statevector_estimator.py 1 94.55%
crates/qasm2/src/lex.rs 4 91.94%
Totals Coverage Status
Change from base Build 7729918317: 0.02%
Covered Lines: 59357
Relevant Lines: 66242

💛 - Coveralls

@t-imamichi
Copy link
Member

Don't we need to rename classes with prefix "_", e.g., BindingsArray_BindingsArray?

@ikkoham
Copy link
Contributor

ikkoham commented Jan 31, 2024

I don't think the definition of private in Qiskit is clear. Is it private if it is not in __init__.py or if it starts with _? @mtreinish
If the former, I approve.

@mtreinish
Copy link
Member

From qiskit 1.0 moving forward we're being more explicit about what are public stable interface is. We've documented the stable public interface for qiskit as being anything documented: https://docs.quantum.ibm.com/start/install#qiskit-versioning (it's annoyingly under the "release schedule" section and I can't directly link to it). So ensuring that we're not re-exporting it from __init__.py and making sure it's not documented is sufficient to have the the classes treated as private. That being said prepending them with _ makes the classes explicitly private and will likely reduce confusion around them, even if it's not strictly required.

ihincks
ihincks previously approved these changes Jan 31, 2024
Copy link
Contributor

@ihincks ihincks left a comment

Choose a reason for hiding this comment

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

I have a slight preference not to rename them to start with _ because about 4 other repos would need to be changed to accommodate, both now, and when we make them public. Maybe I should be more concerned about user confusion, but since these things are silently created inside of the primitive and not returned, I can't see how it would be common-place.

This removes the EstimatorPub, SamplerPub, BindingsArray, ObservablesArray classes from API docs to make them internal and not subject to qiskits deprecation policies for public classes.
Copy link
Contributor

@ihincks ihincks left a comment

Choose a reason for hiding this comment

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

LGTM, but I would like to brainstorm a bit more improving the visibility of the *Like union types in our api docs.

Copy link
Contributor

@ihincks ihincks left a comment

Choose a reason for hiding this comment

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

Approving because concerns in my previous comment can be their own PR.

@chriseclectic chriseclectic added this pull request to the merge queue Jan 31, 2024
Merged via the queue into Qiskit:main with commit 4f25133 Jan 31, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog mod: primitives Related to the Primitives module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants