feat: implement `OperationsRestAsyncTransport` to support long running operations #700

parthea · 2024-09-20T17:28:11Z

Towards b/362946071

This PR depends on #701

google/api_core/operations_v1/__init__.py

google/api_core/operations_v1/transports/__init__.py

google/api_core/operations_v1/transports/rest_asyncio.py

…ions

google/api_core/operations_v1/__init__.py

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

tests/unit/operations_v1/test_operations_rest_client.py

vchudnov-g · 2024-10-01T22:27:15Z

tests/unit/operations_v1/test_operations_rest_client.py

+from google.auth.aio import credentials as ga_credentials_async
+
+try:
+    import aiohttp


Do we use aiohttp in this file? A search reveals no references

We don't. We're only importing it to check that the library is installed with the async_rest extra.

Now, the actual problem is that if the libraries within the try block aren't installed, we silently skip the tests for async. Happy to discuss that.

So we try importing aiohttp and transport because (1) we'll need them elsewhere for async REST, and (2) we want to set the flag GOOGLE_AUTH_AIO_INSTALLED. We only look at the flag in this file to do the right thing.

I agree with doing the right thing. What makes me uneasy is the mysterious coupling with other modules "elsewhere" in (1). I think it would be more elegant to have a package-wide function or constant that takes care of querying the right things for async REST, probably by delegating to the files that use these imports.

However, this is a test file, so no need to block on this. But we should follow up.

tests/unit/operations_v1/test_operations_rest_client.py

google/api_core/operations_v1/transports/__init__.py

google/api_core/operations_v1/transports/rest_asyncio.py

vchudnov-g · 2024-10-02T17:31:38Z

google/api_core/operations_v1/transports/rest_asyncio.py

+        self,
+        request: operations_pb2.ListOperationsRequest,
+        *,
+        timeout: Optional[float] = None,


Why don't we have retry as in the sync case?

retry isn't passed down to the auth layer i.e. it's unused. We should file an issue to fix that for the case of sync and add this as a follow up feature to the async transport class.

Could you file an issue to do those things?

Filed issues for each.

google/api_core/operations_v1/transports/rest_asyncio.py

google/api_core/gapic_v1/client_info.py

vchudnov-g · 2024-10-02T21:35:47Z

tests/unit/operations_v1/test_operations_rest_client.py

+from google.auth.aio import credentials as ga_credentials_async
+
+try:
+    import aiohttp


So we try importing aiohttp and transport because (1) we'll need them elsewhere for async REST, and (2) we want to set the flag GOOGLE_AUTH_AIO_INSTALLED. We only look at the flag in this file to do the right thing.

I agree with doing the right thing. What makes me uneasy is the mysterious coupling with other modules "elsewhere" in (1). I think it would be more elegant to have a package-wide function or constant that takes care of querying the right things for async REST, probably by delegating to the files that use these imports.

However, this is a test file, so no need to block on this. But we should follow up.

google/api_core/operations_v1/transports/rest_asyncio.py

vchudnov-g · 2024-10-02T22:01:24Z

google/api_core/operations_v1/transports/rest_asyncio.py

+        self,
+        request: operations_pb2.ListOperationsRequest,
+        *,
+        timeout: Optional[float] = None,


Could you file an issue to do those things?

google/api_core/operations_v1/transports/rest_asyncio.py

google/api_core/operations_v1/transports/rest.py

google/api_core/operations_v1/transports/rest_asyncio.py

vchudnov-g · 2024-10-02T22:24:48Z

google/api_core/operations_v1/transports/rest_asyncio.py

