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 asynchronous rest streaming #686

Merged
merged 30 commits into from
Sep 18, 2024

Conversation

ohmayr
Copy link
Contributor

@ohmayr ohmayr commented Aug 11, 2024

This PR adds support for asynchronous rest streaming in core to be leveraged in GAPICs.

The changes in this PR can be tested against: googleapis/google-auth-library-python#1577.

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Aug 11, 2024
@ohmayr ohmayr marked this pull request as ready for review August 20, 2024 15:45
@ohmayr ohmayr requested review from a team as code owners August 20, 2024 15:45
@ohmayr ohmayr changed the base branch from async-rest-support-in-core to main August 22, 2024 17:20
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.

Could you "fork" the file you're refactoring from into the file you're refacotring "to", adapting the dirctions in https://devblogs.microsoft.com/oldnewthing/20190919-00/?p=102904 ? It will make it easier to review, and preserve git history for the future.

@ohmayr ohmayr force-pushed the add-support-for-async-rest-streaming branch from 29d662b to fdeb437 Compare August 23, 2024 15:26
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!

google/api_core/_rest_streaming_base.py Outdated Show resolved Hide resolved
google/api_core/_rest_streaming_base.py Outdated Show resolved Hide resolved
google/api_core/_rest_streaming_base.py Outdated Show resolved Hide resolved
google/api_core/rest_streaming.py Show resolved Hide resolved
google/api_core/rest_streaming_async.py Outdated Show resolved Hide resolved
tests/unit/test_rest_streaming_async.py Outdated Show resolved Hide resolved
tests/unit/test_rest_streaming_async.py Outdated Show resolved Hide resolved
tests/unit/test_rest_streaming_async.py Outdated Show resolved Hide resolved
tests/unit/test_rest_streaming_async.py Outdated Show resolved Hide resolved
tests/unit/test_rest_streaming_async.py Outdated Show resolved Hide resolved
@ohmayr
Copy link
Contributor Author

ohmayr commented Aug 24, 2024

I will move test_rest_streaming_async.py from tests/unit to tests/asyncio which is where we have our async tests once we do not have any other open concerns.

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.

Please address the comments, but no blockers. Thanks for doing this!

google/api_core/rest_streaming.py Show resolved Hide resolved
google/api_core/_rest_streaming_base.py Outdated Show resolved Hide resolved
ValueError: If `response_message_cls` is not a subclass of `proto.Message` or `google.protobuf.message.Message`.
ValueError:
- If `response_message_cls` is not a subclass of `proto.Message` or `google.protobuf.message.Message`.
- If `response` is not an instance of `requests.Response`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this exception raised? Line 46 will succeed with a matching method of any class, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's reasonable to not raise an error for this. We've added the relevant type hints. Cleaned up the docstring.

tests/unit/test_rest_streaming_async.py Outdated Show resolved Hide resolved
tests/helpers.py Outdated Show resolved Hide resolved
@ohmayr ohmayr added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 27, 2024
@ohmayr ohmayr removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 18, 2024
@ohmayr ohmayr merged commit 1b7bb6d into main Sep 18, 2024
39 checks passed
@ohmayr ohmayr deleted the add-support-for-async-rest-streaming branch September 18, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants