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: adds media translation samples #3117

Merged
merged 23 commits into from
Mar 31, 2020
Merged

feat: adds media translation samples #3117

merged 23 commits into from
Mar 31, 2020

Conversation

telpirion
Copy link
Contributor

This pull request adds the following code snippet region tags:

  • media_translation_translate_from_file
  • media_translation_translate_from_mic

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 18, 2020

config = mediatranslation.StreamingTranslateSpeechConfig(
audio_config=mediatranslation.TranslateSpeechConfig(
audio_encoding='linear16',
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged. I will raise that issue with product engineering.

Comment on lines 74 to 80
if __name__ == '__main__':
parser = argparse.ArgumentParser(
description=__doc__,
formatter_class=argparse.RawDescriptionHelpFormatter)
parser.add_argument('stream', help='File to stream to the API')
args = parser.parse_args()
translate_from_file(args.stream)
Copy link
Contributor

@nnegrey nnegrey Mar 23, 2020

Choose a reason for hiding this comment

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

Suggested change
if __name__ == '__main__':
parser = argparse.ArgumentParser(
description=__doc__,
formatter_class=argparse.RawDescriptionHelpFormatter)
parser.add_argument('stream', help='File to stream to the API')
args = parser.parse_args()
translate_from_file(args.stream)
# [END media_translation_translate_from_file]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

os.path.join(RESOURCES, 'audio.raw'))
out, err = capsys.readouterr()

assert re.search(r'Partial translation', out, re.DOTALL | re.I)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine, you can also just do : assert 'Partial translation' in out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know!

@telpirion telpirion marked this pull request as ready for review March 25, 2020 16:11
@telpirion telpirion requested a review from a team as a code owner March 25, 2020 16:11
Copy link
Contributor

@dmahugh dmahugh left a comment

Choose a reason for hiding this comment

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

LGTM

@nnegrey
Copy link
Contributor

nnegrey commented Mar 27, 2020

Blocked by: #3116


client = mediatranslation.SpeechTranslationServiceClient()

speech_config = mediatranslation.TranslateSpeechConfig(
Copy link
Contributor

@nnegrey nnegrey Mar 30, 2020

Choose a reason for hiding this comment

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

Nit: so up to you, personally I like to have the variables names match the argument they'll be used in later, but the object name is TranslateSpeechConfig

Suggested change
speech_config = mediatranslation.TranslateSpeechConfig(
audio_config = mediatranslation.TranslateSpeechConfig(

Same thing for config below --> streaming_config, but again object name isn't the same.

Copy link
Contributor

@nnegrey nnegrey left a comment

Choose a reason for hiding this comment

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

LGTM (once README.rst is added)

Note: I still don't like that the audio_encoding is a string, but that is outside the scope here.

@nnegrey nnegrey added the automerge Merge the pull request once unit tests and other checks pass. label Mar 31, 2020
@telpirion telpirion merged commit 7bc4783 into GoogleCloudPlatform:master Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants