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

Qiskit Services API #4105

Closed
jaygambetta opened this issue Apr 8, 2020 · 15 comments
Closed

Qiskit Services API #4105

jaygambetta opened this issue Apr 8, 2020 · 15 comments
Assignees
Labels
type: enhancement It's working, but needs polishing

Comments

@jaygambetta
Copy link
Member

jaygambetta commented Apr 8, 2020

With the introduction of more IQX services, such as transpiler-as-a-service, we are moving towards a model where there are local and remote executions of the same task (e.g. simulation, transpilation), and additionally remote executions can be low- or high- latency (e.g. simulator vs. device run).

In order to make it easy for users to switch between different types of executions, we would like to create a uniform API between local vs. remote executions, and additionally accomodate both low- and high-latency executions in a natural way.

The proposal is as follows:

  1. Introduce two methods for explicit sync and async runs, and allow the user to choose one that suits their specific run. run() waits for results and returns the result. run_async() returns a job handle that can be later queried for result. One would use run() for local simulation, small remote simulations, and typical transpilation. run_async() can be used for device runs or large remote transpilation jobs.

  2. Remove Result as wrapper of a job result, which has to be queried again for the things you care about. Instead, directly return the thing that the run was intended for, be it counts, memory, unitary, statevector, or a circuit.

So a generic service API will look like:

from qiskit.providers.ibmq import IBMQProvider
provider = IBMQProvider(hub, group, project)
service = provider.service.service_instance
service.setup(service_config)

# run option 1
service_job = service.run_async(service_input)
service_output = service_job.result()

# run option 2
service_output = service.run(service_input)

# run option 3 (for future)
service_stream = service.run_stream(service_input)

Concrete examples:

# local simulation
counts = qasm_sim.run(circuit)
statevector = statevector_sim.run(circuit)
unitary = unitary_sim.run(circuit)

# remote device
backend.setup(run_config)
job = backend.run_async(circuit)  # JobResult
counts = job.result()

# remote device but want per-shot readouts
run_config.memory=True
backend.setup(run_config)
job = backend.run_async(circuit)  # JobResult
memory = job.result()

# local passmanager
from qiskit.transpiler.preset_pass_managers import local_pass_manager
pm = local_pass_manager(passmanager_config)
compiled_circuit = local_pass_manager.run(circuit)

# remote passmanager
pm = remote_pass_manager()
pm.setup(passmanager_config)  # sets up remote service
compiled_circuit = remote_pass_manager.run(circuit)
# or if there are many large circuits
job = remote_pass_manager.run_async(circuit)  # PassManagerJob
compiled_circuit = job.result()

Questions:

Backwards compatibility

We need to keep a deprecation period for the old style to still work. This could be like:

job = backend.run(service_input=qobj)  # +warning, instruct to use job = backend.run_async(service_input=circuit) or result = backend.run(service_input=circuit)
depending on operational.

execute is edited to default to use run_async() so that it works in the same way but add a message that it will be depreicated.

execute(circuit, backend) # deprecate warining and when backend.run(circuit) exists. will default to .run_async() during the deprecation period

"Setup"

Can we do the setup in a Pythonic way for remote services as well? i.e. instead of

pm = remote_pass_manager()
pm.setup(passmanager_config)  # sets up remote service

doing

pm = remote_pass_manager(passmanager_config)

and have the config persist across multiple remote runs.

This would be be my preference as this allows the user to see what the service provider can set up

Configurations

In the above, the setup is done with a configuration, currently in the form of PassManagerConfig or RunConfig objects. Should we remove these classes and just have pm.setup() and backend.setup() accept these kwargs directly?

class PassManagerConfig:
    """Pass Manager Configuration.
    """

    def __init__(self,
                 initial_layout=None,
                 basis_gates=None,
                 coupling_map=None,
                 layout_method=None,
                 routing_method=None,
                 backend_properties=None,
                 seed_transpiler=None):

class RunConfig:
    """Run Configuration.
    """

    def __init__(self,
                 shots=None,
                 memory=None,
                 parameter_binds=None,
                 max_credits=None,
                 seed_simulator=None):

