-
Notifications
You must be signed in to change notification settings - Fork 354
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 function/method enhancements, demo samples #122
Changes from 12 commits
c835503
4e78f6f
fb81497
1bb3c51
9d44556
23c0dc9
e84c679
7a4cde5
ec89cd1
7457ecd
53e89e9
4cb779f
5b162d0
25b8814
833c0fd
ae5c058
2d70605
45d5c7a
fe05ee5
eb1bb2d
3b20252
bd0a9fd
6053297
390a58c
890e749
821adb4
667b49f
de2c3dd
8832966
c4a469d
2a269f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,8 @@ | |
|
||
import proto # type: ignore | ||
|
||
|
||
from google.cloud.aiplatform.v1beta1.schema.predict.instance import text_sentiment_pb2 as gcaspi_text_sentiment # type: ignore | ||
# DO NOT OVERWRITE FOLLOWING LINE: it was manually edited. | ||
from google.cloud.aiplatform.v1beta1.schema.predict.instance import TextSentimentPredictionInstance | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't this be overwritten by the next re-generation? Why is it necessary to change the import here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this replace needs to be made permanent, please do it in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
|
||
__protobuf__ = proto.module( | ||
|
@@ -59,7 +59,7 @@ class Prediction(proto.Message): | |
instance = proto.Field( | ||
proto.MESSAGE, | ||
number=1, | ||
message=gcaspi_text_sentiment.TextSentimentPredictionInstance, | ||
message=TextSentimentPredictionInstance, | ||
) | ||
|
||
prediction = proto.Field(proto.MESSAGE, number=2, message=Prediction,) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
# -*- coding: utf-8 -*- | ||
|
||
# Copyright 2020 Google LLC | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
# | ||
from google.cloud.aiplatform_helpers.value_converter import to_value | ||
from google.cloud.aiplatform_helpers.value_converter import from_value | ||
from google.cloud.aiplatform_helpers.value_converter import from_map | ||
|
||
from proto.marshal import Marshal | ||
from proto.marshal.rules.struct import ValueRule | ||
from google.protobuf.struct_pb2 import Value | ||
|
||
|
||
class ConversionValueRule(ValueRule): | ||
def to_python(self, value, *, absent: bool = None): | ||
return super().to_python(value, absent=absent) | ||
|
||
def to_proto(self, value): | ||
|
||
# Need to check whether value is an instance | ||
# of an enhanced type | ||
if callable(getattr(value, 'to_value', None)): | ||
return value.to_value() | ||
else: | ||
return super().to_proto(value) | ||
|
||
|
||
def add_methods_to_classes_in_package(pkg): | ||
classes = dict([(name, cls) | ||
for name, cls in pkg.__dict__.items() | ||
if isinstance(cls, type)]) | ||
|
||
for class_name, cls in classes.items(): | ||
# Add to_value() method to class with docstring | ||
setattr(cls, 'to_value', to_value) | ||
cls.to_value.__doc__ = to_value.__doc__ | ||
|
||
# Add from_value() method to class with docstring | ||
cls.from_value = add_from_value_to_class(cls) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this one not calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed. I was just trying different methods of assigning members dynamically; forgot to standardize on one technique. |
||
cls.from_value.__doc__ = from_value.__doc__ | ||
|
||
# Add from_map() method to class with docstring | ||
setattr(cls, 'from_map', add_from_map_to_class(cls)) | ||
cls.from_map.__doc__ = from_map.__doc__ | ||
|
||
|
||
def add_from_value_to_class(cls): | ||
def _from_value(value): | ||
return from_value(cls, value) | ||
return _from_value | ||
|
||
|
||
def add_from_map_to_class(cls): | ||
def _from_map(map_): | ||
return from_map(cls, map_) | ||
return _from_map | ||
|
||
|
||
marshal = Marshal(name='google.cloud.aiplatform.v1beta1') | ||
marshal.register(Value, ConversionValueRule(marshal=marshal)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
# Copyright 2020 Google LLC | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# https://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
from __future__ import absolute_import | ||
from google.protobuf.struct_pb2 import Value | ||
from proto.marshal.collections.maps import MapComposite | ||
from proto.marshal import Marshal | ||
from google.protobuf import json_format | ||
from google.protobuf.struct_pb2 import Value | ||
from proto import Message | ||
from proto.message import MessageMeta | ||
|
||
|
||
def to_value(self: Message) -> Value: | ||
"""Converts a message type to a :class:`~google.protobuf.struct_pb2.Value` object. | ||
|
||
Args: | ||
message: the message to convert | ||
|
||
Returns: | ||
the message as a :class:`~google.protobuf.struct_pb2.Value` object | ||
""" | ||
def is_prop(prop): | ||
if prop[0].isupper(): | ||
return False | ||
if prop.startswith('_'): | ||
return False | ||
return True | ||
|
||
props = list(filter(is_prop, dir(self._pb))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like the intention here is to collect all the field names - is there a better to do that than relying on attribute name's first character? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @software-dov Do you have any suggestsions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that was my hack for trying to get around the "int64s as strings" issue. However, playing with the Java library the other day, I think that sending int64 values as strings might not be as big a deal as I originally though. I'm going to switch this to a simple call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This apparently isn't needed! I've removed this code. |
||
|
||
props_dict = {} | ||
for prop in props: | ||
props_dict[prop] = getattr(self._pb, prop) | ||
|
||
return json_format.ParseDict(props_dict, Value()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this work if some of the values of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed this bit. |
||
|
||
|
||
def from_value(cls: MessageMeta, value: Value) -> Message: | ||
"""Creates instance of class from a :class:`~google.protobuf.struct_pb2.Value` object. | ||
|
||
Args: | ||
value: a :class:`~google.protobuf.struct_pb2.Value` object | ||
|
||
Returns: | ||
Instance of class | ||
""" | ||
value_dict = json_format.MessageToDict(value) | ||
return json_format.ParseDict(value_dict, cls()._pb) | ||
|
||
|
||
def from_map(cls: MessageMeta, map_: MapComposite) -> Message: | ||
"""Creates instance of class from a :class:`~proto.marshal.collections.maps.MapComposite` object. | ||
|
||
Args: | ||
map_: a :class:`~proto.marshal.collections.maps.MapComposite` object | ||
|
||
Returns: | ||
Instance of class | ||
""" | ||
map_dict = dict(map_) | ||
marshal = Marshal(name='marshal') | ||
pb = marshal.to_proto(Value, map_) | ||
return from_value(cls, pb) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,8 +14,8 @@ | |
|
||
# [START aiplatform_create_training_pipeline_image_classification_sample] | ||
from google.cloud import aiplatform | ||
from google.protobuf import json_format | ||
from google.protobuf.struct_pb2 import Value | ||
from google.cloud.aiplatform.v1beta1.schema.trainingjob import definition | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it possible to remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not very easily, no. |
||
ModelType = definition.AutoMlImageClassificationInputs().ModelType | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we don't have to create an instance of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
|
||
|
||
def create_training_pipeline_image_classification_sample( | ||
|
@@ -30,18 +30,18 @@ def create_training_pipeline_image_classification_sample( | |
# Initialize client that will be used to create and send requests. | ||
# This client only needs to be created once, and can be reused for multiple requests. | ||
client = aiplatform.gapic.PipelineServiceClient(client_options=client_options) | ||
training_task_inputs_dict = { | ||
"multiLabel": True, | ||
"modelType": "CLOUD", | ||
"budgetMilliNodeHours": 8000, | ||
"disableEarlyStopping": False, | ||
} | ||
training_task_inputs = json_format.ParseDict(training_task_inputs_dict, Value()) | ||
|
||
icn_training_inputs = definition.AutoMlImageClassificationInputs( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that here we introduce an inconsistent style of using instance of a particular python class, whereas the rest of the sample is in python dicts. I think technically we could keep using python dict here too (with only the change of camelCase to snake_case for dict keys). @leahecole let us know if this is fine according to sample style guidelines. Several more samples will be updated and follow the same pattern as the two samples of this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instances are nicer b/c IDEs can offer more assistance with field names and types. Dicts are sometimes easier to pass around though. I don't think we currently mandate one style or the other in the style guide. There is a mix in the currently published samples. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. General consensus amongst the owners was preference to have generated classes for API resources - having spell check and autocomplete as well as knowing where to look in reference docs is helpful If you're using a user-defined object with arbitrary properties, a dict may be simpler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be a place where we need to avoid imposing consistency across the board. For example explicitly construct instances can make certain samples (e.g. https://github.com/googleapis/python-aiplatform/blob/master/samples/snippets/create_hyperparameter_tuning_job_python_package_sample.py) much more difficult to read than dicts, and in some cases we are forced to use instances (e.g. https://github.com/googleapis/python-aiplatform/blob/master/samples/snippets/upload_model_explain_image_managed_container_sample.py) because of dependency issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to clarify: I'm keeping this sample as-is? |
||
multi_label=True, | ||
model_type=ModelType.CLOUD, | ||
budget_milli_node_hours=8000, | ||
disable_early_stopping=False | ||
) | ||
|
||
training_pipeline = { | ||
"display_name": display_name, | ||
"training_task_definition": "gs://google-cloud-aiplatform/schema/trainingjob/definition/automl_image_classification_1.0.0.yaml", | ||
"training_task_inputs": training_task_inputs, | ||
"training_task_inputs": icn_training_inputs.to_value(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I rather prefer not to have additional method calls here. (that is: define a new variable above so that the value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also note that this is simply a style preference with some hidden implication on sample generation. please feel free to leave it as is for sample review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed. |
||
"input_data_config": {"dataset_id": dataset_id}, | ||
"model_to_upload": {"display_name": model_display_name}, | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,9 @@ | |
import base64 | ||
|
||
from google.cloud import aiplatform | ||
from google.protobuf import json_format | ||
from google.protobuf.struct_pb2 import Value | ||
from google.cloud.aiplatform.v1beta1.schema.predict import instance | ||
from google.cloud.aiplatform.v1beta1.schema.predict import params | ||
from google.cloud.aiplatform.v1beta1.schema.predict import prediction | ||
|
||
|
||
def predict_image_classification_sample( | ||
|
@@ -36,25 +37,29 @@ def predict_image_classification_sample( | |
|
||
# The format of each instance should conform to the deployed model's prediction input schema. | ||
encoded_content = base64.b64encode(file_content).decode("utf-8") | ||
instance_dict = {"content": encoded_content} | ||
|
||
instance = json_format.ParseDict(instance_dict, Value()) | ||
instances = [instance] | ||
# See gs://google-cloud-aiplatform/schema/predict/params/image_classification_1.0.0.yaml for the format of the parameters. | ||
parameters_dict = {"confidence_threshold": 0.5, "max_predictions": 5} | ||
parameters = json_format.ParseDict(parameters_dict, Value()) | ||
instance_obj = instance.ImageClassificationPredictionInstance({ | ||
"content": encoded_content}) | ||
|
||
instance_val = instance_obj.to_value() | ||
instances = [instance_val] | ||
|
||
params_obj = params.ImageClassificationPredictionParams({ | ||
"confidence_threshold": 0.5, "max_predictions": 5}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems more common to pass these in as parameters as opposed to a dict, as is done in the other sample of this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
endpoint = client.endpoint_path( | ||
project=project, location=location, endpoint=endpoint_id | ||
) | ||
response = client.predict( | ||
endpoint=endpoint, instances=instances, parameters=parameters | ||
endpoint=endpoint, instances=instances, parameters=params_obj | ||
) | ||
print("response") | ||
print(" deployed_model_id:", response.deployed_model_id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - Is there a reason for the extra space at the beginning of this print statement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had a \t character in there earlier, but it got dropped accidentally. Added it back. |
||
# See gs://google-cloud-aiplatform/schema/predict/prediction/classification.yaml for the format of the predictions. | ||
predictions = response.predictions | ||
for prediction in predictions: | ||
print(" prediction:", dict(prediction)) | ||
for prediction_ in predictions: | ||
prediction_obj = prediction.ClassificationPredictionResult.from_map(prediction_) | ||
print(prediction_obj) | ||
|
||
|
||
# [END aiplatform_predict_image_classification_sample] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,4 +31,4 @@ def test_ucaip_generated_predict_image_classification_sample(capsys): | |
) | ||
|
||
out, _ = capsys.readouterr() | ||
assert 'string_value: "daisy"' in out | ||
assert 'deployed_model_id:' in out | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did this test case change? Is there any chance this could lead to a false positive if no model ID is returned? Or will the sample straight up fail before it gets to this print statement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A couple of reasons, but biggest of them: we want to avoid testing for the output of models, since retraining can cause the predictions to change. No, a model ID must be returned as part of the online prediction--you can't have a prediction without a model! The sample will fail if you attempt to send a prediction request to an endpoint that has no model deployed to it. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
# Copyright 2020 Google LLC | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# https://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
from __future__ import absolute_import | ||
import unittest | ||
|
||
from google.cloud.aiplatform.v1beta1.schema.trainingjob import definition | ||
ModelType = definition.AutoMlImageClassificationInputs().ModelType | ||
|
||
|
||
class EnhancedTypesTests(unittest.TestCase): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other library unit tests are written in the style of plain pytest functions. Unless that is not feasible for what you are testing here, please follow the same convention. Also perhaps add another folder under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CODEOWNERS are by directory, so that is definitely possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both done: moved tests, switched to pytest rather than unittest. |
||
def setUp(self): | ||
self.test_training_input = definition.AutoMlImageClassificationInputs( | ||
multi_label=True, | ||
model_type=ModelType.CLOUD, | ||
budget_milli_node_hours=8000, | ||
disable_early_stopping=False | ||
) | ||
|
||
def test_exposes_to_value_method(self): | ||
assert(hasattr(self.test_training_input, 'to_value')) | ||
|
||
def test_exposes_from_value_method(self): | ||
assert(hasattr(self.test_training_input, 'from_value')) | ||
|
||
def test_exposes_from_map_method(self): | ||
assert(hasattr(self.test_training_input, 'from_map')) |
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 would suggest using a namespace that does not imply public API, since we don't expect the users to use this, right?. perhaps something like
_helpers
instead ofaiplatform_helpers
?Also I think the convention here is to import the module and not individual methods or classes.
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.
That's what the public Google Python style guide says. https://google.github.io/styleguide/pyguide.html#22-imports I don't know if we've followed it consistently in the past, but probably best to adhere to this for new code.
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.
The value_converter.py module is intended to be public. It will be helpful for tabular developers who need to format their prediction instances, for example.
I'll change the name of the methods intended to be private so that they have a leading underscore in their names.
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 see - perhaps
add_methods_to_classes_in_package
should be in a private module, where asvalue_converter
a public module. in that case perhaps a nested submoduleaiplatform.helpers.value_converter
would be preferred overaiplatform_helpers.value_converter
.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.