Vision: imageContext isn't documented

[jmdobry]

Customer wondered how to pass a crop hint option to Vision#cropHints(). Turns out, you can't. The helper methods only accept an Image and a grpc options object, and do not accept imageContext.

In order to pass say, the aspectRatios option as part of the cropHints request, you have to use the annotateImage method directly, like so:

const Vision = require('@google-cloud/vision');
const vision = Vision();
vision.annotateImage({
  image: {
    source: {
      filename: '/path/to/image.png'
    }
  },
  features: [{ type: Vision.types.Feature.Type.CROP_HINTS }],
  imageContext: {
    cropHintsParams: {
      aspectRatios: [0.25]
    }
  }
}).then(...);

I'm not sure how folks are supposed to know this.

imageContext itself isn't even documented on the annotateImage method

Also, accessing the enum (e.g. Vision.types.Feature.Type.CROP_HINTS) is only documented in the client library's README, and even then, only for FACE_DETECTION.

[arpwal]

You might want to update these docs also when you're at it --
https://cloud.google.com/vision/docs/detecting-crop-hints#vision-crop-hints-detection-nodejs

Thanks for following up jmdobry@

[stephenplusplus]

Thanks for reporting and sorry for the trouble @arpwal.

I believe we have two action items here:

1. Add a link to AnnotateImageRequest from annotateImage's JSDocs: https://github.com/GoogleCloudPlatform/google-cloud-node/blob/vision-0.12.0/packages/vision/src/helpers.js#L101
2. Allow a raw AnnotateImageRequest to be sent to the shorthand methods, like cropHints:

   vision.cropHints({
     image: {...},
     imageContext: {...}
   }, gaxOptions, callback)

   The internal method would extend the user's input with features: [features.CROP_HINTS]. This would be a breaking change. Do we want this behavior, or should we refer users to the annotateImage method instead?

[jmdobry]

Why does 2) have to be a breaking change?

var _createSingleFeatureMethod = featureValue => {
  return function(image, options) {
    return this.annotateImage({
      image: image,
      features: [{type: featureValue}],
    }, options);
  };
};

=>

var _createSingleFeatureMethod = featureValue => {
  return function(request, options) {
    // Assume that "request" is a full AnnotateImageRequest

    const features = [{ type: featureValue }];

    if (!request.image) {
      // All of users' current code would hit this path
      request = {
        image: request,
        features: features
      };
    }

    if (!request.features) {
      request.features = features;
    }
    return this.annotateImage(request, options);
  };
};

[stephenplusplus]

I'm not too bothered by the breaking change, and I would avoid the duality and just stick with "one way" of using the method. This API was designed by @lukesneeringer / automated, so let's get his take on it.

[jmdobry]

Okay. I'm personally okay with a breaking change, and making the shorthand methods take the normal AnnotateImageRequest, instead of taking just AnnotateImageRequest#image

[lukesneeringer]

I will look at it when I am at the office tomorrow, but this looks like a slip up on my part and easily corrected. It might be a breaking change only insofar as we take an extremely rarely used argument and shift it back one slot.

[lukesneeringer]

So, a couple thoughts:
First, I do not think it makes sense for the shorthand methods to take a full AnnotateImageRequest, because it muddies the source of truth when it comes to features. And, at that point, why have the shorthand methods at all?

In Python, I took arbitrary keyword arguments which I mapped onto the AnnotateImageRequest; I think something similar is also the right thing to do here. Basically there would be an object as the second argument, and any key other than features and image would be mapped onto the request.

This is a little less good in Node because it essentially means that these methods take three objects, but it still seems like the right thing to do to make the simple case easy and the advanced case possible.

[lukesneeringer]

Oh, wow, I just noticed the repeated instantiations needed in order to get docs working. :-/ I really hope we can make that automatic and use a JSDoc plugin in the future.

[stephenplusplus]

Oh, wow, I just noticed the repeated instantiations needed in order to get docs working.

What do you mean?

This is a little less good in Node because it essentially means that these methods take three objects, but it still seems like the right thing to do to make the simple case easy and the advanced case possible.

This concept is going to affect a lot of our methods; short hand arguments vs raw API format vs GAX options. We should figure out the best solution now and start applying it wherever it fits. We've struggled with this in various APIs since our inception, and have ended up with a couple different solutions. Most recently, we had to fit "gaxOptions" to the Logging API:

• Methods that don't have any options now accept gaxOptions: https://github.com/GoogleCloudPlatform/google-cloud-node/blob/7166f1a313191d54323b9339ad79e8798eecbeba/packages/logging/src/sink.js#L110

• Methods that accept options/configuration were given a new property: config.gaxOptions: https://github.com/GoogleCloudPlatform/google-cloud-node/blob/42c3958d4437225b23a16545bc24204b464a541c/packages/logging/src/index.js#L101 -- I chose the name gaxOptions so that it would be less likely / impossible to conflict with an API setting.

It's not great using the term "gaxOptions", because this has no meaning to the user. However, they can click from our docs to the gax docs and see all of the knobs without us having to duplicate docs and worry about going stale.

This is a little less good in Node because it essentially means that these methods take three objects, but it still seems like the right thing to do to make the simple case easy and the advanced case possible.

Mixing a method's config/options object with gaxOptions isn't great either, but has to be done to avoid conflict between them. If two serial object arguments are optional, how do you differentiate between the two? You can look for a whitelist of properties expected by either one, but that has the stale/future-proof concern.

[jmdobry]

I think these shorthand methods currently introduce greater cognitive dissonance than they alleviate. It's tiring deciphering the differences between a Service's REST/GRPC API and the request/response formats of the client library. I think the implementation of Vision#cropHints() should require nothing more than:

