feat: adds function/method enhancements, demo samples #122

telpirion · 2020-12-06T22:26:58Z

This PR does the following:

adds the to_value(), from_value(), and from_map() utility functions
assigns the utility functions to enhanced types (at the module level)
adds a new conversion rule for marshaling protobuf.Value objects
demonstrates two different usages of the enhanced classes (dict with to_value(); direct assignment with marshaling)

telpirion · 2020-12-08T02:03:36Z

tests/unit/test_value_converter.py

+        # `expected_type` is `test_value_converter.SomeMessage` while
+        # `actual_from_value_output` is just `SomeMessage`
+        # Use `isinstance()` instead.
+        #assert(type(actual_from_value_output) is type(expected_type))


I tried changing this to isinstance(), but instead get this message:

E AssertionError: assert False E + where False = isinstance(test_str: "Omnia Gallia est divisa"\ntest_int64: 3\ntest_bool: true\n, SomeMessage)

This feels like an artifact of how the tests are collected and run.

It could be. I'm inclined to leave this test as-is for now (since we test property-level equivalency). I'll log a GitHub issue and assign it to myself to further investigate this problem.

Does that work for you?

Sounds good.

dizcology · 2020-12-09T18:29:25Z

google/cloud/aiplatform/v1beta1/schema/predict/instance/__init__.py

@@ -14,6 +14,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
+from google.cloud.aiplatform_helpers import add_methods_to_classes_in_package


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 of aiplatform_helpers?

Also I think the convention here is to import the module and not individual methods or classes.

Also I think the convention here is to import the module and not individual methods or classes.

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.

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.

I see - perhaps add_methods_to_classes_in_package should be in a private module, where as value_converter a public module. in that case perhaps a nested submodule aiplatform.helpers.value_converter would be preferred over aiplatform_helpers.value_converter.

dizcology · 2020-12-09T18:32:51Z

google/cloud/aiplatform/v1beta1/schema/predict/prediction_v1beta1/types/text_sentiment.py

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


wouldn't this be overwritten by the next re-generation? Why is it necessary to change the import here?

If this replace needs to be made permanent, please do it in the synth.py. example

dizcology · 2020-12-09T18:59:33Z

google/cloud/aiplatform_helpers/__init__.py

+        cls.to_value.__doc__ = to_value.__doc__
+
+        # Add from_value() method to class with docstring
+        cls.from_value = add_from_value_to_class(cls)


why is this one not calling setattr like the other two methods?

Changed.

I was just trying different methods of assigning members dynamically; forgot to standardize on one technique.

dizcology · 2020-12-09T19:05:04Z

google/cloud/aiplatform_helpers/value_converter.py

+            return False
+        return True
+
+    props = list(filter(is_prop, dir(self._pb)))


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?

@software-dov Do you have any suggestsions?

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 json_format.ParseDict() and make sure that I can still train a model.

This apparently isn't needed! I've removed this code.

dizcology · 2020-12-09T19:05:55Z

google/cloud/aiplatform_helpers/value_converter.py

+    for prop in props:
+        props_dict[prop] = getattr(self._pb, prop)
+
+    return json_format.ParseDict(props_dict, Value())


does this work if some of the values of props_dict are nested proto messages?

Removed this bit.

dizcology · 2020-12-09T19:31:31Z

samples/snippets/create_training_pipeline_image_classification_sample.py


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


I rather prefer not to have additional method calls here.

(that is: define a new variable above so that the value of "trainign_task_inputs" is just that variable)

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.

dizcology · 2020-12-09T19:37:14Z

samples/snippets/predict_image_classification_sample.py

+    instances = [instance_val]
+
+    params_obj = params.ImageClassificationPredictionParams({
+        "confidence_threshold": 0.5, "max_predictions": 5})


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.

dizcology · 2020-12-09T19:43:19Z

tests/unit/test_enhanced_types.py

+ModelType = definition.AutoMlImageClassificationInputs().ModelType
+
+
+class EnhancedTypesTests(unittest.TestCase):


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 tests/unit to house tests that specifically have to do with enhanced types, and add yourself as codeowner of that folder (if tests are allowed to have codeowners).

CODEOWNERS are by directory, so that is definitely possible.

Both done: moved tests, switched to pytest rather than unittest.

dizcology · 2020-12-09T19:50:20Z

tests/unit/test_value_converter.py

+        # `expected_type` is `test_value_converter.SomeMessage` while
+        # `actual_from_value_output` is just `SomeMessage`
+        # Use `isinstance()` instead.
+        #assert(type(actual_from_value_output) is type(expected_type))


This feels like an artifact of how the tests are collected and run.

tests/unit/test_value_converter.py

busunkim96 · 2020-12-11T16:28:22Z

google/cloud/aiplatform/v1beta1/schema/predict/instance/__init__.py

@@ -14,6 +14,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
+from google.cloud.aiplatform_helpers import add_methods_to_classes_in_package


Also I think the convention here is to import the module and not individual methods or classes.

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.

busunkim96 · 2020-12-11T16:29:47Z

google/cloud/aiplatform/v1beta1/schema/predict/prediction_v1beta1/types/text_sentiment.py

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


If this replace needs to be made permanent, please do it in the synth.py. example

busunkim96 · 2020-12-11T16:30:48Z

...e/cloud/aiplatform/v1beta1/schema/trainingjob/definition_v1beta1/types/automl_forecasting.py

@@ -78,14 +78,14 @@ class AutoMlForecastingInputs(proto.Message):
            function over the validation set.

            The supported optimization objectives:
-              "minimize-rmse" (default) - Minimize root-
+            "minimize-rmse" (default) - Minimize root-


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

busunkim96 · 2020-12-11T17:39:37Z

google/cloud/aiplatform_helpers/value_converter.py

+            return False
+        return True
+
+    props = list(filter(is_prop, dir(self._pb)))


@software-dov Do you have any suggestsions?

busunkim96 · 2020-12-11T17:42:33Z

samples/snippets/create_training_pipeline_image_classification_sample.py

-    }
-    training_task_inputs = json_format.ParseDict(training_task_inputs_dict, Value())
+
+    icn_training_inputs = definition.AutoMlImageClassificationInputs(


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.

busunkim96 · 2020-12-11T17:43:23Z

tests/unit/test_enhanced_types.py

+ModelType = definition.AutoMlImageClassificationInputs().ModelType
+
+
+class EnhancedTypesTests(unittest.TestCase):


CODEOWNERS are by directory, so that is definitely possible.

leahecole · 2020-12-16T19:24:37Z

samples/snippets/create_training_pipeline_image_classification_sample.py

-    }
-    training_task_inputs = json_format.ParseDict(training_task_inputs_dict, Value())
+
+    icn_training_inputs = definition.AutoMlImageClassificationInputs(


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.

leahecole · 2020-12-16T19:25:24Z

samples/snippets/predict_image_classification_sample.py

    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)


nit - Is there a reason for the extra space at the beginning of this print statement?

I had a \t character in there earlier, but it got dropped accidentally. Added it back.

leahecole · 2020-12-16T19:26:38Z

samples/snippets/predict_image_classification_sample_test.py

@@ -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


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?

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.

leahecole · 2020-12-16T19:29:07Z

tests/unit/enhanced_library/CODEOWNERS

@@ -0,0 +1,2 @@
+# Tests for enhancements to the AI Platform library for Python.
+*       @telpirion


@busunkim96 should this instead be at .github/CODEOWNERS ?

and then be formatted as
(this might have whitespace errors but I'm just trying to convey the idea here :) )
/tests/unit/enhanced_library @telpirion

feat: adds function/method enhancements

c835503

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 6, 2020

product-auto-label bot added the samples Issues that are directly related to samples. label Dec 6, 2020

telpirion added 10 commits December 6, 2020 14:46

fix: sample tests

4e78f6f

fix: sample tests

fb81497

fix: region tag (linter)

1bb3c51

fix: change to aiplatform.gapic

9d44556

fix: linter

23c0dc9

fix: linter

e84c679

fix: lint

7a4cde5

feat: add unit tests

ec89cd1

fix: linter

7457ecd

fix: changed name

53e89e9

telpirion marked this pull request as ready for review December 7, 2020 19:34

telpirion requested review from dizcology and a team as code owners December 7, 2020 19:34

telpirion requested review from leahecole, busunkim96 and software-dov and removed request for a team December 7, 2020 19:34

fix: docstring issues in generated files

4cb779f

telpirion commented Dec 8, 2020

View reviewed changes

telpirion added 4 commits December 7, 2020 19:26

fix: adds from_map test, small fix to generated enhanced type

5b162d0

fix: lint

25b8814

fix: docstring breaks build :S

833c0fd

fix: more docstrings

ae5c058

dizcology reviewed Dec 9, 2020

View reviewed changes

busunkim96 reviewed Dec 11, 2020

View reviewed changes

fix: lint

2d70605

telpirion added 12 commits December 14, 2020 19:43

fix: lint;

45d5c7a

fix: blacken files

fe05ee5

fix: per reviewer

eb1bb2d

Merge branch 'master' into enhanced-lib2

3b20252

fix: per reviewers

bd0a9fd

Merge branch 'master' into enhanced-lib2

6053297

fix: reblacken

390a58c

fix: per reviewer

890e749

fix: per reviewer

821adb4

fix: per reviewer

667b49f

chore: added CODEOWNERS file to enhanced library tests

de2c3dd

fix: lint

8832966

telpirion requested review from dizcology and busunkim96 December 16, 2020 04:34

leahecole reviewed Dec 16, 2020

View reviewed changes

telpirion added 2 commits December 16, 2020 15:08

fix: per reviewer

c4a469d

fix: per reviewer

2a269f8

dizcology approved these changes Dec 17, 2020

View reviewed changes

dizcology merged commit 1a302d2 into master Dec 17, 2020

		ModelType = definition.AutoMlImageClassificationInputs().ModelType


		class EnhancedTypesTests(unittest.TestCase):

		@@ -0,0 +1,2 @@
		# Tests for enhancements to the AI Platform library for Python.
		* @telpirion

feat: adds function/method enhancements, demo samples #122

feat: adds function/method enhancements, demo samples #122

Conversation

telpirion commented Dec 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dizcology Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

dizcology Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telpirion Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dizcology Dec 9, 2020 •

edited

Loading

dizcology Dec 11, 2020 •

edited

Loading

telpirion Dec 16, 2020 •

edited

Loading