Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for creating exceptions from an asynchronous response #688

Merged
merged 11 commits into from
Sep 10, 2024

Conversation

ohmayr
Copy link
Contributor

@ohmayr ohmayr commented Aug 22, 2024

This PR implements a _RestCall class to map GoogleAPICallError to an asynchronous rest callable that raises google.auth.exceptions.TransportError.This wrapping is done via a helper method rest_helpers_async.wrap_errors.

A new parameter kind is introduced in wrap_method which can be one of grpc_asyncio (Default Value) or rest_asyncio to distinguish between the type of callables so that we can apply the appropriate wrapping logic.

The property kind is exposed at the transport level here and will be configured when wrapping methods during the transport initialization here.

As a follow up, a similar logic must be implemented for synchronous rest callables to avoid gRPC specific stack trace within a rest call.

RE: Updated Workflow:

I've removed the proposed _RestCall class after determining that it is not needed to wrap a rest callable and just adds unnecessary logic.

The new proposed workflow is as follows:

  • This PR still introduces a new parameter kind in wrap_method to determine the type of a callable i.e. grpc_asyncio or rest_asyncio.
  • If kind=="rest_asyncio", we skip the call to grpc_helpers_async.wrap_errors which is gRPC specific.

Essentially, this does the trick if we follow a similar pattern for error handling to what we're doing in a sync rest call.

We have been lazily creating and raising a GoogleAPICallError from a requests.Response within our GAPICs using the from_http_response method here. Therefore, we don't need to add the error handling logic to the callable beforehand (we're currently doing both the things for a sync rest call and should skip wrapping the callable beforehand there too).

This PR only handles the error handling logic for an async REST call. It also introduces a new helper i.e. format_http_response_error to create and raise a GoogleAPICallError for a more generic response since the existing from_http_response is specific to handling a requests response.

Alternate approach for the introduced helper:

The proposed implementation of format_http_response_error takes in payload as an argument so that we don't need to await for it within the call and keep this function synchronous.

Alternatively, we can have an async version of from_http_response that can be used to create GoogleAPICallError for async rest calls.

Follow Up:

As a follow up, we should skip wrapping sync rest callable with grpc_helpers.wrap_errors (similar to what we're doing here) to avoid the unnecessary gRPC specific logic within a rest call stracktrace.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Aug 22, 2024
@ohmayr ohmayr changed the base branch from add-support-for-async-rest-streaming to main August 23, 2024 06:41
@ohmayr ohmayr force-pushed the add-support-for-mapping-rest-callables branch from 98d205f to 573413a Compare August 23, 2024 06:43
@ohmayr ohmayr changed the title feat: add suport for mapping exceptions to rest callables feat: add support for creating exceptions from an asynchronous Response Aug 23, 2024
@ohmayr ohmayr changed the title feat: add support for creating exceptions from an asynchronous Response feat: Add support for creating exceptions from an asynchronous response Aug 23, 2024
@ohmayr ohmayr marked this pull request as ready for review August 23, 2024 06:47
@ohmayr ohmayr requested review from a team as code owners August 23, 2024 06:47
return message


def format_http_response_error(response, method, url, payload=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: add type hints
nit: add code comments to clarify the reason that we're moving away from from_http_response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed with a comment.

Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

Looks good. Just minor comments/questions. Please address @parthea's comments as well.

google/api_core/exceptions.py Show resolved Hide resolved
google/api_core/exceptions.py Show resolved Hide resolved
@@ -32,6 +32,7 @@ def wrap_method(
default_timeout=None,
default_compression=None,
client_info=client_info.DEFAULT_CLIENT_INFO,
kind="grpc",
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, and when we call wrap_method from the GAPIC, it will specify this parameter, right? So that will be in a follow-up PR in the generator repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct! Note to myself to update the default value to grpc_asyncio.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Update the PR description with the right default value.

@ohmayr ohmayr added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 29, 2024
@ohmayr
Copy link
Contributor Author

ohmayr commented Aug 29, 2024

Adding a do not merge label until open comments are addressed and the default value is updated to grpc_asyncio.

@ohmayr ohmayr removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 9, 2024
@daniel-sanche
Copy link
Contributor

Would this fix the issue in googleapis/gapic-generator-python#1764? Or is this something else?

@ohmayr
Copy link
Contributor Author

ohmayr commented Sep 10, 2024

googleapis/gapic-generator-python#1764

This PR removes grpc code path from the stack trace when an error is raised from an asynchronous rest call.

@ohmayr ohmayr merged commit 1c4b0d0 into main Sep 10, 2024
33 checks passed
@ohmayr ohmayr deleted the add-support-for-mapping-rest-callables branch September 10, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants