-
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
automl beta #1575
automl beta #1575
Conversation
23d0f49
to
8153edf
Compare
client = automl.AutoMlClient() | ||
|
||
# Get the latest state of a long-running operation. | ||
response = client.transport._operations_client.get_operation( |
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 you explain why you need to be able to get the operation status? If you think there is a legitimate use case for this we may want to consider adding a method on the client object for this, since you are relying on an internal object here.
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.
these training jobs can be long running, up to a day.
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.
Alright. I am going to create an issue on the library for us to consider this as a change. Thanks!
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 am not 100% sure but I think response.operation.refresh() will give the current status.
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.
what would response be in response.operation.refresh()?
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 have only reviewed automl_natural_language_dataset.py so far. Most of my comments are applicable to all sample functions and in other python files too, so I avoided to duplicate them. I will review other files soon.
@@ -0,0 +1,300 @@ | |||
#!/usr/bin/env python | |||
|
|||
# Copyright 2018 Google Inc. All Rights Reserved. |
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 was told we are supposed to start using LLC instead of Inc from now on.
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.
done
https://cloud.google.com/natural-language/automl/docs/ | ||
""" | ||
|
||
# [START automl_natural_language_import] |
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.
Do we need region tags for imports too? I did not know.
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.
removed
# [END automl_natural_language_import] | ||
|
||
|
||
# [START automl_natural_language_create_dataset] |
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.
Tim Swast advised me to put the region tags inside the function body. He told me this is the new standard.
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.
done
import argparse | ||
import os | ||
|
||
from google.cloud import automl_v1beta1 as automl |
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 believe for samples this import should be inside the function body. That way then the code snippet is selected using the region tags, the imports will also be included in the selected snippet.
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.
done
Default is False. | ||
""" | ||
client = automl.AutoMlClient() | ||
|
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.
You need these comment lines here (or something like this):
# TODO(developer): Uncomment and set to a path to your audio file.
# project_id = 'PROJECT_ID_HERE'
# compute_region = 'COMPUTE_REGION_HERE'
# dataset_name = 'DATASET_NAME_HERE'
The reason is that once the function body is selected to be shown in some web page using the region tags, these comments tell the user what these parameters should be.
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.
done
# Classification type is assigned based on multilabel value. | ||
classification_type = "MULTICLASS" | ||
if multilabel: | ||
classification_type = "MULTILABEL" |
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.
Nothing wrong with the lines above, a more compact way is of course:
classification_type = "MULTILABEL" if multilabel else "MULTICLASS"
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.
doesn't really affect snippet quality or functionality. won't fix.
# [END automl_natural_language_create_dataset] | ||
|
||
|
||
# [START automl_natural_language_list_datasets] |
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.
Same as above comments. Region tag should be inside the function body.
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.
done
print("\tnanos: {}".format(dataset.create_time.nanos)) | ||
|
||
|
||
# [END automl_natural_language_list_datasets] |
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 too should be inside the function
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.
done
args = parser.parse_args() | ||
|
||
if args.command == "create_dataset": | ||
multilabel = True if args.multilabel == "True" else False |
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 line can be simplified to:
multilabel = (args.multilabel == "True")
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.
doesn't affect snippet quality or functionality. won't fix.
args = parser.parse_args() | ||
|
||
if args.command == "create_dataset": | ||
multilabel = True if args.multilabel == "True" else False |
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 line can be simplified to:
multilabel = (args.multilabel == "True")
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.
doesn't affect snippet quality or functionality. won't fix.
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 quite a large PR. It will be great to have smaller PR's in the future. I will LGTM it, but it will be a good idea to review it in more details later on.
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
* automl initial commit * lint * fix import groupings * add requirements.txt * address review comments
add automl samples and tests