[Syntax Review] Additional types of SampleGen code samples by beccasaurus · Pull Request #9970 · googleapis/google-cloud-python

beccasaurus · 2019-12-12T21:56:10Z

Follow-up code review PR –

To evaluate the readiness of 🐍 Python's GAPIC sample generator for use on cloud.google.com,
this provides a full set of samples which demonstrate the basic features of generated samples.

The .yaml sample config files for these samples can all be found here: .yaml sample configs

The first review Pull Request contained samples to show the basic structure as well as LRO samples.

This contains samples demonstrating those features and a few more use-cases for review –

Sample	Description
`samplegen_basics.py`	Shows basic structure/anatomy of Python samples
`samplegen_no_config.py`	Minimal sample example (zero configuration)
`samplegen_no_response.py`	API method which returns Empty
`samplegen_paged.py`	Paged API method (iterates over pages of results)
`samplegen_resource_path.py`	Sample which requires user's Google Project ID
`samplegen_map_field_access.py`	Access map value by key and enumerate keys/values
`samplegen_read_and_write_files.py`	Read binary file into bytes & write string to local file
`samplegen_repeated_fields.py`	Repeated request field & show loop/access by index
`samplegen_write_bytes_to_file.py`	Write bytes field in response to local file
`samplegen_lro.py`	Call long-running operation API endpoint

This Pull Request is focused on feedback collection & a similar PR is being sent to all languages.

Once you've completed your review and left comments, you may feel free to close the PR!

We will scope out making all of the requested changes and reach out to language team offline.

🙏 Thanks in advance for your review!

P.S. Nits welcome! We'd like to get these as polished as possible for usage on cloud.google.com.

Please feel free to note in comments whether something is: 🛑 Blocker -vs- 📌 Nit

🔍 Known Issues

Should import libraries w/o version suffix, e.g. from google.cloud import speech_v1 as speech
Docstring is missing : and is missing type, it should be var_name (Type): description here
❓ Open Question: Is the preference to show the method definition or only show method body?

📑 (internal)

…omment

beccasaurus · 2019-12-12T21:56:18Z

@busunkim96 Follow-up PR, as promised!

This shows additional syntax which previous PRs didn't include, so y'all can see all of the main snippets used to generate these Python samples. Please feel free to leave nits and blockers alike!

I mentioned some carry-over comments from the previous PR in the description above.

beccasaurus · 2019-12-12T22:04:40Z

+#   description: This is the sample description
+#   usage: python3 samples/v1/samplegen_basics.py [--display_name "This is the default value of the display_name request field"]
+
+# [START samplegen_basics]


@tswast You mentioned a preference for not showing the method definition in the embedded code? (START/END)

Do you know if that's the standard nowadays? I've seen both types (with and without method signature).

No opinion on my end! The template for samples can be updated to whatever is standard! 😄

Does anyone have a canonical example? e.g. for Node.js they have these canonical examples

I'd love to update the structure if that's what's needed. Would it be a blocker if we show the method definition?

/cc @busunkim96 – This is the biggest piece of feedback from the previous PR ^

Python never settled on the canonical format, but there are a few of us working towards doing so (the original PR was currently here, and we have an AI on cleaning it up and creating a sample style guide).

Personally, I have a slight preference to leaving the function signature in (since it would make it easier for me to copy/paste into existing code).

Thanks for that context @kurtisvg!

I'm not biased in either direction, iirc we didn't originally have the method signature and then we added it to the generator template.

My preference: "leave the function signature in" +1

Happy to change this at any time!

I would find it most helpful if we can say that it's OK to launch in Q1 with the method signature and we can change after (ie. let's not block on this one item)

We should block on things like fixing the docstring format, though. Which we'll fix!

+1 - this is a nit for me.

gguuss

A few nits :)

beccasaurus · 2019-12-18T21:47:20Z

Sign in to view

+    language = "en"
+
+    # The text content to analyze
+    content = "Googleplex is at 1600 Amphitheatre Parkway, Mountain View, CA."


As we've settled on blacken, I think the double quotes have to stay.

I'm gonna resolve (hide) this thread. Adding a new comment will re-open, so feel free to follow-up with additional/different thoughts!

gguuss · 2019-12-12T22:23:40Z

+
+# [START samplegen_read_and_write_files]
+from google.cloud import speech_v1p1beta1
+from google.cloud.speech_v1p1beta1 import enums


I prefer importing modules over members. Maybe just have the following

from google.cloud import speech_v1p1beta1

And later using speech_v1p1beta1.enums... instead of aliasing enums globally.

This helps to avoid alias collisions should people be importing multiple client libraries.

Ack. Thanks for pointing this out!

If I remember correctly, some LOC can be very long with <module>.<enums>. for each enum. If the generator were smart enough, it could decide when [not] to import enums.

@gguuss @leahecole et al

Does this fall into nit or blocker category?

And are we sure that this change should be made?

I've seen a preference for importing enums – it's a style I've seen in a bunch of code samples. In python-docs-samples, you can find the import enums style used for all of these 10 APIs:

Asset

BigTable

KMS

Natural Language

Speech-to-Text

AutoML Tables

Text-to-Speech

Translation

Video Intelligence

Vision

Search for "import enums"

beccasaurus · 2019-12-18T21:53:33Z

Sign in to view

+# [END samplegen_read_and_write_files]
+
+
+def main():


Hiding this thread in favor of this one above (duplicate)

busunkim96 · 2019-12-12T22:26:26Z

CC/ @dmahugh @leahecole @kurtisvg @kurtisvg @crwilcox

beccasaurus · 2019-12-13T18:52:03Z

@@ -0,0 +1,49 @@
+# -*- coding: utf-8 -*-


ℹ️ Note: this "no config" sample is different from the rest. Rather than demonstrating a feature, it shows what a sample looks like when not configured at all.

These samples could be generated for every API method without any manual configuration.

Review of the "no config" sample is not required, this is more of an FYI :)

kurtisvg

