-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Add small, generated version of language_sentiment_text
#1660
Conversation
FYI generated from the following YAML GAPIC config: sample_value_sets: - id: analyze_sentiment title: "Analyzing Sentiment" description: "Proof of concept for analyzing sentiment" parameters: defaults: - document.type=PLAIN_TEXT - document.content="Your text to analyze, e.g. Hello, world!" attributes: - parameter: document.content sample_argument: true on_success: - define: sentiment=$resp.document_sentiment - print: - "Score: %s" - sentiment.score - print: - "Magnitude: %s" - sentiment.magnitude samples: standalone: - calling_forms: ".*" value_sets: analyze_sentiment region_tag: language_sentiment_text
Move language_python_migration_document_text so it uses a different snippet in preparation for deprecation of existing language_sentiment_text sample
@@ -39,12 +39,10 @@ def sentiment_text(text): | |||
text = text.decode('utf-8') | |||
|
|||
# Instantiates a plain text document. | |||
# [START language_python_migration_document_text] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hand-written version of the snippet will be deprecated later.
In preparation, I moved this region tag to a different snippet (below) so it'll be safe to delete this hand-written snippet.
…n sample template)
|
||
# [START language_sentiment_text] | ||
|
||
from google.cloud import language_v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be from google.cloud import language_v1 as language
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we also trying to move the imports to inside the code snippet and inside the method?
Not sure what's possible with the auto-gen tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we also trying to move the imports to inside the code snippet and inside the method
Either! There's a current project to decide on this moving forward. But, that aside, we should prefer imports > no imports. We can put them into the method or outside of the method.
Why do some of the current Python samples have the import
s inside the method?
- because the hand-written snippets are usually in a file alongside many other snippets.py
- we generate snippets in their own files
- this means that we can more easily show imports!
- this ALSO means that we can do this for COMPILED languages as well now!!! 🤘
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update this to as language
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dizcology Is this a blocker or can we revisit after this sample?
# [START language_sentiment_text] | ||
|
||
from google.cloud import language_v1 | ||
from google.cloud.language_v1 import enums |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to not import this separately, but use it as language.enums
in the code?
|
||
def main(): | ||
# FIXME: Convert argv from strings to the correct types. | ||
sample_analyze_sentiment(*sys.argv[1:]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is okay, but if we are going to keep the command line interface (probably not), it's still better to use argparse if possible.
|
||
|
||
def sample_analyze_sentiment(content): | ||
# [START language_sentiment_text_core] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen this before, but are the 2 region tags necessary or which one would we use in docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right! These are here for API Explorer, as it was described to me. I'll look into removing this – it doesn't display for users :)
# content = 'Your text to analyze, e.g. Hello, world!' | ||
|
||
type_ = enums.Document.Type.PLAIN_TEXT | ||
document = {'type': type_, 'content': content} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to get the tool to use the helper method here?
document = types.Document(
content=text,
type=enums.Document.Type.PLAIN_TEXT)
All in all, pretty cool and exciting! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Excited to see this continually developed!!! :)
* Generated sample: language_sentiment_text FYI generated from the following YAML GAPIC config: sample_value_sets: - id: analyze_sentiment title: "Analyzing Sentiment" description: "Proof of concept for analyzing sentiment" parameters: defaults: - document.type=PLAIN_TEXT - document.content="Your text to analyze, e.g. Hello, world!" attributes: - parameter: document.content sample_argument: true on_success: - define: sentiment=$resp.document_sentiment - print: - "Score: %s" - sentiment.score - print: - "Magnitude: %s" - sentiment.magnitude samples: standalone: - calling_forms: ".*" value_sets: analyze_sentiment region_tag: language_sentiment_text * Add requirements.txt (not currently generated) * Add test for language_sentiment_text (not currently generated) * Move language_python_migration_document_text Move language_python_migration_document_text so it uses a different snippet in preparation for deprecation of existing language_sentiment_text sample * Rename generated snippets so filename == region tag * Fix test for generated code sample (file rename to match region tag) * Update Copyright year to 2018 in new hand-written file * Fix lint errors of #language_sentiment_text test * Regenerate #language_sentiment_text to fix lint errors (updated Python sample template) * Binary string support in samples! From PR googleapis/gapic-generator#2272
…oogleCloudPlatform/python-docs-samples#1660) * Generated sample: language_sentiment_text FYI generated from the following YAML GAPIC config: sample_value_sets: - id: analyze_sentiment title: "Analyzing Sentiment" description: "Proof of concept for analyzing sentiment" parameters: defaults: - document.type=PLAIN_TEXT - document.content="Your text to analyze, e.g. Hello, world!" attributes: - parameter: document.content sample_argument: true on_success: - define: sentiment=$resp.document_sentiment - print: - "Score: %s" - sentiment.score - print: - "Magnitude: %s" - sentiment.magnitude samples: standalone: - calling_forms: ".*" value_sets: analyze_sentiment region_tag: language_sentiment_text * Add requirements.txt (not currently generated) * Add test for language_sentiment_text (not currently generated) * Move language_python_migration_document_text Move language_python_migration_document_text so it uses a different snippet in preparation for deprecation of existing language_sentiment_text sample * Rename generated snippets so filename == region tag * Fix test for generated code sample (file rename to match region tag) * Update Copyright year to 2018 in new hand-written file * Fix lint errors of #language_sentiment_text test * Regenerate #language_sentiment_text to fix lint errors (updated Python sample template) * Binary string support in samples! From PR googleapis/gapic-generator#2272
…oogleCloudPlatform/python-docs-samples#1660) * Generated sample: language_sentiment_text FYI generated from the following YAML GAPIC config: sample_value_sets: - id: analyze_sentiment title: "Analyzing Sentiment" description: "Proof of concept for analyzing sentiment" parameters: defaults: - document.type=PLAIN_TEXT - document.content="Your text to analyze, e.g. Hello, world!" attributes: - parameter: document.content sample_argument: true on_success: - define: sentiment=$resp.document_sentiment - print: - "Score: %s" - sentiment.score - print: - "Magnitude: %s" - sentiment.magnitude samples: standalone: - calling_forms: ".*" value_sets: analyze_sentiment region_tag: language_sentiment_text * Add requirements.txt (not currently generated) * Add test for language_sentiment_text (not currently generated) * Move language_python_migration_document_text Move language_python_migration_document_text so it uses a different snippet in preparation for deprecation of existing language_sentiment_text sample * Rename generated snippets so filename == region tag * Fix test for generated code sample (file rename to match region tag) * Update Copyright year to 2018 in new hand-written file * Fix lint errors of #language_sentiment_text test * Regenerate #language_sentiment_text to fix lint errors (updated Python sample template) * Binary string support in samples! From PR googleapis/gapic-generator#2272
…oogleCloudPlatform/python-docs-samples#1660) * Generated sample: language_sentiment_text FYI generated from the following YAML GAPIC config: sample_value_sets: - id: analyze_sentiment title: "Analyzing Sentiment" description: "Proof of concept for analyzing sentiment" parameters: defaults: - document.type=PLAIN_TEXT - document.content="Your text to analyze, e.g. Hello, world!" attributes: - parameter: document.content sample_argument: true on_success: - define: sentiment=$resp.document_sentiment - print: - "Score: %s" - sentiment.score - print: - "Magnitude: %s" - sentiment.magnitude samples: standalone: - calling_forms: ".*" value_sets: analyze_sentiment region_tag: language_sentiment_text * Add requirements.txt (not currently generated) * Add test for language_sentiment_text (not currently generated) * Move language_python_migration_document_text Move language_python_migration_document_text so it uses a different snippet in preparation for deprecation of existing language_sentiment_text sample * Rename generated snippets so filename == region tag * Fix test for generated code sample (file rename to match region tag) * Update Copyright year to 2018 in new hand-written file * Fix lint errors of #language_sentiment_text test * Regenerate #language_sentiment_text to fix lint errors (updated Python sample template) * Binary string support in samples! From PR googleapis/gapic-generator#2272
…1660) * Generated sample: language_sentiment_text FYI generated from the following YAML GAPIC config: sample_value_sets: - id: analyze_sentiment title: "Analyzing Sentiment" description: "Proof of concept for analyzing sentiment" parameters: defaults: - document.type=PLAIN_TEXT - document.content="Your text to analyze, e.g. Hello, world!" attributes: - parameter: document.content sample_argument: true on_success: - define: sentiment=$resp.document_sentiment - print: - "Score: %s" - sentiment.score - print: - "Magnitude: %s" - sentiment.magnitude samples: standalone: - calling_forms: ".*" value_sets: analyze_sentiment region_tag: language_sentiment_text * Add requirements.txt (not currently generated) * Add test for language_sentiment_text (not currently generated) * Move language_python_migration_document_text Move language_python_migration_document_text so it uses a different snippet in preparation for deprecation of existing language_sentiment_text sample * Rename generated snippets so filename == region tag * Fix test for generated code sample (file rename to match region tag) * Update Copyright year to 2018 in new hand-written file * Fix lint errors of #language_sentiment_text test * Regenerate #language_sentiment_text to fix lint errors (updated Python sample template) * Binary string support in samples! From PR googleapis/gapic-generator#2272
…oogleCloudPlatform/python-docs-samples#1660) * Generated sample: language_sentiment_text FYI generated from the following YAML GAPIC config: sample_value_sets: - id: analyze_sentiment title: "Analyzing Sentiment" description: "Proof of concept for analyzing sentiment" parameters: defaults: - document.type=PLAIN_TEXT - document.content="Your text to analyze, e.g. Hello, world!" attributes: - parameter: document.content sample_argument: true on_success: - define: sentiment=$resp.document_sentiment - print: - "Score: %s" - sentiment.score - print: - "Magnitude: %s" - sentiment.magnitude samples: standalone: - calling_forms: ".*" value_sets: analyze_sentiment region_tag: language_sentiment_text * Add requirements.txt (not currently generated) * Add test for language_sentiment_text (not currently generated) * Move language_python_migration_document_text Move language_python_migration_document_text so it uses a different snippet in preparation for deprecation of existing language_sentiment_text sample * Rename generated snippets so filename == region tag * Fix test for generated code sample (file rename to match region tag) * Update Copyright year to 2018 in new hand-written file * Fix lint errors of #language_sentiment_text test * Regenerate #language_sentiment_text to fix lint errors (updated Python sample template) * Binary string support in samples! From PR googleapis/gapic-generator#2272
#language_sentiment_text
This Pull Requests adds our first snippet which has been generated using gapic-generator
Comparison (before & after)
The current, hand-written version of this snippet (
language/cloud-client/v1/snippets.py
) ::The new, generated snippet (
language/generated-samples/v1/analyze_sentiment.py
) ::Note: I am continuing with the
:product/:sample_group/:version/:file.py
sample location. Today, most products have a:product/cloud-client/
directory. For the ML APIs, these samples can hopefully all be converted to generated samples and will live in:product/generated-samples/:version/:region_tag.py
(for now, future location tbd)✅ Improvements:
🚫 Regressions:
type_
is an icky edge-case when currently using certain keywords with the generator. We'll look into correcting this. PLEASE feel free to mark this as a required change to block the sample on this and we'll prioritize looking into it (might not be a trivial change to make)"""description"""
missing – we will add this back, PLEASE feel free to mark this as a required change to block the sample on this (TRIVIAL change to make)📝 Sample Source (YAML)
FYI here is the GAPIC configuration for generating the above snippet ::
This will eventually be committed to
googleapis/googleapis
but at a later date📖 Instructions for generating this sample
🚧 TODO (later!) 🚧
Follow-up PRs with additional generated samples will look at adding instructions to show how y'all can generate these samples. The development setup is very non-trivial so I'm hoping we can avoid this being a blocker for adding this sample 😇
🤔 What about this sample supporting Migration Docs?
The existing
language_sentiment_text
sample is used in the Migrating to Python Client Library v0.26.1 pageHere are the parts used (part of existing snippet below) ::
The migration regions which are being used to take into account:
language_python_migration_document_text
language_python_migration_sentiment_text
📝Note about README and CLI
main()
method is included but please do not treat this as a requirement↪️ Steps