Vision Partial Veneer #2298

lukesneeringer · 2017-05-11T22:21:16Z

This is the new surface with breaking changes.

This is an initial pass at the implementation of a "partial Veneer" for Vision.

It:

deprecates the entire manual layer
makes the GAPIC, with helpers, available at require('@google-cloud/vision').v1().
provides the annotateImage and single-feature methods
has 100% test coverage on manual code

It does not:

~~provide a default partial Veneer on the versionless entry point (because it is taken by the deprecated manual layer)~~ have backwards compatibility, at all, even remotely a little
handle specifying a file handler for an image (needs to be done)
provide documentation for the partial Veneer (needs to be done)
have 100% test coverage on automated code (needs to be done)

Notes:

This introduces the use of ES6 syntax supported by Node 4.
This uses power-assert ~~sinon and chai, which are not currently being used by other packages~~

stephenplusplus · 2017-05-11T22:32:17Z

provide a default partial Veneer on the versionless entry point (because it is taken by the deprecated manual layer)

If the manual layer wasn't there, what would the ideal solution have been?

This uses sinon and chai, which are not currently being used by other packages

Instinctively, I would think we should stick to our testing style in the rest of the repo. Is it just because it's easier, prettier, had some better features? We use sinon in places for easier stubbing, the chai part would be new.

jmdobry · 2017-05-11T22:32:39Z

provide a default partial Veneer on the versionless entry point (because it is taken by the deprecated manual layer)

In which version of @google-cloud/vision do you plan to export the partial gapic as the versionless entrypoint?

jmdobry · 2017-05-11T22:34:36Z

If the manual layer wasn't there, what would the ideal solution have been?

The ideal solution would be:

module.exports = vision_v1;
module.exports.v1 = vision_v1;
// fictional other version
module.exports.v1beta1 = vision_v1beta1;

right?

stephenplusplus · 2017-05-11T23:24:01Z

I think so, but I can see being explicit worth considering, as well, if we intend to carry multiple versions.

lukesneeringer · 2017-05-12T15:29:39Z

If the manual layer wasn't there, what would the ideal solution have been?

The ideal solution would be:
module.exports = vision_v1;
module.exports.v1 = vision_v1;
// fictional other version
module.exports.v1beta1 = vision_v1beta1;
right?

Precisely.

If you ask for a version, you get that version.
If you do not, you get the latest GA.

If we ever change which one is assigned to module.exports in the future, it is semver-major.

lukesneeringer · 2017-05-12T15:34:41Z

In which version of @google-cloud/vision do you plan to export the partial gapic as the versionless entrypoint?

I do not know; I was going to ask for feedback on that. :-) I did not realize I would run up against that problem until yesterday when I was writing the code.

There are a few options:

Move the old layer to a new namespace (backwards-incompatible change), and document that. Existing customers make a one-line change to "get working again" and then can go port their stuff to the new surface later.
The current form: For now, you need to use the version to get the new stuff.
- A hybrid of the above two: We do the current form and also and old manual namespace; give people a month or so to make the one-line change, then they have a longer-deprecation period to update to the new surface.
Move fast and break things: Just wipe out the manual surface sooner in Node than in other languages. (In fairness, Node developers are likely the most receptive to this option of any language community. :-))

lukesneeringer · 2017-05-12T15:37:29Z

Instinctively, I would think we should stick to our testing style in the rest of the repo. Is it just because it's easier, prettier, had some better features? We use sinon in places for easier stubbing, the chai part would be new.

I really needed sinon to do the mocks I wanted. Since you are using it elsewhere, I assume this is not interesting.

I ended up adding chai at the last minute because assert was not giving me a useful error message for a particular test failure, and I knew that chai would. I switched it, got the error I needed, and fixed the test. Once I fixed the test, I did not bother to change it back. On the one hand, I could totally change it back now; on the other hand, I prefer more useful error messages for future test failures. I could go either way on this.

lukesneeringer · 2017-05-12T15:38:54Z

@stephenplusplus I would also like your thoughts on my use of ES6 syntax. Is that something you are okay with beginning to use now that we dropped Node 0.12 support, or would you prefer have the repo remain ES5 for the forseeable future?

stephenplusplus · 2017-05-12T15:42:04Z

Move fast and break things.

I also don't feel strongly about chai or no chai, if it helps us. My concern would be adding another tool to learn that contributors have to face.

ES6 syntax is great.