var _createSingleFeatureMethod = (featureValue) => {
  return function(annotateImageRequest, callOptions) {
    if (!annotateImageRequest.features) {
      annotateImageRequest.features = [{ type: featureValue }];
    }
    return this.annotateImage(annotateImageRequest, callOptions);
  };
};

[lukesneeringer]

Oh, wow, I just noticed the repeated instantiations needed in order to get docs working.

What do you mean?

I previously had a one-shot loop that iterated over an enum and created all the methods. Now the method is declared 8-10 times with the differing values, in order to allow the documentation parser to pick up the documentation comments.

This concept is going to affect a lot of our methods; short hand arguments vs raw API format vs GAX options. We should figure out the best solution now and start applying it wherever it fits. We've struggled with this in various APIs since our inception, and have ended up with a couple different solutions. Most recently, we had to fit "gaxOptions" to the Logging API:

Oh, it is nice to realize that this has some prior art. I did not realize that. (For a bikeshed, I would have preferred callOptions over gaxOptions, but that ship has now sailed.)

[stephenplusplus]

cc @callmehiphop for the API-wide discussion of a frictionless way to integrate gaxOptions (/callOptions)

[lukesneeringer]

I think these shorthand methods currently introduce greater cognitive dissonance than they alleviate. It's tiring deciphering the differences between a Service's REST/GRPC API and the request/response formats of the client library. I think the implementation of Vision#cropHints() should require nothing more than:

var _createSingleFeatureMethod = (featureValue) => {
  return function(annotateImageRequest, callOptions) {
    if (!annotateImageRequest.features) {
      annotateImageRequest.features = [{ type: featureValue }];
    }
    return this.annotateImage(annotateImageRequest, callOptions);
  };
};

Hmm. At the point that we are doing that, I almost wonder if it would make more sense to scrap these entirely and just ask the user to call annotateImage. Although not having to manually futz with the features list is still a significant value-add.

I could potentially get behind this, and it does have the advantage of solving every other problem.

[stephenplusplus]

I previously had a one-shot loop that iterated over an enum and created all the methods. Now the method is declared 8-10 times with the differing values, in order to allow the documentation parser to pick up the documentation comments.

Ah, yeah. Where that started: #2298 (comment)

---

I also like the @jmdobry proposal. These are just shortcut methods, that do "one shortcut" (setting the feature option for you). Otherwise, the interface being the same is a good thing.

[lukesneeringer]

One change I would make if we go with @jmdobry's solution is that the caution on updating features is probably an issue. It would be very surprising to call #cropHints() and not get crop hints because you provided features: [{type: 'labelDetection'}] or whatnot.

Therefore, the logic would need to become...

var _createSingleFeatureMethod = featureValue => {
  return function(annotateImageRequest, callOptions) {
    annotateImageRequest.features = annotateImageRequest.features || [];

    // Ensure the feature value indicated by the user's method choice exists on the
    // features array; if it does not, add it.
    var found = false;
    for (let feature of annotateImageRequest.features) {
      if (feature.type === featureValue) {
        found = true;
        break;
      }
    }
    if (found === false) {
      annotateImageRequest.features.push({type: featureValue});
    }

    // Call the underlying #annotateImage method.
    return this.annotateImage(annotateImageRequest, callOptions);
  };
};

[lukesneeringer]

I previously had a one-shot loop that iterated over an enum and created all the methods. Now the method is declared 8-10 times with the differing values, in order to allow the documentation parser to pick up the documentation comments.

Ah, yeah. Where that started: #2298 (comment)

Yeah, I definitely understand why you did it, but in the medium-term I would like to find a better way. The entire point of creating those methods on a loop is that the underlying enum (defined by autogen) may change.

We should punt on that issue until we covert Vision to JSDoc. I bet once we do, I can write a plugin to handle this case.

[lukesneeringer]

I am slowly becoming increasingly convinced that @jmdobry's solution is the best one.

[jmdobry]

I think if at all possible, google-cloud-node should rely on (meaning should accept and return) the request/response formats defined in the protos (though fully-handwritten libs might be an exception).

In cases where google-cloud-node needs to add stuff, they should be "non-breaking" additions, e.g. proto has field foo, but google-cloud-node accepts an additional field bar for client-library-specific configuration. Does NOT rename foo to fooWtf or hide foo or move foo up or down a level in the object (cropHints currently does this).

cropHints (if my suggestion is implemented) would be an example of a convenience method that sets the value for a proto-defined field on the standard proto-defined object on behalf of the user. The coerceImage image functionality is an example where the client lib accepts an additional library-specific field (image.source.filename) in an additive fashion (almost like it extended the proto in a backwards compatible way).

Couple of benefits:

1. Less dissonance between what users see in the client lib's ref docs vs the service's REST/gRPC ref docs, makes writing the client lib's ref docs a little easier (there's less to write).
2. As users bounce between the service ref docs and the client lib's ref docs, and even other languages' ref docs, their knowledge gained becomes cumulative, as opposed to constantly encountering client-library-specific request/response formats and being unable to recognize the patterns, resulting in frustration (we feel empowered when we recognize the patterns).
3. Client library leverages the thought and engineering that went into designing the service's API surface, which the eng/product team thinks represents what users want.

Obviously we still want the library to be idiomatic, but I think it can be idiomatic and also be well representative of the service's API surface.

[lukesneeringer]

I agree with all of these principles. :-) In fact, these were the reasons for going from the one-off methods in the manual layer to the one we have now.

I also agree that the proposed solution follows these principles better than what I did originally. Let's go with that.

[lukesneeringer]

#2555 updated accordingly.

[stephenplusplus]

Fixed in #2555.