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

Add RFC for v2 providers interface and prepare for new services APIs #2

Closed
wants to merge 12 commits into from

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Jan 22, 2020

This commit adds an rfc for implementing a new providers interface. The
goal of this rfc is to document how we evolve the interface between
qiskit and providers moving forward to be something that is faster and
easier to work with. This new interface also lays the groundwork for
implementing new types of services providers that will enable different
type of external services.

TODO:

  • Add v2 api scaffold for results
  • Add v2 api scaffold for jobs
  • Add v2 api scaffold for backend properties

This commit adds an rfc for implementing a new providers interface. The
goal of this rfc is to document how we evolve the interface between
qiskit and providers moving forward to be something that is faster and
easier to work with.
@nonhermitian
Copy link

The interface for the configurable backends is meant to be the same as that of the transpiler passmanagers (for service consistency), namely:

backend = provider.get_backend('ibmq_blah')
backend(backend_config)
job = backend.run([circuits])

@kdk
Copy link
Member

kdk commented Jan 27, 2020

With terra native objects as the base data structures for the V2 API, how do providers handle upgrades to terra which may change those structures? (Do terra objects define and expose a versioned interface, or do providers bind to a specific minor version of terra, or something else?)

@mtreinish
Copy link
Member Author

With terra native objects as the base data structures for the V2 API, how do providers handle upgrades to terra which may change those structures? (Do terra objects define and expose a versioned interface, or do providers bind to a specific minor version of terra, or something else?)

My thinking on this is that we need to provide stability guarantees on APIs of terra's structures (ie a method to get a list of instructions from a QuantumCircuit object). We mostly are good about doing this already since we adopted the deprecation policy: https://qiskit.org/documentation/contributing_to_qiskit.html#deprecation-policy Provider implementers will then use minimum versions, etc in requirements to adapt to any changes we make to the terra api. Since we'll give them at least a 3 month runway to adapt to changes.

That being said this is actually worse in today's v1 model because right now the interface is defined but not stable as it's hard coupled to the API specification, and changes to that change the interface for providers. But api changes happen more opaquely and without warning.

@mtreinish
Copy link
Member Author

I've added more details about the v2 interface for backends, configuration, job, and result. But, I'm still probably missing something here

Copy link
Collaborator

@taalexander taalexander left a comment

Choose a reason for hiding this comment

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

Looks great, looking forward to this!

text/###-v2-provider-interface.md Outdated Show resolved Hide resolved
text/###-v2-provider-interface.md Outdated Show resolved Hide resolved
text/###-v2-provider-interface.md Outdated Show resolved Hide resolved
text/###-v2-provider-interface.md Outdated Show resolved Hide resolved
text/###-v2-provider-interface.md Outdated Show resolved Hide resolved
configuration.update_config(**fields)

