feat: implement async client for LROs #707

parthea · 2024-09-24T00:24:05Z

Towards b/362946071

This PR depends on #700 and #701

conventional-commit-lint-gcf · 2024-09-24T00:24:36Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

…ng operations

google/api_core/operations_v1/abstract_operations_async_client.py

tests/unit/operations_v1/test_operations_rest_client.py

google/api_core/operations_v1/__init__.py

vchudnov-g

I have some initial comments. I'll continue next week.

The main thing I want to work out in my head is the class hierarchy, and how it pertains to {sync, async} × {grpc, rest}.

What I've worked out so far is this class hierarchy, but I don't see where gRPC fits in. I need to think about this more, and it might be worth a discussion:


# do any of these include gRPC?

AbstractOperationsBaseClient   o----  OperationsRestTransport
   ^ ^                         o----  OperationsRestAsyncTransport
   | |       
   | |       
   | AbstractOperationsClient       # sync
   |
   |
   AbstractOperationsAsyncClient

ListOperationsPagerBase
  ^  ^
  |  |
  |  |
  |  ListOperationsPager           # sync
  |
  |
  ListOperationsAsyncPager

google/api_core/operations_v1/__init__.py

google/api_core/operations_v1/abstract_operations_base_client.py

google/api_core/operations_v1/abstract_operations_async_client.py

vchudnov-g · 2024-10-03T18:34:39Z

google/api_core/operations_v1/abstract_operations_async_client.py

