Skip to content

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

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
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 Outdated
operation_future = api.async_recognize(config=config, audio=audio)

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

daspecster commented Jan 27, 2017

@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

daspecster commented Jan 27, 2017

@lukesneeringer the _OperationFuture usage is updated. PTAL

lukesneeringer
lukesneeringer suggested changes Jan 30, 2017
View reviewed changes
speech/google/cloud/speech/_gax.py
operation_future = api.async_recognize(config=config, audio=audio)

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

daspecster commented Feb 2, 2017

@lukesneeringer do you have anything else for this?

@lukesneeringer
Copy link
Contributor

lukesneeringer commented Feb 2, 2017

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

@lukesneeringer
Copy link
Contributor

lukesneeringer commented Feb 3, 2017

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

3 more reviewers

@bjwatson bjwatson bjwatson left review comments

@evaogbe evaogbe evaogbe left review comments

@lukesneeringer lukesneeringer lukesneeringer approved these changes

Reviewers whose approvals may not affect merge requirements

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