AsyncIO Integration [Part 1] #26

lidizheng · 2020-04-23T21:10:59Z

Since the PR in microgenerator is pretty stable, the existing set of AsyncIO features should be sufficient to support generated code. I plan to split the AsyncIO integration into 3 parts (please let me know if anything I can do to help review): utility modules, LRO related modules, grpc helpers modules.

This PR contains following changes:

Nox configuration
AsyncIO retry module
AsyncIO config parsing module
Exception parsing patch
Corresponding unit tests

Related #23

lidizheng · 2020-04-23T22:42:47Z

@busunkim96 PTAL.

busunkim96

LGTM. At some point the docs will need to be updated to include the new classes (https://github.com/googleapis/python-api-core/tree/master/docs). Does it make more sense to do that now or to wait for other parts to be added?

busunkim96 · 2020-04-28T00:06:50Z

tests/asyncio/test_retry_async.py

@@ -0,0 +1,397 @@
+# Copyright 2017 Google LLC


Suggested change

# Copyright 2017 Google LLC

# Copyright 2020 Google LLC

tests/asyncio/gapic/test_config_async.py

lidizheng · 2020-04-28T17:25:20Z

@busunkim96 I will add doc in a separate PR. It might be better to update the doc for all new classes once.

lidizheng · 2020-04-28T18:41:25Z

@busunkim96 PTAL at the doc gen code.

After checking the doc generation code, I found you are using Sphinx so it actually is easy to add docs alone the code. So, I will try to add docs templates with code in following PRs.

software-dov

Tests look great, just had some nits to pick.

google/api_core/gapic_v1/__init__.py

google/api_core/retry_async.py

software-dov · 2020-04-28T20:29:47Z

google/api_core/retry_async.py

+    Args:
+        target(Callable): The function to call and retry. This must be a
+            nullary function - apply arguments with `functools.partial`.
+        predicate (Callable[Exception]): A callable used to determine if an


Maybe a name other than 'predicate' would be more helpful. I like the lisp convention of naming predicates with a -p tail; maybe is_retryable_p

This function signature is mimicking the sync version, so we have consistent naming between two versions. I can change if you feel strongly.

Not too strongly, especially if we're trying to preserve a convention.

google/api_core/retry_async.py

software-dov · 2020-04-28T20:47:03Z

google/api_core/retry_async.py

+        Returns:
+            AsyncRetry: A new retry instance with the given deadline.
+        """
+        return AsyncRetry(


It might be worthwhile to add a replace method to AsyncRetry that takes optional parameters corresponding to the constructor params and returns a new AsyncRetry with the passed in overrides applied.

I think the original designer of this class try to use the builder pattern. Naming each one with_* makes the application code more meaningful semantically. WDYT?

I'm fine preserving the interfaces, but it seems like a lot of boilerplate for a common, straightforward desire: given an object, return a copy with specific fields updated. I think having a replace method that gets called by the with_deadline and with_predicate methods is still a net win.

Switched to use a common _replace method. Thanks for the suggestion.

noxfile.py

software-dov · 2020-04-28T20:59:24Z

tests/asyncio/gapic/test_config_async.py

+    assert retry._initial == 1.0
+    assert retry._multiplier == 2.5
+    assert retry._maximum == 120.0
+    assert retry._deadline == 600.0
+    assert timeout._initial == 120.0
+    assert timeout._multiplier == 1.0
+    assert timeout._maximum == 120.0


Why not define an __eq__ method to handle all this boilerplate? The retry error predicate would still need to be tested by hand, but it would clean this bit up.

Can you explain more about this comment? Do you mean an __eq__ method to compare a retry/timeout object and parsed JSON object?

lidizheng

@software-dov Thanks for the detailed review. I definitely learned today. PTALA.

google/api_core/gapic_v1/__init__.py

lidizheng · 2020-04-28T22:17:13Z

google/api_core/retry_async.py

+    Args:
+        target(Callable): The function to call and retry. This must be a
+            nullary function - apply arguments with `functools.partial`.
+        predicate (Callable[Exception]): A callable used to determine if an


This function signature is mimicking the sync version, so we have consistent naming between two versions. I can change if you feel strongly.

google/api_core/retry_async.py

lidizheng · 2020-04-28T22:30:47Z

google/api_core/retry_async.py

+        Returns:
+            AsyncRetry: A new retry instance with the given deadline.
+        """
+        return AsyncRetry(


I think the original designer of this class try to use the builder pattern. Naming each one with_* makes the application code more meaningful semantically. WDYT?

google/api_core/retry_async.py

noxfile.py

lidizheng · 2020-04-29T22:44:47Z

BTW, I don't have the permission to merge PRs (and I don't have the full context of the release process). So, if there is any blocker, please let me know, no rush.

busunkim96 · 2020-05-01T17:05:38Z

@lidizheng There's nothing blocking the release since the changes are additive. Let's release this next week.

Thanks for all your work on this and @software-dov thank you for the thorough review. 😁

software-dov

LGTM. One minor issue, no need to repost if you don't think it's worth the hassle.

software-dov · 2020-05-01T22:08:46Z

google/api_core/retry_async.py

+        Returns:
+            AsyncRetry: A new retry instance with the given deadline.
+        """
+        return AsyncRetry(


I'm fine preserving the interfaces, but it seems like a lot of boilerplate for a common, straightforward desire: given an object, return a copy with specific fields updated. I think having a replace method that gets called by the with_deadline and with_predicate methods is still a net win.

This change includes: * Nox configuration support for AsynciO unit tests * No pre release gRPC Python required * AsyncIO retry module * AsyncIO config parsing module * Exception parsing patch * Corresponding unit test cases

Children PR of #26. This PR includes AsyncIO version of: * Polling future * Page iterator The AsyncIO version of polling future still uses the same mechanism as the sync version. The AsyncIO polling future tries to update its own state whenever the application want to access information or perform actions. For page iterator, it has similar interface design as sync version. But instead of fulfilling normal generator protocol, it is based on the async generator. Related #23

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 23, 2020

lidizheng force-pushed the aio-part-1 branch 2 times, most recently from 997a7b5 to 0f9b66f Compare April 23, 2020 21:39

lidizheng marked this pull request as ready for review April 23, 2020 22:42

lidizheng force-pushed the aio-part-1 branch from bba853d to eaf7aea Compare April 24, 2020 17:22

busunkim96 approved these changes Apr 28, 2020

View reviewed changes

lidizheng force-pushed the aio-part-1 branch 2 times, most recently from 4589485 to 97fa36c Compare April 28, 2020 18:39

busunkim96 requested a review from software-dov April 28, 2020 19:29

software-dov reviewed Apr 28, 2020

View reviewed changes

lidizheng force-pushed the aio-part-1 branch from 97fa36c to c0aa09b Compare April 28, 2020 23:03

lidizheng commented Apr 28, 2020

View reviewed changes

lidizheng force-pushed the aio-part-1 branch from c0aa09b to a647aba Compare April 30, 2020 18:15

software-dov approved these changes May 1, 2020

View reviewed changes

feat: First batch of AIO integration

d2e6ec6

This change includes: * Nox configuration support for AsynciO unit tests * No pre release gRPC Python required * AsyncIO retry module * AsyncIO config parsing module * Exception parsing patch * Corresponding unit test cases

lidizheng force-pushed the aio-part-1 branch from a647aba to d2e6ec6 Compare May 4, 2020 17:57

This was referenced May 18, 2020

feat: AsyncIO Integration [Part 2] #28

Merged

feat: AsyncIO Integration [Part 3] #29

Merged

busunkim96 merged commit a82f289 into googleapis:master May 27, 2020

busunkim96 mentioned this pull request Jun 17, 2020

WIP: AsyncIO integration #22

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AsyncIO Integration [Part 1] #26

AsyncIO Integration [Part 1] #26

lidizheng commented Apr 23, 2020

lidizheng commented Apr 23, 2020

busunkim96 left a comment

busunkim96 Apr 28, 2020

lidizheng commented Apr 28, 2020

lidizheng commented Apr 28, 2020

software-dov left a comment

software-dov Apr 28, 2020

lidizheng Apr 28, 2020

software-dov May 1, 2020

software-dov Apr 28, 2020

lidizheng Apr 28, 2020

software-dov May 1, 2020

lidizheng May 4, 2020

software-dov Apr 28, 2020

lidizheng May 4, 2020

lidizheng left a comment

lidizheng Apr 28, 2020

lidizheng Apr 28, 2020

lidizheng commented Apr 29, 2020

busunkim96 commented May 1, 2020

software-dov left a comment

software-dov May 1, 2020

AsyncIO Integration [Part 1] #26

AsyncIO Integration [Part 1] #26

Conversation

lidizheng commented Apr 23, 2020

lidizheng commented Apr 23, 2020

busunkim96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidizheng commented Apr 28, 2020

lidizheng commented Apr 28, 2020

software-dov 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

lidizheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidizheng commented Apr 29, 2020

busunkim96 commented May 1, 2020

software-dov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment