NotificationsYou must be signed in to change notification settings
Fork90
Star131

feat: add support for asynchronous rest streaming#686

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ohmayr merged 30 commits intomainfromadd-support-for-async-rest-streaming

Sep 18, 2024

Merged

feat: add support for asynchronous rest streaming#686

ohmayr merged 30 commits intomainfromadd-support-for-async-rest-streaming

Sep 18, 2024

Conversation

Copy link

Contributor

ohmayr commentedAug 11, 2024•
edited
Loading

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-labelbot added the size: lPull request size is large. label

Aug 11, 2024

ohmayr assignedvchudnov-g

Aug 20, 2024

ohmayr marked this pull request as ready for review

August 20, 2024 15:45

ohmayr requested review froma team ascode owners

August 20, 2024 15:45

ohmayr changed the base branch fromasync-rest-support-in-core tomain

August 22, 2024 17:20

vchudnov-g reviewed

Aug 22, 2024

View reviewed changes

Copy link

Contributor

vchudnov-g left a comment•
edited
Loading

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 inhttps://devblogs.microsoft.com/oldnewthing/20190919-00/?p=102904 ? It will make it easier to review, and preserve git history for the future.

ohmayr added5 commits

August 23, 2024 15:16

duplicating file to base

4df2120

restore original file

26f52a5

duplicate file to async

204920a

restore original file

2014ef6

Merge branch 'dups' into add-support-for-async-rest-streaming

fdeb437

ohmayr force-pushed theadd-support-for-async-rest-streaming branch from29d662b tofdeb437Compare

August 23, 2024 15:26

ohmayrand others added10 commits

August 23, 2024 15:33

duplicate test file for async

8014f47

restore test file

e84f03a

Merge branch 'dups' into add-support-for-async-rest-streaming

91f5c13

feat: add support for asynchronous rest streaming

0c2ee5c

🦉 Updates from OwlBot post-processor

75ae7d7

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

fix naming issue

db7cfb3

Merge branch 'add-support-for-async-rest-streaming' of github.com:goo…

9da0113

…gleapis/python-api-core into add-support-for-async-rest-streaming

fix import module name

87eeca3

pull auth feature branch

d6abddd

revert setup file

a6a648d

vchudnov-g reviewed

Aug 23, 2024

View reviewed changes

Copy link

Contributor

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 OutdatedShow resolvedHide resolved

google/api_core/rest_streaming.pyShow resolvedHide resolved

google/api_core/rest_streaming_async.py OutdatedShow resolvedHide resolved

tests/unit/test_rest_streaming_async.py OutdatedShow resolvedHide resolved

ohmayr mentioned this pull request

Aug 24, 2024

Setrandom.seed explicitly in each test function for rest streaming#689

Open

ohmayrand others added3 commits

August 24, 2024 20:52

address PR comments

8d30b2d

🦉 Updates from OwlBot post-processor

0b51b09

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

run black

7d8f1e1

Copy link

ContributorAuthor

ohmayr commentedAug 24, 2024

I will movetest_rest_streaming_async.py fromtests/unit totests/asyncio which is where we have our async tests once we do not have any other open concerns.

vchudnov-g approved these changes

Aug 26, 2024

View reviewed changes

Copy link

Contributor

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.pyShow resolvedHide resolved

google/api_core/_rest_streaming_base.py OutdatedShow resolvedHide resolved

google/api_core/rest_streaming.py Outdated

		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

vchudnov-gAug 26, 2024

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

ContributorAuthor

ohmayrSep 11, 2024

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 OutdatedShow resolvedHide resolved

tests/helpers.py OutdatedShow resolvedHide resolved

ohmayr added7 commits

August 27, 2024 05:35

address PR comments

d793069

Merge branch 'add-support-for-async-rest-streaming' of github.com:goo…

5f48f72

…gleapis/python-api-core into add-support-for-async-rest-streaming

update nox coverage

70d1cdb

address PR comments

ee3a5ac

fix nox session name in workflow

5a6e488

use https for remote repo

f2180da

add context manager methods

bc811e5

ohmayr added the do not mergeIndicates a pull request not ready for merge, due to either quality or timing. label

Aug 27, 2024

ohmayr commented

Sep 2, 2024

View reviewed changes

google/api_core/rest_streaming_async.py OutdatedShow resolvedHide resolved

ohmayr added3 commits

September 11, 2024 05:46

address PR comments

fc74ae0

Merge branch 'main' into add-support-for-async-rest-streaming

1603818

update auth error versions

cee06ab

ohmayr removed the do not mergeIndicates a pull request not ready for merge, due to either quality or timing. label

Sep 18, 2024

ohmayr added2 commits

September 18, 2024 11:09

Merge branch 'main' into add-support-for-async-rest-streaming

380dedf

update import error

750d4cb

ohmayr merged commit1b7bb6d intomain

Sep 18, 2024

39 checks passed

ohmayr deleted the add-support-for-async-rest-streaming branch

September 18, 2024 15:36

release-pleasebot mentioned this pull request

Sep 18, 2024

chore(main): release 2.20.0#693

Merged

Labels

size: l

Pull request size is large.

2 participants

Movatterモバイル変換

feat: add support for asynchronous rest streaming#686

feat: add support for asynchronous rest streaming#686

Uh oh!

Conversation

ohmayr commentedAug 11, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

vchudnov-g left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ohmayr commentedAug 24, 2024

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vchudnov-gAug 26, 2024

Choose a reason for hiding this comment

Uh oh!

ohmayrSep 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ohmayr commentedAug 11, 2024•
edited
Loading

vchudnov-g left a comment•
edited
Loading