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

automl beta #1575

Merged
merged 7 commits into from
Jul 20, 2018
Merged

automl beta #1575

merged 7 commits into from
Jul 20, 2018

Conversation

sirtorry
Copy link
Contributor

add automl samples and tests

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 17, 2018
@sirtorry sirtorry changed the title automl initial commit automl beta Jul 17, 2018
@sirtorry sirtorry force-pushed the automl branch 4 times, most recently from 23d0f49 to 8153edf Compare July 19, 2018 21:21
@sirtorry sirtorry requested a review from happyhuman July 19, 2018 21:33
client = automl.AutoMlClient()

# Get the latest state of a long-running operation.
response = client.transport._operations_client.get_operation(
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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!

Copy link
Contributor

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.

Copy link
Contributor Author

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()?

Copy link
Contributor

@happyhuman happyhuman left a 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.
Copy link
Contributor

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.

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

https://cloud.google.com/natural-language/automl/docs/
"""

# [START automl_natural_language_import]
Copy link
Contributor

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.

Copy link
Contributor Author

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]
Copy link
Contributor

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.

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

import argparse
import os

from google.cloud import automl_v1beta1 as automl
Copy link
Contributor

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.

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

Default is False.
"""
client = automl.AutoMlClient()

Copy link
Contributor

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.

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

# Classification type is assigned based on multilabel value.
classification_type = "MULTICLASS"
if multilabel:
classification_type = "MULTILABEL"
Copy link
Contributor

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"

Copy link
Contributor Author

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]
Copy link
Contributor

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.

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

print("\tnanos: {}".format(dataset.create_time.nanos))


# [END automl_natural_language_list_datasets]
Copy link
Contributor

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

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

args = parser.parse_args()

if args.command == "create_dataset":
multilabel = True if args.multilabel == "True" else False
Copy link
Contributor

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")

Copy link
Contributor Author

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
Copy link
Contributor

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")

Copy link
Contributor Author

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.

Copy link
Contributor

@happyhuman happyhuman left a 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.

@sirtorry sirtorry merged commit a918a34 into master Jul 20, 2018
busunkim96 pushed a commit to busunkim96/python-automl that referenced this pull request Aug 17, 2020
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
busunkim96 pushed a commit to googleapis/python-automl that referenced this pull request Aug 17, 2020
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
busunkim96 pushed a commit to googleapis/python-translate that referenced this pull request Sep 1, 2020
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
busunkim96 pushed a commit to googleapis/python-translate that referenced this pull request Sep 1, 2020
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
busunkim96 pushed a commit to googleapis/python-automl that referenced this pull request Sep 15, 2020
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
busunkim96 pushed a commit to googleapis/python-automl that referenced this pull request Sep 15, 2020
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
hkdevandla pushed a commit to hkdevandla/python-language that referenced this pull request Sep 26, 2020
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
busunkim96 pushed a commit to googleapis/python-language that referenced this pull request Sep 29, 2020
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
telpirion pushed a commit that referenced this pull request Nov 16, 2022
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
arbrown pushed a commit that referenced this pull request Nov 17, 2022
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
dandhlee pushed a commit that referenced this pull request Nov 17, 2022
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
m-strzelczyk pushed a commit that referenced this pull request Nov 18, 2022
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
dandhlee pushed a commit that referenced this pull request Nov 18, 2022
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
parthea pushed a commit to googleapis/google-cloud-python that referenced this pull request Jul 6, 2023
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
parthea pushed a commit to googleapis/google-cloud-python that referenced this pull request Oct 21, 2023
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
parthea pushed a commit to googleapis/google-cloud-python that referenced this pull request Oct 21, 2023
* automl initial commit

* lint

* fix import groupings

* add requirements.txt

* address review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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