+        super().__init__(
+            credentials=credentials,  # type: ignore
+            # NOTE: If a transport is not provided, we force the client to use the async
+            # REST transport, as it should.


Why should it? This abstract operations class does not say it's REST-specific, so why couldn't we go with gRPC async? We should see default to one of the ones that are supported (have the right dependencies); if both gRPC and REST are possible, sure, we could have a policy as to which one we choose for the default. As per my other comment, if the plan is to do this as we clean up this code, etc., let's add a TODO with link to a tracking issue that includes setting the right default.

Regardless, I would remove the final clause:

Suggested change

# REST transport, as it should.

# REST transport.

Also, update the class doc above so instead if "If set to None, a transport is chosen automatically", it says "If set to None, this defaults to 'rest_asyncio'".

I don't think we need a TODO to add the right default. This is an async REST client so we're setting the right default value i.e. rest_asyncio.

What we may instead want to do is update the name / docstring of the class to make it clear that this is specific to async REST.

In future, we may want to spend some time to investigate if we can combine different transport and client class for gRPC and REST that we maintain.

So if this is an async rest client, why do we have a transport parameter at all? What do we do if the transport is not an async rest transport (ours or user-defined)? Might be worth filing a follow-up issue.

Users can still configure an instance of an async transport class but I agree that it is a bit weird. Can add a note about it in the issue that i file.

Filed: #726

vchudnov-g · 2024-10-03T18:43:07Z

google/api_core/operations_v1/abstract_operations_async_client.py

+try:
+    from google.auth.aio import credentials as ga_credentials  # type: ignore
+except ImportError as e:  # pragma: NO COVER
+    raise ImportError(


The name of the file and the name of the class do not make it clear that this is REST-specific. So async should work if at least one of { (REST dependencies), (gRPC dependencies) } are available. So it seems to me we should check for both sets of dependencies being absent before we error.

That, at least, when we get to the steady state of having everything implemented and cleaned up. If that's the reason we're not doing that now, let's add a TODO with a link to a tracking issue.

You're right. The name doesn't specify it but it is indeed REST specific. I agree that it's confusing. The name used here is consistent to what we have for the sync REST client / transport. We can have a discussion on what we want to name our async client / transport.

Updating the sync client name would be a breaking change.

google/api_core/operations_v1/abstract_operations_async_client.py

vchudnov-g · 2024-10-03T18:59:18Z

google/api_core/operations_v1/abstract_operations_async_client.py

+        page_token: Optional[str] = None,
+        timeout: Optional[float] = None,
+        metadata: Sequence[Tuple[str, str]] = (),
+    ) -> pagers.ListOperationsAsyncPager:


AbstractOperationsClient has a compression parameter. Since This abstract class is, I assume, also applicable to gRPC, we should accept that parameter.

AbstractOperationsClient shouldn't have a compression parameter since it's a REST only client. We have a separate client for gRPC.

tests/unit/operations_v1/test_operations_rest_client.py

ohmayr · 2024-10-07T16:06:45Z

@vchudnov-g I've addressed all of your comments.

The only thing that's left and is causing a bit of confusion is the name of client/transport class. Let's discuss this, address it accordingly, and get this over the line since I believe it's not a big change.

vchudnov-g

Yes, let's have a clearer naming convention for files and classes. We want to cover {sync, async} × {REST, gRPC}.

In the ideal world, we would have prefixes for each of those two dimensions:

`Foo` (eg `Transport` or `Client`)
`SyncFoo`		`AsyncFoo`
`gRPCSyncFoo`	`RestSyncFoo`	`gRPCAsyncFoo`	`RestAsyncFoo`

Given historical naming and wanting to keep backward-compatibility, maybe we want something like this?:

`BaseFoo` (eg `BaseTransport` or `BaseClient`)
`BaseSyncFoo`		`BaseAsyncFoo`
`Foo`	`RestSyncFoo`	`AsyncFoo`	`RestAsyncFoo`

Let's discuss the right names.....

Also, we should consider that we could have the "ideal" names in their own logical namespace, and assign the legacy names in the legacy namespace, e.e.:
transports.Foo = transports.ideal.gRPCSyncFoo
transports.AsyncFoo = transports.ideal.gRPCAsyncFoo
We'd have to figure out what name to use; it obviously wouldn't literally be "ideal".
Luckily, we can do this now or later, so we have options.

tests/unit/operations_v1/test_operations_rest_client.py

vchudnov-g

Thanks for the discussion on the class hierarchy now and how we want it to evolve. Googlers, see b/372066378

vchudnov-g · 2024-10-07T22:24:56Z

google/api_core/operations_v1/abstract_operations_async_client.py

+        super().__init__(
+            credentials=credentials,  # type: ignore
+            # NOTE: If a transport is not provided, we force the client to use the async
+            # REST transport, as it should.


So if this is an async rest client, why do we have a transport parameter at all? What do we do if the transport is not an async rest transport (ours or user-defined)? Might be worth filing a follow-up issue.

* 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: xl Pull request size is extra large. label Sep 24, 2024

parthea changed the base branch from main to operations-rest-async-transport September 24, 2024 00:24

parthea force-pushed the operations-rest-async-client branch 3 times, most recently from 1492985 to cb2eab0 Compare September 25, 2024 15:32

parthea changed the base branch from operations-rest-async-transport to main September 25, 2024 17:35

parthea force-pushed the operations-rest-async-client branch from 787f0ba to 35d88d7 Compare September 25, 2024 17:55

parthea changed the title ~~[WIP] Operations rest async client~~ feat: Add AbstractOperationsAsyncClient Sep 25, 2024

ohmayr force-pushed the operations-rest-async-client branch from 5ea5e27 to 4d75cca Compare October 1, 2024 20:30

ohmayr changed the base branch from main to operations-rest-async-transport October 1, 2024 20:31

ohmayr force-pushed the operations-rest-async-client branch 2 times, most recently from 5e6c365 to 74e9bb8 Compare October 1, 2024 21:02

ohmayr changed the base branch from operations-rest-async-transport to main October 1, 2024 21:03

ohmayr marked this pull request as ready for review October 1, 2024 21:09

ohmayr requested review from a team as code owners October 1, 2024 21:09

blunderbuss-gcf bot assigned vchudnov-g Oct 1, 2024

ohmayr force-pushed the operations-rest-async-client branch from 4f3671c to 2af34aa Compare October 2, 2024 20:45

ohmayr changed the base branch from main to operations-rest-async-transport October 2, 2024 21:26

Base automatically changed from operations-rest-async-transport to async-rest-lro-support-in-core October 3, 2024 05:06

ohmayr changed the title ~~feat: Add AbstractOperationsAsyncClient~~ feat: implement async client for LROs Oct 3, 2024

parthea and others added 8 commits October 3, 2024 05:19

feat: implement AbstractOperationsAsyncClient to support long runni…

4613cb3

…ng operations

remove coverage guards

009a5ee

address presubmit failures

0f7a7d0

fix coverage for cancel operation

5b51c64

tests cleanup

50d751e

fix incorrect tests

5a4af8d

file bugs

2dea508

add auth import

f448498

ohmayr force-pushed the operations-rest-async-client branch from 5564d01 to f448498 Compare October 3, 2024 05:19

ohmayr reviewed Oct 3, 2024

View reviewed changes

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

tests/unit/operations_v1/test_operations_rest_client.py Show resolved Hide resolved

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

vchudnov-g reviewed Oct 3, 2024

View reviewed changes

address PR comments

ec2baa4

ohmayr changed the base branch from async-rest-lro-support-in-core to main October 7, 2024 02:10

ohmayr added 2 commits October 7, 2024 02:37

address PR comments

1913c76

fix unit tests and address more comments

80e27e4

ohmayr changed the base branch from main to async-rest-lro-support-in-core October 7, 2024 05:10

ohmayr added 2 commits October 7, 2024 07:11

disable retry parameter

dbf4abd

add retry parameter

53591d8

vchudnov-g reviewed Oct 7, 2024

View reviewed changes

tests/unit/operations_v1/test_operations_rest_client.py Outdated Show resolved Hide resolved

address PR comments

c0daf3d

vchudnov-g approved these changes Oct 7, 2024

View reviewed changes

ohmayr merged commit 043785e into async-rest-lro-support-in-core Oct 7, 2024
5 checks passed

ohmayr deleted the operations-rest-async-client branch October 7, 2024 22:34

vchudnov-g 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 async client for LROs #707

feat: implement async client for LROs #707

parthea commented Sep 24, 2024 •

edited

Loading

conventional-commit-lint-gcf bot commented Sep 24, 2024 •

edited

Loading

vchudnov-g left a comment

vchudnov-g Oct 3, 2024

ohmayr Oct 7, 2024

vchudnov-g Oct 7, 2024

ohmayr Oct 7, 2024

ohmayr Oct 8, 2024

vchudnov-g Oct 3, 2024

ohmayr Oct 3, 2024

vchudnov-g Oct 3, 2024

ohmayr Oct 3, 2024

ohmayr commented Oct 7, 2024

vchudnov-g left a comment •

edited

Loading

vchudnov-g left a comment

vchudnov-g Oct 7, 2024

feat: implement async client for LROs #707

feat: implement async client for LROs #707

Conversation

parthea commented Sep 24, 2024 • edited Loading

conventional-commit-lint-gcf bot commented Sep 24, 2024 • edited Loading

vchudnov-g 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

ohmayr commented Oct 7, 2024

vchudnov-g left a comment • edited Loading

Choose a reason for hiding this comment

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthea commented Sep 24, 2024 •

edited

Loading

conventional-commit-lint-gcf bot commented Sep 24, 2024 •

edited

Loading

vchudnov-g left a comment •

edited

Loading