+DEFAULT_CLIENT_INFO = gapic_v1.client_info.ClientInfo(
+    gapic_version=BASE_DEFAULT_CLIENT_INFO.gapic_version,
+    grpc_version=None,
+    rest_version=auth_version,


I also just filed #719 to rationalize this further, separately.

google/api_core/operations_v1/transports/rest_asyncio.py

* feat: implement `OperationsRestAsyncTransport` to support long running operations (#700) * feat: Add OperationsRestAsyncTransport to support long running operations * update TODO comment * update TODO comment * address feedback * address feedback * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * fix mypy and lint issues * minor fix * add no cover * fix no cover tag * link coverage issue * silence coverage issue * fix statement name error * address PR feedback * address PR feedback * address PR comments --------- Co-authored-by: ohmayr <omairnaveed@ymail.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * feat: implement async client for LROs (#707) * feat: implement `AbstractOperationsAsyncClient` to support long running operations * remove coverage guards * address presubmit failures * fix coverage for cancel operation * tests cleanup * fix incorrect tests * file bugs * add auth import * address PR comments * address PR comments * fix unit tests and address more comments * disable retry parameter * add retry parameter * address PR comments --------- Co-authored-by: ohmayr <omairnaveed@ymail.com> Co-authored-by: ohmayr <omairn@google.com> * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Anthonios Partheniou <partheniou@google.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

product-auto-label bot added the size: l Pull request size is large. label Sep 20, 2024

parthea force-pushed the operations-rest-async-transport branch 7 times, most recently from 21bdadd to 1e108b4 Compare September 20, 2024 20:30

parthea changed the base branch from main to add-extra-for-aiohttp September 20, 2024 20:30

parthea force-pushed the operations-rest-async-transport branch 2 times, most recently from 0ee82e0 to 6e51532 Compare September 20, 2024 20:50

parthea mentioned this pull request Sep 23, 2024

Add support for mtls in OperationsRestAsyncTransport #705

Open

parthea force-pushed the operations-rest-async-transport branch 2 times, most recently from b6c1c6c to eb78445 Compare September 23, 2024 16:43

parthea mentioned this pull request Sep 23, 2024

Add support for service_account.Credentials in OperationsRestAsyncTransport #706

Open

parthea marked this pull request as ready for review September 25, 2024 15:06

parthea requested review from a team as code owners September 25, 2024 15:06

blunderbuss-gcf bot assigned ohmayr Sep 25, 2024

This was referenced Sep 25, 2024

Add support for default_host in OperationsRestAsyncTransport #708

Open

feat: implement async client for LROs #707

Merged

ohmayr reviewed Sep 25, 2024

View reviewed changes

ohmayr changed the title ~~feat: Add OperationsRestAsyncTransport to support long running operations~~ feat: implement OperationsRestAsyncTransport to support long running operations Sep 30, 2024

Base automatically changed from add-extra-for-aiohttp to main September 30, 2024 20:58

parthea and others added 5 commits September 30, 2024 20:59

feat: Add OperationsRestAsyncTransport to support long running operat…

09ce3b0

…ions

update TODO comment

daca264

update TODO comment

9f980fd

address feedback

f52378b

address feedback

f880c72

ohmayr force-pushed the operations-rest-async-transport branch from bf76338 to f880c72 Compare September 30, 2024 20:59

ohmayr reviewed Sep 30, 2024

View reviewed changes

google/api_core/operations_v1/__init__.py Outdated Show resolved Hide resolved

gcf-owl-bot bot and others added 8 commits September 30, 2024 21:01

🦉 Updates from OwlBot post-processor

34b795f

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

fix mypy and lint issues

62f0c39

minor fix

3bfad42

add no cover

3e9a1a6

fix no cover tag

7c08eeb

link coverage issue

cdfa64f

silence coverage issue

2891173

fix statement name error

acdadee

ohmayr changed the base branch from main to async-rest-lro-support-in-core October 1, 2024 19:45

vchudnov-g reviewed Oct 2, 2024

View reviewed changes

ohmayr added 2 commits October 2, 2024 19:34

address PR feedback

62dfff9

address PR feedback

f93e6bd

vchudnov-g approved these changes Oct 2, 2024

View reviewed changes

address PR comments

637a361

vchudnov-g approved these changes Oct 2, 2024

View reviewed changes

ohmayr merged commit 3c7e43e into async-rest-lro-support-in-core Oct 3, 2024
5 checks passed

ohmayr deleted the operations-rest-async-transport branch October 3, 2024 05:06

ohmayr mentioned this pull request Oct 7, 2024

feat: add support for asynchronous long running operations #724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement `OperationsRestAsyncTransport` to support long running operations #700

feat: implement `OperationsRestAsyncTransport` to support long running operations #700

parthea commented Sep 20, 2024 •

edited

Loading

vchudnov-g Oct 1, 2024

ohmayr Oct 2, 2024

vchudnov-g Oct 2, 2024

vchudnov-g Oct 2, 2024

ohmayr Oct 2, 2024

vchudnov-g Oct 2, 2024

ohmayr Oct 3, 2024

vchudnov-g Oct 2, 2024

vchudnov-g Oct 2, 2024

vchudnov-g Oct 2, 2024

feat: implement OperationsRestAsyncTransport to support long running operations #700

feat: implement OperationsRestAsyncTransport to support long running operations #700

Conversation

parthea commented Sep 20, 2024 • edited Loading

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

feat: implement `OperationsRestAsyncTransport` to support long running operations #700

feat: implement `OperationsRestAsyncTransport` to support long running operations #700

parthea commented Sep 20, 2024 •

edited

Loading