objects removed:

  1. Qobj -> moves into the provider and is the role of the provider to handle serialization
  2. Results class is removed and it is the role of the provider to return the service object (circuit, counts, etc)
  3. Path to removing execuite and encouraging the use the service
backend.run(circuits)
  1. Speed up the local simulator but doing less work to make it look like a remote service
  2. Also removes the assembler
@mtreinish
Copy link
Member

This proposal has a lot of overlap with what we've been working on in Qiskit/RFCs#2 which is the current working proposal (I've been meaning to update it for a couple of weeks now) for reworking the interface between terra and providers. I think we should move the discussion there so we're all on the same page for the new interface.

@jaygambetta
Copy link
Member Author

I'm ok but can you edit your rfc and then let me know as it does not go far enough yet (as well as name). I want the terra interface to be more like this for the extra services being made.

@atilag
Copy link
Member

atilag commented Apr 12, 2020

There are many proposals here, but I'm going to focus only on two of them: async/sync and Result.

Async / Sync

Async interfaces are becoming an standard in almost every interface design which has
some significant computational tasks. I'd say that this is the majority of our use
cases, but it's true that there are experiments run locally that can be pretty fast
and a simpler design is desirable.

By introducing the differentiation between async and sync in the interface,
we are moving towards an hybrid design (async/sync), thus leaving behind the interface
uniformity we have now, and that is something that I'd not feel comfortable with.

I think the problem you brought to the table, is more related to having the
wrong abstractions over our async interface, let me explain:
Any given current example:

qasm_sim = QasmSimulator()
job = qasm_sim.run(circuit)
result = job.result()
print(f"Counts: {}", result.get_counts())

