feat: adds enhancements to library #22

telpirion · 2020-11-26T00:37:14Z

This PR adds the following to the AI Platform library:

Adds helper methods to enhanced classes
Includes samples and sample tests that demonstrate usage
Includes client library tests for manual client library layers

After this pull request is submitted, these tasks are required for follow-up:

snippet-bot · 2020-11-26T00:37:21Z

Here is the summary of changes.

You added 3 region tags.

samples/create-training-pipeline-image-classification.js:26, tag aiplatform_create_training_pipeline_image_classification
samples/list-endpoints.js:19, tag aiplatform_list_endpoints
samples/predict-image-classification.js:20, tag aiplatform_predict_image_classification

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

…ejs-ai-platform into enhanced-lib-try3

codecov · 2020-12-03T22:47:52Z

Codecov Report

Merging #22 (0f9d93a) into master (4f7318b) will decrease coverage by 0.03%.
The diff coverage is 97.18%.

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage   98.85%   98.81%   -0.04%     
==========================================
  Files          12       15       +3     
  Lines       20173    20493     +320     
  Branches      544      575      +31     
==========================================
+ Hits        19942    20251     +309     
- Misses        223      234      +11     
  Partials        8        8

Impacted Files	Coverage Δ
src/value-converter.ts	`93.49% <93.49%> (ø)`
src/decorator.ts	`99.08% <99.08%> (ø)`
src/helpers.ts	`100.00% <100.00%> (ø)`
src/index.ts	`100.00% <100.00%> (ø)`
src/v1beta1/endpoint_service_client.ts	`98.97% <0.00%> (-0.09%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f7318b...0f9d93a. Read the comment docs.

alexander-fenster · 2020-12-15T20:50:05Z

src/decorator.ts

+      const namespace =
+        namespaceName in rootNamespace
+          ? rootNamespace[namespaceName]
+          : undefined;


You don't have to use a ternary here, rootNamespace[namespaceName] will be undefined if it's not there, so just const namespace = rootNamespace[namespaceName] is equivalent to this code

alexander-fenster · 2020-12-15T20:51:11Z

src/decorator.ts

+
+      // Get the namespace object from JSON
+      const namespaceJsonObject =
+        namespaceName in jsonNode ? jsonNode[namespaceName] : undefined;


same as ↑↑

alexander-fenster · 2020-12-15T20:51:59Z

src/decorator.ts

+}
+
+// Assign the toValue() and fromValue() helper methods to the enhanced message objects.
+// tslint:disable-next-line no-any


this disable-next-line should be moved one line down I think?

alexander-fenster · 2020-12-15T20:54:47Z

src/helpers.ts

+  const value = googleProtobufValueFromObject(obj, (val: any) => {
+    return val;
+  });
+  if (typeof value !== 'undefined') {


This if, combined with the final return undefined, can be replaced with just return value;

alexander-fenster · 2020-12-15T20:55:25Z

src/helpers.ts

+  return undefined;
+}
+
+export function fromValue(value: any): object | null | undefined {


tslint-disable-next-line?

alexander-fenster · 2020-12-15T20:55:45Z

src/value-converter.ts

+
+// TODO(): Remove this file once https://github.com/protobufjs/protobuf.js/pull/1495 is submitted.
+export function googleProtobufValueFromObject(
+  object: any,


tslint-disable-next-line?

alexander-fenster · 2020-12-15T20:57:15Z

synth.py

@@ -29,6 +29,19 @@
  library = gapic.node_library(name, version)
  s.copy(library, excludes=["package.json", "README.md"])

+# Expose helper module
+index_file = 'src/index.ts'


So here I always wonder if we should do this or just exclude src/index.ts from the generation (line 30) and just have src/index.ts unchanged by the synthtool. Up to you here :)

I think I'm going to err on the side of patching the library after generation, for now. We're doing some patching after the fact in the Python library too. @bcoe , your input gratefully accepted.

If I'm following this regex, we're simply matching the end of the file? I think I like this approach for now 👍 as it allows us to add new clients via automation, without manual intervention -- let's try it.

Awesome, thank you! I'm happy to work with you on potential better solutions in the future.

Per conversation over chat, we decided to not do any post-processing in synth.py and instead exclude the hand-written files from being overwritten.

alexander-fenster

LGTM with some nits, all of them minor

bcoe

A few very minor nits, you folks did incredible work pulling this together this week.

bcoe · 2020-12-16T16:23:20Z

.gitignore

@@ -12,3 +12,4 @@ system-test/*key.json
 .DS_Store
 package-lock.json
 __pycache__
+.vscode


we should consider moving .gitignore into a template, @sofisl does your library generation tool populate the file now?

bcoe · 2020-12-16T16:25:00Z

samples/create-training-pipeline-image-classification.js

+) {
+  // [START aiplatform_create_training_pipeline_image_classification]
+  /**
+   * TODO(developer): Uncomment these variables before running the sample.\


no need for the break \ on the end of the line, for multiline comments.

bcoe · 2020-12-16T16:29:06Z

samples/test/create-training-pipeline-image-classification.test.js

+      stdout,
+      /Create training pipeline image classification response/
+    );
+    trainingPipelineId = stdout


I could imagine this assertion becoming a bit fragile, I'd just test for:

assert.match(stdout, /\/locations\/us-central1\/trainingPipelines\//)

Like above.

bcoe · 2020-12-16T16:30:49Z

src/decorator.ts

+// Walk the tree of nested namespaces contained within the enhanced-types.json file
+function walkNamespaces(
+  // tslint:disable-next-line no-any
+  jsonNode: Record<string, any>,


would it silence these warnings if we we gave a slightly more specific type hint like Record<string, object>?

Changed.

Unfortunately no, setting those to Record<string, object> didn't work.

I tried to provide define / provide types as much as possible in the recent commits. However, in some cases I needed to cast to unknown before casting to another type. In two specific cases, I wasn't able to work around using any--both uses are deep within the bowels of internal functions. (Even so, I'll continue to noodle on ways to address this issue.)

bcoe · 2020-12-16T16:34:15Z

synth.py

@@ -29,6 +29,19 @@
  library = gapic.node_library(name, version)
  s.copy(library, excludes=["package.json", "README.md"])

+# Expose helper module
+index_file = 'src/index.ts'


If I'm following this regex, we're simply matching the end of the file? I think I like this approach for now 👍 as it allows us to add new clients via automation, without manual intervention -- let's try it.

tsconfig.json

JustinBeckwith · 2020-12-15T16:54:16Z

samples/create-training-pipeline-image-classification.js

+    }
+  }
+  // [END aiplatform_create_training_pipeline_image_classification]
+  await createTrainingPipelineImageClassification();


This method call needs to be inside of the region tag, or the code the customer pastes will never be run 🙃 Here and anywhere else in the samples please.

@telpirion if you take this approach, then the await should be dropped. @sofisl @fhinkel, it would be good to update the sample policies to include uncaughtRejections, so that we properly exit with status code 1, even when we're not awaiting.

JustinBeckwith · 2020-12-15T16:55:04Z

samples/package.json

@@ -13,7 +13,9 @@
    "test": "mocha --timeout 600000 test/*.js"
  },
  "dependencies": {
-    "@google-cloud/aiplatform": "^1.0.0"
+    "@google-cloud/aiplatform": "^1.0.0",
+    "chai": "^4.2.0",


chai, and I'm guessing uuid, should be devDependencies if they are not strictly required to run the application code. Dependencies only used in tests go in devDependencies.

JustinBeckwith · 2020-12-15T16:55:37Z

samples/test/create-training-pipeline-image-classification.test.js

+const uuid = require('uuid').v4;
+const cp = require('child_process');
+const execSync = cmd => cp.execSync(cmd, {encoding: 'utf-8'});
+const cwd = path.join(__dirname, '..');


Probably don't need to define or use cwd anywhere here - generally leaving it out works fine.

JustinBeckwith · 2020-12-15T16:57:21Z

samples/test/create-training-pipeline-image-classification.test.js

+      .split('\n')[0];
+  });
+
+  after('should cancel the training pipeline and delete it', async () => {


Similar feedback left recently for @lucaswadedavis - for any of these after hooks, you can't reasonably promise they will always run. If the process crashes, server goes down, someone locally hits CTRL-C - the resources created here will be abandoned. This eventually leads to quota errors, or lagging performance problems as the number of resources grow. We should add some code in the beginning of the test suite that does resource cleanup, reaping any old resources that should have been cleaned up but got missed here.

Resolved over chat--will follow up with clean up function soon.

JustinBeckwith · 2020-12-15T16:57:48Z

src/decorator.ts

+// Walk the tree of nested namespaces contained within the enhanced-types.json file
+function walkNamespaces(
+  // tslint:disable-next-line no-any
+  jsonNode: Record<string, any>,


Please treat any lint warnings as errors (we are going to fix this soon)

Changed (as much as I could). There are still two uses of any that I haven't been able to work around yet (in value-converters.ts). I'll continue to look for ways to remove any where used.

JustinBeckwith · 2020-12-15T16:59:30Z

synth.py

@@ -29,6 +29,19 @@
  library = gapic.node_library(name, version)
  s.copy(library, excludes=["package.json", "README.md"])

+# Expose helper module
+index_file = 'src/index.ts'


Danger! We very much don't want to make code specific changes like this in synth.py if it can be avoided. Can we double click here and talk about what's happening with @alexander-fenster and @bcoe ?

Yes, this one is obviously a ❄️ . We have a plan to think how these exceptions (libraries augmented by extra stuff, like speech, vision, and this one) can be avoided, this is something I will look into in Q1.

After chatting with @alexander-fenster and @sofisl , I've reverted the changes to synth.py so that we exclude the handwritten changes from being over written (rather than having custom post-processing steps in synth.py).

feat: adds enhancements to library

b5d62d6

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

telpirion added 12 commits November 25, 2020 16:37

Delete launch.json

ed2caea

fix: sample parameter values

ba97e91

fix: samples issue

2e44a2e

fix: sample tests

c981127

fix: linter

ffe8e42

fix: linter

663428b

fix: linter

cfe878d

fix: linter

59f85d7

fix: remove .vscode

7bddd04

Merge branch 'enhanced-lib-try3' of https://github.com/googleapis/nod…

7bb3d5e

…ejs-ai-platform into enhanced-lib-try3

fix: lint

83cb877

chore: add VSCode artifacts to .gitignore

5c0e34a

telpirion added 7 commits December 5, 2020 14:41

Merge branch 'master' into enhanced-lib-try3

e01e371

chore: per reviewer feedback

5bf647b

fix: lint

9801518

Merge branch 'master' into enhanced-lib-try3

4009b1a

chore: changes to synth.py

76edfbf

fix: resolved TS lint issue

671b9a4

fix: lint

e97531f

telpirion requested review from bcoe, alexander-fenster and sofisl December 14, 2020 00:58

telpirion marked this pull request as ready for review December 14, 2020 00:58

telpirion requested review from a team as code owners December 14, 2020 00:58

fix: broken pack n' play test

05d6766

alexander-fenster reviewed Dec 15, 2020

View reviewed changes

alexander-fenster approved these changes Dec 15, 2020

View reviewed changes

telpirion added 2 commits December 15, 2020 20:46

fix: per reviewer

5dea693

fix: adds docstrings to toValue(), fromValue() functions

e8beb5e

bcoe added the cla: yes This human has signed the Contributor License Agreement. label Dec 16, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 16, 2020

bcoe suggested changes Dec 16, 2020

View reviewed changes

JustinBeckwith suggested changes Dec 16, 2020

View reviewed changes

telpirion added 6 commits December 16, 2020 17:15

fix: removing any

e4d68f2

fix: edits to synth.py per reviewer

3b77763

fix: per reviewer

7a54031

fix: per reviewer

8e5640e

fix: per reviewer

1606b8a

fix: lint

4fde9f7

telpirion requested review from bcoe and JustinBeckwith December 17, 2020 03:59

telpirion added 4 commits December 16, 2020 20:03

fix: lint

8941fe5

fix: add more test coverage

2933e48

fix: tests

f220b3f

chore: added comment about conversion interface

0f9d93a

bcoe approved these changes Dec 17, 2020

View reviewed changes

JustinBeckwith approved these changes Dec 18, 2020

View reviewed changes

telpirion merged commit b697a38 into master Dec 18, 2020

release-please bot mentioned this pull request May 18, 2022

chore(main): release 2.0.0 #305

Merged

feat: adds enhancements to library #22

feat: adds enhancements to library #22

Conversation

telpirion commented Nov 26, 2020 • edited Loading

snippet-bot bot commented Nov 26, 2020 • edited Loading

codecov bot commented Dec 3, 2020 • edited Loading

Codecov Report

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

alexander-fenster Dec 15, 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

alexander-fenster left a comment

Choose a reason for hiding this comment

bcoe left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telpirion commented Nov 26, 2020 •

edited

Loading

snippet-bot bot commented Nov 26, 2020 •

edited

Loading

codecov bot commented Dec 3, 2020 •

edited

Loading

alexander-fenster Dec 15, 2020 •

edited

Loading