I'll do a deeper dive soon.

jmdobry · 2017-05-12T15:45:20Z

I agree, move fast and break things. It's better to remove the manual code pre-1.0 than to have to do a post-1.0 major version bump.

jmdobry · 2017-05-12T15:46:29Z

I prefer more useful error messages

The most useful error message come not from using chai, but by using power-assert.

lukesneeringer · 2017-05-12T15:49:15Z

I agree, move fast and break things. It's better to remove the manual code pre-1.0 than to have to do a post-1.0 major version bump.

These are not the only options, though. We are not going 1.0 when we release this. We could deprecate the manual layer, and still remove it pre-1.0.

(Nonetheless, I am okay with "move fast and break things" in Node. I am not okay with it in Python and Ruby.)

lukesneeringer · 2017-05-12T15:55:18Z

The most useful error message come not from using chai, but by using power-assert.

Oh hey, I like that way better. (That is similar to py.test in Python.)
I will swap to that later.

lukesneeringer · 2017-05-12T19:17:53Z

I moved over to power-assert. Holding off on scrapping the manual layer until that can have more discussion.

packages/vision/package.json

    "tmp": "^0.0.31"
  },
+  "nyc": {


packages/vision/src/helpers.js

+    // it to content.
+    if (request.image.source && request.image.source.filename) {
+      request.image = {
+        content: fs.readFileSync(request.image.source.filename),


packages/vision/src/helpers.js

+    });
+
+    // Assign the single-feature method to the `methods` object.
+    methods[methodName] = promisify(_createSingleFeatureMethod(featureValue));


packages/vision/src/helpers.js

+   *   API call.
+   *
+   * @example
+   * var vision = require('@google-cloud/vision').v1();


packages/vision/src/helpers.js

+   *   image: {source: {image_uri: 'gs://path/to/image.jpg'}},
+   *   features: [],
+   * };
+   * client.annotateImage(request).then(response => {


packages/vision/src/index.js

+  // Create the image annotator client with the provided options.
+  var client = gapic.v1(opts).imageAnnotatorClient(opts);
+  if (is.undefined(client.annotateImage)) {
+    Object.assign(client.constructor.prototype, helpers('v1'));


coveralls · 2017-06-09T16:25:32Z

Coverage decreased (-3.6%) to 96.4% when pulling 5dfd111 on lukesneeringer:vision-semi-gapic into f4c4a79 on GoogleCloudPlatform:master.

lukesneeringer · 2017-06-09T16:42:40Z

The coverage change seems to actually be because so much code was removed as to materially affect the overall total. The Vision coverage is 100%.

lukesneeringer · 2017-06-09T16:43:55Z

@stephenplusplus @jmdobry The ML folks want this delivered reasonably soon. Can you give it another look and see if everything looks good?

In particular, be looking at the pattern, since future GAPIC and partial Veneer releases will follow it.

package.json

    "propprop": "^0.3.1",
    "semver": "^5.3.0",
    "shelljs": "^0.7.3",
+    "sinon": "^2.2.0",


stephenplusplus · 2017-06-09T20:09:10Z

@lukesneeringer I'm okay with it overall. The only issue is the docs: #2298 (comment)

coveralls · 2017-07-26T17:08:32Z

Coverage decreased (-3.5%) to 96.525% when pulling 9ff7626 on lukesneeringer:vision-semi-gapic into d10362f on GoogleCloudPlatform:master.

coveralls · 2017-07-26T17:17:08Z

Coverage decreased (-3.5%) to 96.525% when pulling 9ff7626 on lukesneeringer:vision-semi-gapic into d10362f on GoogleCloudPlatform:master.

coveralls · 2017-07-26T17:26:06Z

Coverage decreased (-3.5%) to 96.525% when pulling 5934a6d on lukesneeringer:vision-semi-gapic into d10362f on GoogleCloudPlatform:master.

coveralls · 2017-07-26T17:39:27Z

Coverage remained the same at 100.0% when pulling f84598f on lukesneeringer:vision-semi-gapic into d10362f on GoogleCloudPlatform:master.

stephenplusplus · 2017-07-26T17:45:18Z

Made some changes.

Docs: http://stephenplusplus.github.io/gcloud-node/#/docs/vision/master/vision

lukesneeringer · 2017-07-26T17:46:04Z

Looks good to me. Thanks @stephenplusplus

julien-c · 2017-10-11T21:45:28Z

I'm using the Vision API to annotate an image with several features at the same time.

Unless I'm missing something obvious, the new 0.12 SDK makes it pretty hard to do this (I had to read the code to find the feature type enumeration)

Happy to open a new issue if that makes sense.

lukesneeringer added api: vision Issues related to the Cloud Vision API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. status: do not merge labels May 11, 2017

lukesneeringer self-assigned this May 11, 2017

lukesneeringer requested review from stephenplusplus and jmdobry May 11, 2017 22:21

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 11, 2017

stephenplusplus reviewed May 13, 2017

View reviewed changes

packages/vision/package.json Outdated

"tmp": "^0.0.31"

},

"nyc": {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

jmdobry reviewed May 15, 2017

View reviewed changes

packages/vision/src/helpers.js Outdated

// it to content.

if (request.image.source && request.image.source.filename) {

request.image = {

content: fs.readFileSync(request.image.source.filename),

This comment was marked as spam.

Sign in to view

stephenplusplus reviewed Jun 9, 2017

View reviewed changes

lukesneeringer removed the status: do not merge label Jun 9, 2017

stephenplusplus reviewed Jun 9, 2017

View reviewed changes

package.json Outdated

"propprop": "^0.3.1",

"semver": "^5.3.0",

"shelljs": "^0.7.3",

"sinon": "^2.2.0",

This comment was marked as spam.

Sign in to view

Luke Sneeringer and others added 9 commits July 26, 2017 12:18

Play with Vision example a bit.

e073421

string together docs

9516cda

rm gh-pages

8fe730a

Change to use manual explicit single-feature methods with documentation

95c0fa6

Sync with gapic auto-gen

aa88cc1

Adjust package version and dummy js files for doc

bd2e8ef

Add smoke test and update version number

ec8b1eb

rebase

7d0b9b3

urls

b391b7d

stephenplusplus force-pushed the vision-semi-gapic branch from ada694f to b391b7d Compare July 26, 2017 16:44

stephenplusplus added 3 commits July 26, 2017 12:52

typos

434c791

docs fix

d674b45

remove instantation from examples

9ff7626

remove coverage dir, revert version, remove sinon

5934a6d

redefine exclude patterns

f84598f

stephenplusplus merged commit a3406b0 into googleapis:master Jul 26, 2017

lukesneeringer removed the priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. label Jul 26, 2017

stephenplusplus removed cla: no This human has *not* signed the Contributor License Agreement. status: do not merge labels Jul 26, 2017

stephenplusplus mentioned this pull request Aug 24, 2017

Vision: imageContext isn't documented #2553

Closed

sofisl mentioned this pull request Jan 17, 2023

migrate code from googleapis/nodejs-vision #3860

Merged

4 tasks

sofisl pushed a commit that referenced this pull request Jan 17, 2023

Vision Partial Veneer (#2298)

214ab0e

Vision Partial Veneer #2298

Vision Partial Veneer #2298

Conversation

lukesneeringer commented May 11, 2017 • edited by swcloud Loading

stephenplusplus commented May 11, 2017

jmdobry commented May 11, 2017

jmdobry commented May 11, 2017

stephenplusplus commented May 11, 2017

lukesneeringer commented May 12, 2017 • edited Loading

lukesneeringer commented May 12, 2017 • edited Loading

lukesneeringer commented May 12, 2017

lukesneeringer commented May 12, 2017

stephenplusplus commented May 12, 2017

jmdobry commented May 12, 2017

jmdobry commented May 12, 2017

lukesneeringer commented May 12, 2017

lukesneeringer commented May 12, 2017

lukesneeringer commented May 12, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Jun 9, 2017

lukesneeringer commented Jun 9, 2017

lukesneeringer commented Jun 9, 2017

This comment was marked as spam.

stephenplusplus commented Jun 9, 2017 • edited Loading

coveralls commented Jul 26, 2017

coveralls commented Jul 26, 2017

coveralls commented Jul 26, 2017

coveralls commented Jul 26, 2017

stephenplusplus commented Jul 26, 2017

lukesneeringer commented Jul 26, 2017

julien-c commented Oct 11, 2017

lukesneeringer commented May 11, 2017 •

edited by swcloud

Loading

lukesneeringer commented May 12, 2017 •

edited

Loading

lukesneeringer commented May 12, 2017 •

edited

Loading

stephenplusplus commented Jun 9, 2017 •

edited

Loading