@property
def configuration(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps type hints would make this clearer to understand input/output.

pass
```

For the job/results interface one thing to note is that there is now a Counts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent

convert the circuit objects to whichever format is needed for interfacing the
circuit to their backends/services.

We'll need to keep `assemble()` (and the dissasembler) in terra for backwards
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

def wait_for_final_state():
raise NotImplementedError

def get_memory(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Per our discussion, a "job" can be used to transport other types of requests, such as transpilation or visualization. So the get_foo() methods don't apply to all cases and shouldn't be in BaseJob.

def status():

def cancel():
raise NotImplementedError
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of raising NotImplementedError here? If it's an optional method, why not just leave it out of BaseJob (so it doesn't show up in autocomplete/docs)? If it's required, isn't @abstractmethod a better choice?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to make it optional. I left it it because we want this interface to be standardized between providers that implement this feature. We don't want to leave the interface freeform but don't want to force every provider to implement it. Were you thinking of defining this somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

My main concern is that it puts the burden on the users to know whether their job instance supports cancel() (or to except NotImplementedError), especially since the cancel() method will show up in autocomplete/docs.

Co-Authored-By: Thomas Alexander <thomasalexander2718@gmail.com>
A new Terra issue Qiskit/qiskit#4105 was opened that has partial
overlap with the work being done here. It had some good ideas that
would make the v2 interface a better fit for a world where there are
backends that do more than just run circuits (or pulse schedules). This
commit starts the process of integrating the ideas from that into the
larger refactor pending for the provider interface.
@mtreinish mtreinish changed the title [WIP] Add RFC for v2 providers interface Add RFC for v2 providers interface and prepare for new services APIs Apr 15, 2020
The job/result section was poorly worded and structured, leading to some
confusion about the intention to combine the 2 classes. This commit
updates the sdection to try and make it better written.
@atilag
Copy link
Member

atilag commented Apr 27, 2020

Moving along the idea of shifting wire protocol responsibilities to the providers is totally fine with me. We have been suffering from JSON related problems in Aer for a long time and the new non-json approach has had a huge performance boost.

About Sync/Async
I'm not a big fan of hybrid interfaces, basically because uniform interfaces are way more friendly so enhances user experience, but in the real world it's all about trade-offs because we have constraints, and one of the biggest is backward compatibility so I'd love to explore every possible alternative before taking the decision of moving towards a hybrid interface.

Before continuing, I must say that I'm ok with the async interface we have right now.
Hybrid interfaces were more common few years ago, when I/O concurrency became a thing, but I'd say (and this is very subjective) that modern interface design has deprecated this hybrid approach and leaned toward having a unique async-future based interface, because we can always block our future-like object to become sync.
In our case, we have this very same async-future interface, so I see the "extra" step we have to take for local simulations as making a trade-off between having a unique and uniform interface or a hybrid interface, and we favor the former.

Result
What I don't really like is that our Result class violates Liskov principle, so it's overloaded of methods that are useless in many use cases, and from what I see about your proposal, this doesn't change... or is it just part of the first phase?

    def get_counts(self):
        return self.result().get_counts()
    def get_statevector(self):
        return self.result().get_statevector()
    def get_unitary(self):
        return self.result().get_statevector()

If we have specific Result classes we are achieving two major things:

  1. Move results responsibility to the backend.
  2. Clearly states what kind of results we are expecting, so users don't make this very common mistake again:
result.get_statevector() # <--- boom! there's no statevector in this simulation! only counts.

By "specific result classes" I mean: CountsResult, StatevectorResult, UnitaryResult or MyCrazyBackendResult, Terra should provide with an abstract class and a bare minimum common interface though, like we already do with BaseBackend.

Example of syncish code:

qasm_sim = QasmSimulator()
counts_result = qasm_sim.run(circuit).result()
print(f"Counts: {}", counts_result)
...
sv_result = statevector.run(circuit).result()
print(f"Statevector: {}", sv_result)

Example of async code:

sim = UnitarySimulator()
job = qasm_sim.run(circuit)
print(f"Counts: {}", job.result().get_unitary())

Thoughts?

@jyu00
Copy link
Contributor

jyu00 commented May 1, 2020

Can we also describe how large experiments would be handled? For example, if an experiment exceeds the maximum number of circuits/gates allowed by the backend, aqua today splits that into multiple jobs and merges the results together. ibmq-provider also has a similar function called Job Manager.

With v2 we can take the opportunity to say backend.run() will return a "job" that may contain multiple traditional jobs. This simplifies the user interface as people can use the same backend.run(), job.get_counts(), etc for large and small experiments. The disadvantage is that operations on individual traditional jobs become more complex, but in theory people shouldn't need to drill into the individual ones very often.

mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 21, 2020
This commit adds the initial implementation of the v2 providers
interface. The rfc document in Qiskit/RFCs#2 documents the rationale
beind the PR. At a high level this introduces a python based API that
provides a general interface for writing additional providers to terra
that is flexible enough for both additional types of services in the
future and also making it less specific to the IBM Quantum API.
Moving forward we will encourage existing provider to migrate to this
new interface.

Included in this commit the Basicaer provider has been converted to a
v2 provider. This will provide a good example for providers on how to
use the new model.

This is still very much in progress, prior to removing the WIP this will
be split into 3 PRs. The first will add the Counts class, the second
will add the v2 providers interface, and the 3rd will migrate the
BasicAer provider to use the v2 interface all 3 are combined for now for
ease ease of testing and development (with github's limitations around
dependencies between PRs), but each is an independent piece that can
stand on it's own (and will make it much easier for final review).

Implements Qiskit/RFCs#2
Fixes Qiskit#4105
mtreinish and others added 4 commits June 12, 2020 11:22
Between the PoC/WIP PR Qiskit/qiskit#4485
and recent review comments there has been a lot of feedback made on
the earlier versions of this RFC. This commit updates the RFC to take
much of that into account and also provide a clearer definition on some
aspects of the proposal.
@jyu00
Copy link
Contributor

jyu00 commented Jun 16, 2020

Another thing I'd like to discuss is the concept of a "job". Currently a job is what we use to wrap around the circuit/pulse sent to a backend for execution, and job results are the results of said execution. Soon IBMQ will have more services in addition to just "backend". During a discussion earlier this year @nonhermitian mentioned to continue using "job" as the wrapper for the inputs of these services. If that's the case then we'll need an even more generic Job class (e.g. doesn't require a backend) and maybe BackendJob as a replacement for today's BaseJob.

@jwoehr
Copy link

jwoehr commented Jun 16, 2020

Soon IBMQ will have more services in addition to just "backend". During a discussion earlier this year @nonhermitian mentioned to continue using "job" as the wrapper for the inputs of these services. If that's the case then we'll need an even more generic Job class (e.g. doesn't require a backend) and maybe BackendJob as a replacement for today's BaseJob.

Instead of changing the semantics of existing code, there could be a Service class, possibly a complete superset of today's BaseJob.

@mtreinish
Copy link
Member Author

This has been implemented in Qiskit/qiskit#5086 and Qiskit/qiskit#5885 so I'm just going to close this.

@mtreinish mtreinish closed this Feb 21, 2022
@mtreinish mtreinish deleted the provider-interface branch February 21, 2022 16:27
gadial pushed a commit to gadial/rfcs that referenced this pull request Dec 7, 2022
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.

7 participants