There are 3 abstractions here (we don't care about the circuit abstraction here):

  1. Backend abstraction
qasm_sim = QasmSimulator()
  1. Computational abstraction
job = qasm_sim.run(circuit)
  1. Results abstraction
result = job.result()

And we have these 3 abstractions mainly because the cloud computing has been a major
driver in the design of a big part of Qiskit, due to the time it takes for an
experiment to run in our machines, so the concept of job has had a principal role.

In the other hand, we have local simulations where the concept of job may fade away
in importance because we have almost instant results, so why having a job in the
first place? it will only introduces an extra useless step, right?
Well, maybe we have the wrong abstractions.

What if we merge the concepts of job and result?
Example:

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

result is the final result of the computation.
It can act as a future/job for cloud based experiments, so we don't block program
main threads, we can query for the status of the computation as we do with
job, and there's no extra step for local simulations, the results are in
there and we could, for example, override __str__ or __repr__ to get the desired
output (or/and export an interface). We are moving the responsibility of implementing the
result object to the providers
, offering them just a simple base interface to inherit from.

This new result object will have to expose some of the interface we have in job
as well, and to be honest, this is the part I like the less because breaking Single
Responsibility Principle makes me nervous, so I guess it all depends on how much
complexity we are adding to it.

About the result

Yes, I don't like our current Result class either, and now that I talked about breaking
principles, this is a flagrant Liskov violation. It exports many methods that only make sense for specific types of simulations. Here my proposal is a common practice in fixing
Liskov's: specialize Result per output type, so we have CountsResult, StatevectorResult,
UnitaryResult, etc... so the backend decides which type of result returns.

@nonhermitian
Copy link
Contributor

Actually, the Result class is fairly pointless. The use case is always job.result(). So the result just needs to be part of the job, not the other way around.

@jaygambetta
Copy link
Member Author

I am ok with the results (or job) but it needs to be a class per service and sub-service (simulator vs real backend)

mtreinish added a commit to mtreinish/rfcs that referenced this issue Apr 15, 2020
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.
@atilag
Copy link
Member

atilag commented Apr 15, 2020

Actually, the Result class is fairly pointless. The use case is always job.result(). So the result just needs to be part of the job, not the other way around.

The way I see this, Result is a transversal concept to all the use-cases: we always want a result from an execution. job however is a consequence of the async model we choose (there are others that know nothing about jobs), only applicable to the case of cloud computation as local doesn't really need it.
That's why I'd move job into Result, so code like this is more expressive:

qasm_sim = QasmSimulator()
result = qasm_sim.run(circuit) # result type being: CountsResult
print(f"Counts: {}", result)

Otherwise we have the current model: querying job to get a result, right?

@jaygambetta
Copy link
Member Author

I prefer results over the job as well. We can have a method result.job_id() or results.job_status to give the feeling that there are jobs running. But I do like that we are all agreeing that there is not the need for two classes.

@jaygambetta
Copy link
Member Author

@atilag also good if you edit my original with your proposal.

@atilag
Copy link
Member

atilag commented Apr 15, 2020

Will do.
This is a major change in the API, so I'd love to have more consensus with the rest of the team.
Summoning @kdk @ajavadia @1ucian0 @mtreinish to get some more feedback.

@nonhermitian
Copy link
Contributor

Again, the job has everything we need already built in. One can supplement the job with things like having the call statement give the results, but moving in the direction that every is a result is not a great idea. Especially given that the cloud usage is the long term direction over local execution. I honest thought this was decided months ago by @jaygambetta @ajavadia and myself.

@jaygambetta
Copy link
Member Author

I think we are saying the same thing call job or results somehting_cool we want something_cool to come back from service.run() or service.run_async() I don't have strong view on the name but it should not be two objects. From looking at the code I would prefer results but I'm happy with job.

@mtreinish mtreinish self-assigned this Apr 21, 2020
@mtreinish
Copy link
Member

I started a WIP on this last week: mtreinish@dd25e46. My biggest concerns here are around the backwards compatibility of the user facing api mainly making run() sync instead of async and changing the return type. The issue is that returning something other than a Job from run() will basically break anyone who has written code for qiskit that runs a circuit. I tried to workaround it by adding result() methods to all the possible result types for backwards compat (so the normal pattern you'll see of qobj.run.result()) but that wasn't enough in all places.

I think the way to resolve this is to make run() either async or sync and have it return a job object (which is actually how it works today). I agree for things like BasicAer wrapping the execution in concurrent futures is an unnecessary overhead and adds needless complexity (it's been a big headache for me the last couple of weeks in the marshmallow removal PRs), but there is nothing actually requiring it's use, run can just return a completed Job/Result object without any async execution. We can also add an explicit optional run_sync and run_async method in addition to that for backends to implement if they want.

@nonhermitian
Copy link
Contributor

I do not see why we need change anything about a job being returned. We just need to modify a few things about the job. There are use cases where one actually does want to run simulators async (noise modelling on large or many circuits.) We no longer need BasicAer with the quantum_info module features, so that should not be holding us back, i.e. we need not worry about jobs there.

@jaygambetta
Copy link
Member Author

I agree with Paul we should remove BasicAer -- But paul this needs to be its own issue and also implications to show how to switch to quant-info. Lets not mix this with this issue -- happy to start a new one.

The run is my big question for me. Talking with @chriseclectic he suggested we could do run defaults to async and introduce for blocked run

backend.run().block_until_ready()

The object returned I don't care what it is called but I like this code

counts =backend.run([circuit]) and if it is not finished it is empty and if finished it has the counts in it I can plot by simply going

plot_histogram(counts)

if counts is official a job object with a counts.id etc im good.

mtreinish added a commit to mtreinish/qiskit-core that referenced this issue 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
@ajavadia ajavadia added this to the 0.15 milestone Jun 18, 2020
@kdk kdk removed this from the 0.15 milestone Jun 30, 2020
@mtreinish
Copy link
Member

This has been mostly done now that we've moved to a new versioned providers interface. The BackendV1 and JobV1 abstract classes took a lot of ideas from this (like differentiating between sync and async) and most providers have migrated to use these new classes. We'll be continuing to evolve this interface over time (which is why it's versioned now) the next iteration is being planned and discussed in (#5894 and draft PRs #5885 and #5629). I'm going to close this and we can have further discussions on those newer issues about the next iteration of the interface. But, please feel free to reopen this if there is more to discuss here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement It's working, but needs polishing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants