-
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 all 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 |
---|---|---|
@@ -0,0 +1,3 @@ | ||
from google.cloud.aiplatform.helpers import value_converter | ||
|
||
__all__ = (value_converter,) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
# 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 __future__ import absolute_import | ||
from google.cloud.aiplatform.helpers import value_converter | ||
|
||
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", value_converter.to_value) | ||
cls.to_value.__doc__ = value_converter.to_value.__doc__ | ||
|
||
# Add from_value() method to class with docstring | ||
setattr(cls, "from_value", _add_from_value_to_class(cls)) | ||
cls.from_value.__doc__ = value_converter.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__ = value_converter.from_map.__doc__ | ||
|
||
|
||
def _add_from_value_to_class(cls): | ||
def _from_value(value): | ||
return value_converter.from_value(cls, value) | ||
|
||
return _from_value | ||
|
||
|
||
def _add_from_map_to_class(cls): | ||
def _from_map(map_): | ||
return value_converter.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,60 @@ | ||
# 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 google.protobuf import json_format | ||
from proto.marshal.collections.maps import MapComposite | ||
from proto.marshal import Marshal | ||
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 | ||
""" | ||
tmp_dict = json_format.MessageToDict(self._pb) | ||
return json_format.ParseDict(tmp_dict, Value()) | ||
|
||
|
||
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 | ||
""" | ||
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,13 +30,14 @@ 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_task_inputs = icn_training_inputs.to_value() | ||
|
||
training_pipeline = { | ||
"display_name": display_name, | ||
|
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. |
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.
PSA: If the proto comments are formatted correctly but the docstrings are getting generated in a weird state please file bugs on the generator repo. https://github.com/googleapis/gapic-generator-python