Add async support in EngineClient, EngineSampler, etc. #5219
Conversation
maffoo
force-pushed
the
u/maffoo/engine-async
branch
from
eaf0415 to
54b15b4
Compare
maffoo
force-pushed
the
u/maffoo/engine-async
branch
6 times, most recently
from
9a297de to
4eb5fc6
Compare
|
Tests are now passing for python 3.8+ but failing for 3.6 and 3.7 because they don't support |
|
cc @verult |
maffoo
force-pushed
the
u/maffoo/engine-async
branch
2 times, most recently
from
6541b86 to
0e515c4
Compare
maffoo
force-pushed
the
u/maffoo/engine-async
branch
from
0e515c4 to
9c56246
Compare
maffoo
force-pushed
the
u/maffoo/engine-async
branch
from
0ef9d84 to
62babd3
Compare
|
@verult, this is working now that we've dropped support for py3.6. PTAL. |
maffoo
requested review from
wcourtney,
a team,
vtomole,
cduck and
verult
as code owners
There was a problem hiding this comment.
A lot of these changes are "rote" (changing method name to _async, adding an await, adding a duet.sync(...) version). Would it be possible to split the "real" changes (like the executor and make_async_request stuff) into a PR and then the mechanical changes to follow? Could make reviewing easier.
The rote changes LGTM where they exist. I've put some comments about where they should exist; cc #5505 . In particular, I'd like to see some async methods show up in the Abstractxxx base classes so we can continue to rely on methods having local, simulator-backed implementations.
The executor stuff could use a little more documentation so future non-@maffoo devs can have some chance of developing with confidence.
| @@ -49,26 +51,28 @@ def __init__( | |||
| self._processor_ids = [processor_id] if isinstance(processor_id, str) else processor_id | |||
| self._engine = engine | |||
|
|
|||
| def run_sweep( | |||
| async def run_sweep_async( | |||
There was a problem hiding this comment.
Reverted changes to engine_sampler.py
| @@ -38,6 +53,26 @@ def __init__(self, message): | |||
| RETRYABLE_ERROR_CODES = [500, 503] | |||
|
|
|||
|
|
|||
| class AsyncioExecutor: | |||
There was a problem hiding this comment.
docstring for this class?
| @@ -208,7 +209,7 @@ def __str__(self) -> str: | |||
| return f'Engine(project_id={self.project_id!r})' | |||
|
|
|||
| @util.deprecated_gate_set_parameter | |||
| def run( | |||
| async def run_async( | |||
There was a problem hiding this comment.
I think we're moving towards encouraging people to use EngineProcessor.run methods instead of Engine because AbstractEngine doesn't actually have any of these methods so they don't get mocked by the simulated versions.
There was a problem hiding this comment.
Reverted changes to the run* methods on Engine.
|
@mpharrigan, I've create #5526 with the "real" changes to use async grpc with |
There was a problem hiding this comment.
LGTM as written. Would love to see some more PRs that puts some async methods on @dstrain115 's base classes
maffoo
added
the
automerge
|
Automerge cancelled: A status check is failing. |
CirqBot
removed
the
automerge
This is WIP showing how we can add async support with duet to the various Engine classes that wrap the quantum engine API. In
EngineClientwe use anAsyncioExecutorto run the asyncio operations on the underlying gapic-generated code and expose them as duet-compatible futures. ForEngineClientand classes that call it, we expose async methods, as well as synchronous versions wrapped withduet.sync. I have not yet fixed up test, but have tried this out and it seems to work fine.