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.

Sign up for GitHub

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

Jump to bottom

Fix Speech LRO handling and add HTTP side for multiple results. #2965

Merged
merged 3 commits into from
Feb 6, 2017
Merged

Fix Speech LRO handling and add HTTP side for multiple results. #2965

merged 3 commits into from
Feb 6, 2017

Conversation

daspecster
Copy link
Contributor

@daspecster daspecster commented Jan 26, 2017 •
edited
Loading

This is an update to fill out the HTTP side of #2962 and I have a hacky but effective way to handle the new LRO.

I tried using the OperationsClient but for some reason it just kept 404ing. I'm not sure what I was doing wrong.

/cc @lukesneeringer

@daspecster daspecster added the api: speech Issues related to the Speech-to-Text API. label Jan 26, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 26, 2017
daspecster
daspecster commented Jan 26, 2017
View reviewed changes
speech/google/cloud/speech/_gax.py

return Operation.from_pb(response, self)
return Operation.from_pb(operation_future._operation, self)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@lukesneeringer
Copy link
Contributor

lukesneeringer commented Jan 26, 2017 •
edited
Loading

Ideally, I would prefer the user get the OperationFuture object from GAX, and would like to look into why @daspecster is having trouble making it work. I will try to do that in the next day or so.

That said, merging this is almost certainly preferable to (1) reverting #2962 (which fixes an item the Speech team identified as critical) or (2) leaving async broken. :-)

@daspecster daspecster mentioned this pull request Jan 27, 2017
Merged
@daspecster
Copy link
Contributor Author

@eoogbe I was getting a Rendezvous 404 when I tried to use OperationsClient.get_operation().
I pretty much followed the example, but I had to add the scope.

In this PR I'm just pulling _OperationFuture._operation but I'm sure that's not the intended way it should be used.

Any help would be greatly appreciated!

@daspecster
Copy link
Contributor Author

@lukesneeringer the _OperationFuture usage is updated. PTAL

lukesneeringer
lukesneeringer suggested changes Jan 30, 2017
View reviewed changes
speech/google/cloud/speech/_gax.py

return Operation.from_pb(response, self)
return Operation.from_pb(operation_future.last_operation_data(), self)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@daspecster daspecster mentioned this pull request Feb 1, 2017
Merged
@daspecster
Copy link
Contributor Author

@lukesneeringer do you have anything else for this?

@lukesneeringer
Copy link
Contributor

Yeah, I am writing an _OperationFuture clone to go into google.cloud.core this morning.

@lukesneeringer
Copy link
Contributor

Okay, I am aborting that for now. The rabbit hole is going too far. Yes, you can merge this.

@daspecster daspecster merged commit de65409 into googleapis:master Feb 6, 2017
@daspecster daspecster deleted the fix-speech-system-tests branch February 6, 2017 15:38
@daspecster daspecster mentioned this pull request Feb 24, 2017
Merged
19 tasks
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@daspecster
Fix Speech LRO handling and add HTTP side for multiple results. (goog…
fee39bf 
…leapis#2965)

* Update after googleapis#2962 to fill out http side and handle new LRO.

* Update _OperationsFuture usage.

* Mock OperationsClient.
atulep pushed a commit that referenced this pull request Apr 3, 2023
@daspecster
Fix Speech LRO handling and add HTTP side for multiple results. (#2965)
0d972c7 
* Update after #2962 to fill out http side and handle new LRO.

* Update _OperationsFuture usage.

* Mock OperationsClient.
atulep pushed a commit that referenced this pull request Apr 18, 2023
@daspecster
Fix Speech LRO handling and add HTTP side for multiple results. (#2965)
423aebf 
* Update after #2962 to fill out http side and handle new LRO.

* Update _OperationsFuture usage.

* Mock OperationsClient.
parthea pushed a commit that referenced this pull request Oct 22, 2023
@daspecster
Fix Speech LRO handling and add HTTP side for multiple results. (#2965)
fca6333 
* Update after #2962 to fill out http side and handle new LRO.

* Update _OperationsFuture usage.

* Mock OperationsClient.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evaogbe evaogbe evaogbe left review comments

@bjwatson bjwatson bjwatson left review comments

@lukesneeringer lukesneeringer lukesneeringer approved these changes

Assignees

@lukesneeringer lukesneeringer

@tseaver tseaver

Labels
api: speech Issues related to the Speech-to-Text API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@daspecster @lukesneeringer @bjwatson @evaogbe @tseaver @dhermes @googlebot