Speech transcribe async bug #688
Conversation
googlebot
added
the
cla: yes
|
@ryanmats all of the speech test resources should be pre-uploaded, so there should be no need to upload/delete the blob every time (see |
|
Other than the note about the tests, this LGTM. @jerjou can you take a quick look before we merge? |
| if operation.error.message: | ||
| print("") | ||
| print("Operation error:") | ||
| print(operation.error) |
There was a problem hiding this comment.
What? Jon didn't wasn't like, "Please use single quotes for strings"?? WHO ARE YOU AND WHAT HAVE YOU DONE TO @jonparrott ?!? :)
I'd just combine this into one string, for brevity:
print('\nOperation error:\n{}'.format(operation.error))
We should be paid proportional to like, the inverse of lines of code :) Except then we'd be incentivized to not write any code at all... hm...
There was a problem hiding this comment.
I blame my lack of sleep.
| assert re.search(r'how old is the Brooklyn Bridge', out, re.DOTALL | re.I) | ||
|
|
||
| # Delete audio file from storage bucket | ||
| blob.delete() |
There was a problem hiding this comment.
+1 just have the asset on the GCS bucket already :)
But for future reference, if you're going to do this, should probably put this in a context manager (so that eg if the assertion fails, the blob is still deleted), or in one of those fancy pytest fixtures (same idea)
|
Minor comments. Mainly just Jon's thing :) Otherwise, looks good. |
|
@jonparrott @jerjou I pushed the changes based on your code review comments. Also updated the speech sample audio filename in scripts/prepare-testing-project.sh. |
| @@ -44,7 +44,7 @@ echo "Creating pubsub resources." | |||
| gcloud alpha pubsub topics create gae-mvm-pubsub-topic | |||
|
|
|||
| echo "Creating speech resources." | |||
| gsutil cp speech/api/resources/audio.flac gs://$GCLOUD_PROJECT/speech/ | |||
| gsutil cp speech/api-client/resources/audio.raw gs://$GCLOUD_PROJECT/speech/ | |||
There was a problem hiding this comment.
Are there other tests that depend on audio.flac?
There was a problem hiding this comment.
Just searched through the repo and it doesn't look like there are. Also, the audio.flac file doesn't exist in that directory anymore.
There was a problem hiding this comment.
Might want to set -e on the top of this file, so that the build will fail in the future when this happens..
There was a problem hiding this comment.
(doesn't have to be in this CL - just saying it'd probably be a good idea..)
There was a problem hiding this comment.
changed in this PR
| def test_main(resource, capsys): | ||
|
|
||
| # Run the transcribe_async sample on audio.raw, verify correct results | ||
| speech_storage_uri = 'gs://' + os.environ.get('CLOUD_STORAGE_BUCKET') |
There was a problem hiding this comment.
Use the cloud_config or remote_resource fixture.
| @@ -26,12 +26,15 @@ def __init__(self, **kwargs): | |||
| @pytest.fixture(scope='session') | |||
| def cloud_config(): | |||
| """Provides a configuration object as a proxy to environment variables.""" | |||
| speech_storage_uri = 'gs://' + os.environ.get('CLOUD_STORAGE_BUCKET') | |||
There was a problem hiding this comment.
@ryanmats there's already the storage_bucket thing here, there isn't any need to add another one.
…n-docs-samples into speech-transcribe-async-bug
…n-docs-samples into speech-transcribe-async-bug
| def test_main(resource, capsys, cloud_config): | ||
|
|
||
| # Run the transcribe sample on audio.raw, verify correct results | ||
| speech_storage_uri = 'gs://' + cloud_config.storage_bucket |
There was a problem hiding this comment.
Lol okay, I promise I'm not doing this just to annoy you:
Prefer:
storage_uri = 'gs://{}/speech/audio.raw'.format(cloud_config.storage_bucket)(Here and in the other test file).
Fixed bugs for Speech API GRPC sample code: