Skip to content

providers/backend: define Backend type as union of versioned types#10043

Closed
airwoodix wants to merge 1 commit into
Qiskit:mainfrom
airwoodix:backend-base-as-type-union
Closed

providers/backend: define Backend type as union of versioned types#10043
airwoodix wants to merge 1 commit into
Qiskit:mainfrom
airwoodix:backend-base-as-type-union

Conversation

@airwoodix
Copy link
Copy Markdown
Contributor

@airwoodix airwoodix commented Apr 27, 2023

Summary

This patch is a proposal for redefining the qiskit.providers.Backend type as a union of the versioned types instead of as an empty base class.

The empty base class is unfortunately not very useful in a typed context because having an instance of the Backend type doesn't allow calling any methods / accessing any attributes other than version on it.

With the proposed approach, a pre-processor could be defined:

def as_backend_v2(backend: Backend) -> BackendV2:
    if is_backend_v1(backend):
        return BackendV2Converter(backend)

    return backend

such that functions consuming backends can access backend attributes in a typesafe manner:

def print_backend_name(backend: Backend) -> None:
    backend_ = as_backend_v2(backend)
    print(backend.name)  # should have been backend.name() for BackendV1

Since both variants of the new Backend type have a version attribute, the proposed modification is fully backwards compatible. Manualy narrowing using the version attribute is still supported (see below).

Details and comments

  • the PEP 647 type guards could also be implemented as
def is_backend_v1(obj: Backend) -> TypeGuard[BackendV1]:
    return obj.version == 1

@airwoodix airwoodix requested review from a team and jyu00 as code owners April 27, 2023 09:32
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Apr 27, 2023
@qiskit-bot
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to look at this, it fell through the cracks in my review queue. While I really like this change and think this is a much better interface which uses more modern Python conventions than what I came up with back in #5086 I think the biggest issue here is going to be backwards compatibility. The hard class for better or worse is part of the existing API and things are relying on the current structure (both in qiskit which you fixed most of and also in downstream code that does things like isinstance(obj, Backend) and also obj.version. I expect this was the cause of the CI failures because the tests were catching the API change here.

I'm not sure if there is a path to bridge the two styles. Normally I'd propose deprecating the hard class usage, but that's also going to be tricky to catch to raise a deprecation warning with the way the class is used. Maybe something like defining the union type alias as BackendType or something like so that it can be used in a typing scenario and keep the hard class in place as is?

@airwoodix
Copy link
Copy Markdown
Contributor Author

Thanks for the review and apologies for the delayed feedback. As you point out, the proposed change is much more invasive than I initially anticipated while having less practical relevance than I thought.

Since the main advantages would come up only with much wider typing coverage than qiskit-terra currently has, I'd suggest to close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community PR PRs from contributors that are not 'members' of the Qiskit repo

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants