Use duet for asynchrony, in particular for Collector #4009

maffoo · 2021-04-08T22:52:26Z

We are planning to switch to duet for supporting async code, with one of the reasons being that it is "reentrant" and supports nested invocations of the event loop (duet.run). We don't yet have much async code in cirq, but do use it for Collector and PauliSumCollector and this can lead to problems if running in a context where the asyncio event loop is already running (#4004). The solution proposed there is to patch asyncio to make it reentrant, using the nest-asyncio library, but this seems like something we should not rely on. Instead, I propose that we make the switch to duet.

The main question I have is whether we need to continue to support Collector.collect_async and Sampler.*_async methods being asyncio coroutines, or whether we can just switch them to be duet coroutines. In this PR as it stands I've just switched things, but if we want to support asyncio we would need to name the duet coroutine functions differently and then wrap them into an asyncio-compatible form for the existing methods.

95-martin-orion · 2021-04-15T16:03:21Z

Please fix the failing tests, then re-request review.

maffoo · 2021-04-15T17:48:13Z

Hm, we don't have a version of duet for 3.6, and it'd be quite hard to do because of some asnyc/await language features that landed in 3.7. @balopat, what is the plan for dropping python 3.6 support?

balopat · 2021-04-27T18:01:06Z

This is blocked on #3574, which is blocked on google3 having their default python runtime upgraded to 3.7.
If we want to move forward with duet for the OSS community (I think that this is great!) then we have to think about workarounds until that upgrade is done which might take a year still. A workaround design might mean to offer the current async flow on 3.6 and duet on 3.7 somehow.

balopat · 2021-07-09T19:45:16Z

This is blocked on #3574, which is blocked on google3 having their default python runtime upgraded to 3.7.
If we want to move forward with duet for the OSS community (I think that this is great!) then we have to think about workarounds until that upgrade is done which might take a year still. A workaround design might mean to offer the current async flow on 3.6 and duet on 3.7 somehow.

One idea:

We could add a cirq-duet module that would use duet for the collector.
People who have python 3.7+ could use that module, the ones using 3.6 can still use the old collector, on which we could add even a deprecation warning...

WDYT, @maffoo?

mpharrigan · 2021-07-09T23:31:24Z

Do we have an update on timeline for Python 3.7+?

maffoo · 2021-07-12T04:19:26Z

Actually, @fedimser is working on adding python 3.6 support to duet (google/duet#23) which will let us move forward with these async changes without waiting for 3.7 support in colab etc.

maffoo · 2021-07-13T01:21:30Z

This is ready for review. @balopat, @mpharrigan

mpharrigan

I think the actual code changes in this PR are very straightforward. I asked some clarifying questions and posed some nits.

mpharrigan · 2021-08-23T23:34:42Z

cirq-core/cirq/conftest.py

-            f'{pyfuncitem._obj.__name__} is async but not '
-            f'decorated with "@pytest.mark.asyncio".'
+            f'{pyfuncitem._obj.__name__} is a bare async function. '
+            f'Should be decorated with "@duet.sync".'


nit: It should

mpharrigan · 2021-08-23T23:35:47Z

cirq-core/cirq/testing/__init__.py

@@ -14,10 +14,6 @@

 """Utilities for testing code."""

-from cirq.testing.asynchronous import (
-    asyncio_pending,


This is technically a breaking change. I suspect no one externally has been using our async testing helper code? but we should make a note of this for the release

Brought this back since it's needed to test the work pool, which is now deprecated but not removed.

cirq-core/cirq/work/collector.py

mpharrigan · 2021-08-23T23:38:09Z

cirq-core/cirq/work/collector.py

        """
-        pool = work_pool.CompletionOrderedAsyncWorkPool()
+        results = duet.AsyncCollector[Tuple[CircuitSampleJob, 'cirq.Result']]()


what is going on here? Is this a form of type annotation I've never seen before or does this actually change the object that has been created?

The AsyncCollector class derives from typing.Generic, which among other things implements __getitem__ on the class to allow specifying generic type params like this. It doesn't actually create a new class but rather serves as a hint to the type checker. This could also be written as a type annotation, at the cost of some repetition:

results: duet.AsyncCollector[Tuple[CircuitSampleJob, 'cirq.Result']] = duet.AsyncCollector()

ok, glad it's just a type hint. my slight preference would be for the type annotation at the cost of some repetition but I defer to you

Switched to type annotation. It still just fits on one line :-)

mpharrigan · 2021-08-23T23:39:13Z

cirq-core/cirq/work/collector.py

+                if not job_error:
+                    results.error(error)
+                    job_error = error
+            else:


what is try .. else? why not put results.add inside the try?

The else clause executes if the try block does not raise any exceptions. I wanted to make clear that the except clause is only catching exceptions from the await call. If results.add were part of the try clause then the except would also handle errors from that; results.add shouldn't raise errors, but this is just to be safe.

thanks for explaining

mpharrigan · 2021-08-23T23:42:53Z

cirq-core/cirq/work/work_pool.py

-from typing import Optional, Awaitable, Union
-
-
-class CompletionOrderedAsyncWorkPool:


this seems like something someone may be relying on in cirq user land?

It's certainly possible. I can restore this and deprecate it instead.

mpharrigan · 2021-08-24T16:05:20Z

cirq-core/cirq/work/collector.py

+                if not job_error:
+                    results.error(error)
+                    job_error = error
+            else:


thanks for explaining

review-notebook-app · 2021-08-24T16:46:47Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

dstrain115

In general, it might be good to put more line comments in where duet is used, since it will be unfamiliar to the majority of developers, but I don't have any other objections to pulling this in.

@mpharrigan

This switches async sampler methods to use the duet library (https://github.com/google/duet). Duet is "reentrant" in that it supports nested invocations of the event loop (duet.run). This differs from most other python async libraries which are not natively reentrant (without patching as in the case of nest-asyncio for asyncio). Re-entrancy makes it possible to refactor code incrementally to use duet whereas non-reentrant libraries typically require more global changes. The asyncio-based work pool and testing utilities are deprecated and will be removed in a future release. Review: @mpharrigan, @dstrain115

@mpharrigan

This switches async sampler methods to use the duet library (https://github.com/google/duet). Duet is "reentrant" in that it supports nested invocations of the event loop (duet.run). This differs from most other python async libraries which are not natively reentrant (without patching as in the case of nest-asyncio for asyncio). Re-entrancy makes it possible to refactor code incrementally to use duet whereas non-reentrant libraries typically require more global changes. The asyncio-based work pool and testing utilities are deprecated and will be removed in a future release. Review: @mpharrigan, @dstrain115

maffoo requested review from cduck, vtomole and a team as code owners April 8, 2021 22:52

google-cla bot added the cla: yes Makes googlebot stop complaining. label Apr 8, 2021

maffoo added 2 commits April 15, 2021 10:35

Use duet for async function, in particular for Collector

6cdd308

Fix lint and type issues

2092b21

maffoo force-pushed the u/maffoo/duet branch from 7c90792 to 2092b21 Compare April 15, 2021 17:40

maffoo added 2 commits April 22, 2021 11:49

Merge branch 'master' into u/maffoo/duet

6ab081e

Merge branch 'master' into u/maffoo/duet

9c4ce8e

balopat added the status/blocked This PR is blocked on another issue label Apr 27, 2021

rmlarose mentioned this pull request Jun 12, 2021

[docs] Error message in output cell #4197

Closed

balopat self-assigned this Jul 1, 2021

maffoo mentioned this pull request Jul 7, 2021

Add support for python 3.6 google/duet#22

Closed

maffoo added 3 commits July 12, 2021 16:20

Merge branch 'master' into u/maffoo/duet

a9b3ed6

Update to duet 0.2 with new package name

3d89722

Fix direct_fidelity_estimation_test

bf1d5e7

maffoo removed the status/blocked This PR is blocked on another issue label Jul 13, 2021

CirqBot added size: XL lines changed >1000 size: L 250< lines changed <1000 and removed size: XL lines changed >1000 labels Aug 12, 2021

95-martin-orion assigned mpharrigan and unassigned balopat Aug 23, 2021

mpharrigan reviewed Aug 23, 2021

View reviewed changes

maffoo added 3 commits August 23, 2021 22:53

Merge branch 'master' into u/maffoo/duet

70761f1

Fixes from review

b289af6

Restore work_pool[_test].py

7af7d21

mpharrigan approved these changes Aug 24, 2021

View reviewed changes

cirq-core/cirq/work/collector.py

if not job_error:

results.error(error)

job_error = error

else:

Copy link

Collaborator

mpharrigan Aug 24, 2021

Choose a reason for hiding this comment

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

thanks for explaining

maffoo added 2 commits August 24, 2021 09:19

Switch to type annotation

3014a2a

Remove nest-asyncio from operators_and_observables notebook

dd22f24

Remove scary warning cell

ad97b7c

dstrain115 approved these changes Aug 24, 2021

View reviewed changes

Merge branch 'master' into u/maffoo/duet

499f817

maffoo merged commit de31ee9 into master Aug 25, 2021

maffoo deleted the u/maffoo/duet branch August 25, 2021 16:25

maffoo mentioned this pull request Aug 25, 2021

Operators & observables notebook raises runtime error after upgrading notebook #4470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use duet for asynchrony, in particular for Collector #4009

Use duet for asynchrony, in particular for Collector #4009

maffoo commented Apr 8, 2021

95-martin-orion commented Apr 15, 2021

maffoo commented Apr 15, 2021

balopat commented Apr 27, 2021

balopat commented Jul 9, 2021

mpharrigan commented Jul 9, 2021

maffoo commented Jul 12, 2021

maffoo commented Jul 13, 2021

mpharrigan left a comment

mpharrigan Aug 23, 2021

maffoo Aug 24, 2021

mpharrigan Aug 23, 2021

maffoo Aug 24, 2021

mpharrigan Aug 23, 2021

maffoo Aug 24, 2021

mpharrigan Aug 24, 2021

maffoo Aug 24, 2021

mpharrigan Aug 23, 2021

maffoo Aug 24, 2021

mpharrigan Aug 24, 2021

mpharrigan Aug 23, 2021

maffoo Aug 24, 2021

mpharrigan Aug 24, 2021

review-notebook-app bot commented Aug 24, 2021

dstrain115 left a comment

		from typing import Optional, Awaitable, Union


		class CompletionOrderedAsyncWorkPool:

Use duet for asynchrony, in particular for Collector #4009

Use duet for asynchrony, in particular for Collector #4009

Conversation

maffoo commented Apr 8, 2021

95-martin-orion commented Apr 15, 2021

maffoo commented Apr 15, 2021

balopat commented Apr 27, 2021

balopat commented Jul 9, 2021

mpharrigan commented Jul 9, 2021

maffoo commented Jul 12, 2021

maffoo commented Jul 13, 2021

mpharrigan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

review-notebook-app bot commented Aug 24, 2021

dstrain115 left a comment

Choose a reason for hiding this comment