Another topic of discussion (if it hasn't already been brought up) is that it would be helpful if Python3 style type hints were included (especially for client responses)

beccasaurus · 2019-12-18T21:55:00Z

Sign in to view

@@ -0,0 +1,69 @@
+# -*- coding: utf-8 -*-


As this will fix itself with the transition to the µgenerator, I'm hiding this thread.

It's a good catch! But it comes from the common template which the monolithic generator uses for all Python files, not just code samples.

kurtisvg · 2019-12-18T22:07:27Z

Sign in to view

+
+    client = language_v1.LanguageServiceClient()
+
+    type_ = enums.Document.Type.PLAIN_TEXT


I'm okay with manually adjusting for now - presumable these auto-gen samples will still get reviewed like normal ones, so we can encourage it that way if it's listed in the sample guidelines.

@busunkim96 Any idea if the Python documentation includes the available enum values with descriptions?

This is googleapis/artman#579 / #7503

I wonder... since the generators have the .proto, maybe including the list in the samples is the right way to go. We automatically add a comment which lists the available options. No URL required (but great if possible to construct).

@software-dov Thoughts on grabbing the enum display names in the sample generator and automatically including them?

This is language agnostic but seems to be one of the most popular pieces of feedback! 🎉

beccasaurus · 2019-12-18T22:00:53Z

Sign in to view

+    response = client.analyze_entities(document)
+    for entity in response.entities:
+        # Access value by key:
+        print(u"URL: {}".format(entity.metadata["wikipedia_url"]))


☹️

How about we do this?

Remove u""

Don't use f"" (yet)

Sign in to view

+            print(u"{}: {}".format(key, value))
+
+        # Loop over just keys:
+        for the_key, _ in entity.metadata.items():


software-dov · 2019-12-17T19:07:57Z

Sign in to view

+            print(u"Key: {}".format(the_key))
+
+        # Loop over just values:
+        for _, the_value in entity.metadata.items():


Or better yet,

Suggested change

for _, the_value in entity.metadata.items():

for value in entity.metadata.values():

GOOD CATCH 🎣

Will update to .values()

kurtisvg · 2019-12-16T19:00:43Z

+    # A list of strings containing words and phrases "hints"
+    phrases_element = "Fox in socks"
+    phrases_element_2 = "Knox in box"
+    phrases = [phrases_element, phrases_element_2]


nit: Can we just inline these phrase_elements? They only appear to be used in the array, so attaching a variable name seems unnecessary.

Glad you brought this up, I'd be surprised if you hadn't. These variables are pretty gross 😔

kurtisvg · 2019-12-16T19:03:44Z

+    speech_contexts_element = {"phrases": phrases}
+    speech_contexts = [speech_contexts_element]
+    config = {"encoding": encoding, "speech_contexts": speech_contexts}
+    uri = "gs://[BUCKET]/[FILENAME]"


nit: Can we use terms like "gs://[YOUR_BUCKET]/[YOUR_FILENAME]" to make crystal clear that it is up to the user to provide these values?

+1 We could make that a convention for generated sample authors.

In this case, this whole string simply comes from the config:

# Define request message. request: - field: audio.uri value: "gs://[BUCKET]/[FILENAME]" - field: config.encoding value: MP3 - field: config.speech_contexts[0].phrases[0] value: "Fox in socks" comment: 'A list of strings containing words and phrases "hints"' - field: config.speech_contexts[0].phrases[1] value: "Knox in box"

kurtisvg · 2019-12-16T19:07:00Z

+def sample_analyze_entities():
+    """This sample reads and loops over a map field in the response"""
+
+    client = language_v1.LanguageServiceClient()


Can we add a comment to clarify the role of the client? Some clients generate background resources and should only be created once and used multiple times, while others are just wrappers around HTTP calls. I'm not sure which this client falls under, but I think it would be useful to signal this to users.

All of the generated clients are just wrappers around gRPC/HTTP calls.

Can we add a comment to clarify the role of the client?

Yes!

In lots of languages, there is a 1 line comment above this line. Usually simply:

# Initialize client client = new library.SomeServiceClient()

But at least it calls attention do it!

TAL at the layout Node.js uses for inspiration (Node PR)

Just look at the comments, I like how it organizes/structures the sample:

function main(displayName = 'This is the default value of the display_name request field') { // [START samplegen_basics] /** * TODO(developer): Uncomment these variables before running the sample. */ // const displayName = 'This is the default value of the display_name request field'; // Imports the client library const {ProductSearchClient} = require('@google-cloud/vision').v1; // Instantiates a client const productSearchClient = new ProductSearchClient(); async function sampleCreateProductSet() { // Construct request const request = { parent: formattedParent, productSet: productSet, }; // Run request const [response] = await productSearchClient.createProductSet(request); // The API response represents the created product set const productSet = response; console.log(`The full name of the created product set: ${productSet.name}`); } sampleCreateProductSet(); // [END samplegen_basics] }

kurtisvg · 2019-12-16T19:18:38Z

+#   description: This is the sample description
+#   usage: python3 samples/v1/samplegen_basics.py [--display_name "This is the default value of the display_name request field"]
+
+# [START samplegen_basics]


Python never settled on the canonical format, but there are a few of us working towards doing so (the original PR was currently here, and we have an AI on cleaning it up and creating a sample style guide).

Personally, I have a slight preference to leaving the function signature in (since it would make it easier for me to copy/paste into existing code).

kurtisvg · 2019-12-16T19:20:29Z

+
+    client = vision_v1.ProductSearchClient()
+
+    # project = '[PROJECT ID]'


Instead of commenting these out, can we use default values the user can replace?

This way they get compile checked.

def sample_create_product_set(project="YOUR_DISPLAY_NAME"):

I'm ok with this because it's how we do it in node and I'm used to it, but I'd love to hear from more folks as a tie breaker (do we leave it uncommented in java?)

For both Go and Java we switch from commenting them out to using an overriding function to provide defaults. The issue with commented out variables is that there isn't a way to check them vs the compiler, so they often could get changed or updated without anyone noticing.

Same exact thing in both Ruby and C# as well 😄

We're switching from comments to using method parameter default values.

Love it +💯

Sign in to view

+    language = "en"
+
+    # The text content to analyze
+    content = "Googleplex is at 1600 Amphitheatre Parkway, Mountain View, CA."


Sign in to view

+    language = "en"
+
+    # The text content to analyze
+    content = "Googleplex is at 1600 Amphitheatre Parkway, Mountain View, CA."


leahecole · 2019-12-16T21:40:49Z

+    client = texttospeech_v1.TextToSpeechClient()
+
+    text = "Hello, world!"
+    input_ = {"text": text}


Should we instead name this "speech_input" or "sample_input" to avoid having to use the underscore notation? that syntax may not be familiar to folks

Good question. Not being a Pythonista, myself, I was originally confused by these.

Would it be configurable or... whenever a keyword is used, instead of using a _ suffix, use a prefix instead which scopes the variable to the sample so it's safely not a keyword?

Ack +1

Maybe request_input? Trying to think of a prefix that'll work and make sense in as many cases as possible

leahecole · 2019-12-16T21:43:12Z

+
+    client = vision_v1.ProductSearchClient()
+
+    # project = '[PROJECT ID]'


I'm ok with this because it's how we do it in node and I'm used to it, but I'd love to hear from more folks as a tie breaker (do we leave it uncommented in java?)

software-dov · 2019-12-17T19:05:19Z

+        for key, value in entity.metadata.items():
+            print(u"{}: {}".format(key, value))
+
+        # Loop over just keys:


Can you elaborate?

software-dov · 2019-12-17T19:07:30Z

+            print(u"{}: {}".format(key, value))
+
+        # Loop over just keys:
+        for the_key, _ in entity.metadata.items():


Or better yet,

Suggested change

for the_key, _ in entity.metadata.items():

for key in entity.metadata.keys():

Will fix both the .keys() and .values() loops!

software-dov · 2019-12-17T19:07:57Z

+            print(u"Key: {}".format(the_key))
+
+        # Loop over just values:
+        for _, the_value in entity.metadata.items():


Or better yet,

Suggested change

for _, the_value in entity.metadata.items():

for value in entity.metadata.values():

Rebecca Taylor added 5 commits December 9, 2019 10:25

[SampleGen] New sample types for review

b7dc356

[SampleGen] Demonstrate accessing map via accessor rather than 'map'

81a0716

[SampleGen] Update description/title of sample showing read/write files

f6c7406

[SampleGen] Update no response sample to print after response

6e244a3

[SampleGen] Update basic example to include a parameter description c…

10048b0

…omment

beccasaurus requested a review from busunkim96 as a code owner December 12, 2019 21:56

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 12, 2019

beccasaurus mentioned this pull request Dec 12, 2019

🔍 Examples of Generated Samples for Code Review [DO NOT MERGE] #9514

Closed

beccasaurus commented Dec 12, 2019

View reviewed changes

gguuss suggested changes Dec 12, 2019

View reviewed changes

beccasaurus commented Dec 13, 2019

View reviewed changes

kurtisvg suggested changes Dec 16, 2019

View reviewed changes

leahecole reviewed Dec 16, 2019

View reviewed changes

software-dov reviewed Dec 17, 2019

View reviewed changes

busunkim96 closed this Jul 10, 2020


		client = language_v1.LanguageServiceClient()

		type_ = enums.Document.Type.PLAIN_TEXT

	for _, the_value in entity.metadata.items():
	for value in entity.metadata.values():


		client = vision_v1.ProductSearchClient()

		# project = '[PROJECT ID]'

	for the_key, _ in entity.metadata.items():
	for key in entity.metadata.keys():

Conversation

beccasaurus commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🙏 Thanks in advance for your review!

🔍 Known Issues

Uh oh!

beccasaurus commented Dec 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gguuss left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gguuss Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

busunkim96 commented Dec 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurtisvg left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beccasaurus commented Dec 12, 2019 •

edited

Loading

gguuss Dec 12, 2019 •

edited

Loading

beccasaurus Dec 18, 2019 •

edited

Loading

beccasaurus Dec 18, 2019 •

edited

Loading