Skip to content
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

Introduce Gapic-generated files for language API. #1476

Merged
merged 14 commits into from
Aug 25, 2016

Conversation

jmuk
Copy link
Contributor

@jmuk jmuk commented Aug 10, 2016

Fixes #1463
Fixes #1464

  • language_service_api.js and language_service_client_config.json:
    auto-generated
  • v1beta1/index.js: manually written
  • index.js and package.json: manually modified

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 10, 2016
@jmdobry
Copy link
Contributor

jmdobry commented Aug 10, 2016

So, to use it:

var Language = require('@google-cloud/language');

// hand-written client, highest level of abstraction, idiomatic/sugar methods
var language = Language();
language.annotate(...);
language.detectEntities(...);
language.detectSentiment(...);

// auto-gen client, middle level of abstraction, still has auto-retry, auth, etc.
var api = Language.v1beta1.LanguageServiceApi();
api.annotateText(...);
api.analyzeEntities(...);
api.analyzeSentiment(...);

// grpc, lowest level of abstraction, just a grpc stub, have to do auth manually
var LanguageService = Language.v1beta1.grpc().LanguageService;
var stub = new LanguageService('language.googleapis.com', credentials); // credentials acquired manually
stub.annotateText(...);
stub.analyzeEntities(...);
stub.analyzeSentiment(...);

@jmuk
Copy link
Contributor Author

jmuk commented Aug 10, 2016

That's right -- but two notes:

  • on the second part, api has a property called stub for a Promise to the gRPC client stub. So if the user wants to use a gRPC stub, they can always use like api.stub.then(function(stub) { stub.annotateText(...); }
  • I actually added grpc() mostly for enums. Many requests contain enum properties, and we should allow the access to those constants. For example:
var languageGrpc = Language.v1beta1.grpc();
api.annotateText({type: languageGrpc.Document.Type.PLAIN_TEXT, content: "Hello, world!"}, ...);

I am not sure what is the best design to reveal those gRPC constants to the users -- please let me know if you have some better ideas.

@jmdobry
Copy link
Contributor

jmdobry commented Aug 10, 2016

I am not sure what is the best design to reveal those gRPC constants to the users

Wouldn't the hand-written part want to expose them to the user for easiest access? We'd also want JSDoc comments on them so they get into the library's documentation.

@stephenplusplus
Copy link
Contributor

on the second part, api has a property called stub for a Promise to the gRPC client stub. So if the user wants to use a gRPC stub, they can always use like api.stub.then(function(stub) { stub.annotateText(...); }

Just curious why this is a promise. Does anything async need to happen before a stub can be used? In other words, why not just:

var api = Language.v1beta1.LanguageServiceApi();
api.stub.annotateText();

I think I like accessing the "stub" through a different method better than the stub property, like in @jmdobry's example:

// grpc, lowest level of abstraction, just a grpc stub, have to do auth manually
var LanguageService = Language.v1beta1.grpc().LanguageService;
var stub = new LanguageService('language.googleapis.com', credentials); // credentials acquired manually
stub.annotateText(...);
stub.analyzeEntities(...);
stub.analyzeSentiment(...);

How I would personally use that:

var gcloudLanguage = require('@google-cloud/language');
var grpc = gcloudLanguage.v1beta1.grpc();

var language = new grpc.LanguageService('language.googleapis.com', credentials);
language.annotateText({
  // something that needs an enum
  type: grpc.Document.Type.PLAIN_TEXT
}, ...);
language.analyzeEntities(...);
language.analyzeSentiment(...);

In the case where there are multiple services that a user can instantiate, isn't it common or always the case that they can share an auth client? If that's true, maybe we should make the v1beta1 property a function instead, that can persist a single auth client / other config details that can be re-used in the services beneath it:

-var api = language.v1beta1.LanguageServiceApi();
-var language = api.LanguageServiceApi({ projectId: 'grape-spaceship-123', keyFilename: '...' });
-var otherService = api.OtherServiceApi({ projectId: 'grape-spaceship-123', keyFilename: '...' });

+var api = language.v1beta1({ projectId: 'grape-spaceship-123', keyFilename: '...' });
+var language = api.LanguageServiceApi();
+var otherService = api.OtherServiceApi();
// ^-- both share the auth client

language.annotateText(...);
otherService.method(...);

Anywhere there is an upper-camelcase identifier, new is expected to be used. Code linters will throw something like "new expected on constructor function" if they see:

var language = api.LanguageServiceApi();

My preference would be to not require new for the auto-gen client, and just rename the methods to lower camelcase (api.languageServiceApi()).

Would you explain more about what kind of documentation is expected for these new APIs? Specifically, will the auto gen client have a separate page that lists all of the methods and arguments, with examples? And will the "stub" have a separate page with the same? Since we're going to have 3 ways of doing the same thing, I'm concerned about the added complexity on our docs site. Let me know if there's an existing vision for how these docs will intertwine.

@jmuk
Copy link
Contributor Author

jmuk commented Aug 10, 2016

Pushed a new patch, but that fixes minor syntactic things.

on the second part, api has a property called stub for a Promise to the gRPC client stub. So if the user wants to use a gRPC stub, they can always use like api.stub.then(function(stub) { stub.annotateText(...); }

Just curious why this is a promise. Does anything async need to happen before a stub can be used?

That's because gRPC stub needs to wait for the authentication. languageServiceApi handles the authentication and gRPC channel setup, and then creates the stub. Promise packages these asynchronous behavior.

Also languageServiceApi offers methods, so normally users don't have to access stub directly unless they want to access the bare gRPC client behind the API instance.

@jmuk
Copy link
Contributor Author

jmuk commented Aug 10, 2016

In the case where there are multiple services that a user can instantiate, isn't it common or always the case that they can share an auth client?

Right now Google-GAX has its own auth factory as https://github.com/googleapis/gax-nodejs/blob/master/lib/grpc.js#L36, so multiple service instances share the same client instance if the service instances are under the same package.

By adopting google-auto-auth library, your design would make more sense -- but thinking further, the auth client can't be shared among multiple APIs (like Language API and Speech API), right?

var language = require('@google-cloud/language').v1beta1({keyFile: '...'});
var speech = require('@google-cloud/speech').v1({keyFile: '...'});
var langApi = language.languageServiceApi();
var speechApi = speech.speechApi();
langApi.annotateText(...);
speechApi.nonStreamingRecognize(...);
// langApi and speechApi don't share the auth client...

So rather than doing this, it would probably better to accept an 'auth' parameters, wouldn't it?

var Auth = require('google-auto-auth');
var auth = new Auth({keyFile: '...'});
var language = require('@google-cloud/language').v1beta1;
var speech = require('@google-cloud/speech').v1;
var langApi = language.languageServiceApi({auth: auth});
var speechApi = speech.speechApi({auth: auth});
...

@jmuk
Copy link
Contributor Author

jmuk commented Aug 10, 2016

Would you explain more about what kind of documentation is expected for these new APIs? Specifically, will the auto gen client have a separate page that lists all of the methods and arguments, with examples? And will the "stub" have a separate page with the same?

We want to have a separate page for the auto-gen clients, with the list of methods, arguments, and examples.

I don't care about stubs. We don't require showing their documents, it does not affect the usability of auto-gen clients.

@jmdobry
Copy link
Contributor

jmdobry commented Aug 10, 2016

So rather than doing this, it would probably better to accept an 'auth' parameters, wouldn't it?

That was my initial thought on the subject.

I don't care about stubs. We don't require showing their documents, it does not affect the usability of auto-gen clients.

Yeah, the stubs are just a proto file turned into JavaScript by Protobuf.js, and a user can look at the proto file or the API's gRPC reference docs to understand the resulting structure of the stub.

@stephenplusplus
Copy link
Contributor

stephenplusplus commented Aug 11, 2016

var language = require('@google-cloud/language').v1beta1({keyFile: '...'});
var speech = require('@google-cloud/speech').v1({keyFile: '...'});
var langApi = language.languageServiceApi();
var speechApi = speech.speechApi();
langApi.annotateText(...);
speechApi.nonStreamingRecognize(...);

I would rather not require the user to install & learn an external dependency (google-auto-auth) in order to be able to use the autogen layer. Also, google-auto-auth returns an auth client, already bound to the required scopes. The user would have to know the scopes the API requires to create the correct auth client. We should just force the cost on the user of duplicate config objects containing their keyfile, projectId, and other required settings from the API.

That's because gRPC stub needs to wait for the authentication. languageServiceApi handles the authentication and gRPC channel setup, and then creates the stub. Promise packages these asynchronous behavior.

I think our libraries somewhat clash here, since we don't support promises (yet), and I think when we do, we would put anything that returns a promise behind a function call, e.g. getStub().then(...) rather than stub.then(...). I know stub will technically exist on the instantiated object, but can we only document and advise using the grpc() method?

I'm on board with stub if we can get it to work like this:

var api = Language.v1beta1.LanguageServiceApi();
api.stub.annotateText();

multiple service instances share the same client instance if the service instances are under the same package.

var service1 = Language.v1beta1.ServiceOneApi({ auth details });
var service2 = Language.v1beta1.ServiceTwoApi(); // how does this know about `{auth details}`

The user would have to provide the auth details twice, resulting in two auth clients being created, right? If it moves a level up, I believe those issues would go away:

var languageServices = Language.v1beta1({ auth details });
var service1 = languageServices.ServiceOneApi();
var service2 = languageServices.ServiceTwoApi();

Regarding exposing the gRPC constants, can we put those right on the autogen class as statics?

Language.v1beta1.Document.Type.PLAIN_TEXT

@jmuk
Copy link
Contributor Author

jmuk commented Aug 11, 2016

I didn't intend to advertise the existence of stub. It's not expected to be used by normal users. If it attracts your attentions, I now think we should just hide it. That would be simpler.

@jmuk
Copy link
Contributor Author

jmuk commented Aug 11, 2016

I wanted to allow injecting gRPC module itself too, to share the same C-extension. This is quite limited usage, but this would be helpful to some expert users in some case.

I am in the process of modifying v1beta1 as a function (taking auth params), and thinking this gRPC injection as a part of this option. This means:

var language = require('@google-cloud/language');
var service = language.v1beta1({keyFile: ...});
var api = service.LanguageServiceApi();
api.annotateText({type: service.grpc.Document.Type.PLAIN_TEXT, ... });

If the user has some special need to customize gRPC module:

var grpc = ...;
var service = language.v1beta1({keyFile: ..., grpc: grpc});
...

@jmuk
Copy link
Contributor Author

jmuk commented Aug 11, 2016

Forgotten to mention: I didn't think of the difference of scopes, and indeed that would affect the design. Thank you for pointing it out.

@stephenplusplus
Copy link
Contributor

Removing stub, making v1beta1 as a function that takes auth params & a gRPC argument sounds good to me 👍

@jmuk
Copy link
Contributor Author

jmuk commented Aug 12, 2016

Uploaded a new patchset. It relies on a patch for google-gax at jmuk/gax-nodejs@65052ae

@jmdobry
Copy link
Contributor

jmdobry commented Aug 15, 2016

grpc is no longer exported? How would the user get access to the grpc stub?

@jmuk
Copy link
Contributor Author

jmuk commented Aug 15, 2016

I think normally users won't access to grpc stubs, but in case they want, grpc will be exported like

var language = require('@google-cloud/language');
var v1beta1 = language.v1beta1({keyFile: ...});
var stub = new v1beta1.grpc.LanguageService(...);

@jmuk
Copy link
Contributor Author

jmuk commented Aug 15, 2016

Another idea for grpc came up through our offline-discussion, simply grpc objects and Gapic-generated code are in the same namespace, i.e.

var language = require('@google-cloud/language');
var v1beta1 = language.v1beta1({keyFile: ...});
var api = v1beta1.LanguageServiceApi();
api.annotateText({type: v1beta1.Document.Type.PLAIN_TEXT, ...});
// not v1beta1.grpc.Document.Type.PLAIN_TEXT

@jmdobry
Copy link
Contributor

jmdobry commented Aug 15, 2016

var stub = new v1beta1.grpc.LanguageService(...);

Got it.

@jmdobry
Copy link
Contributor

jmdobry commented Aug 16, 2016

So are we pretty satisfied with how this works? Can your manual edits be automated at all? The manual stuff seems pretty minimal, seems like it could be automated.

* @param {String} opts.appVersion
* The version of the calling service.
*/
function LanguageServiceApi(opts) {

This comment was marked as spam.

This comment was marked as spam.

var gaxGrpc = gax.grpc(options);
var result = {};
extend(result, languageServiceApi(gaxGrpc));
return result;

This comment was marked as spam.

This comment was marked as spam.

@jmdobry
Copy link
Contributor

jmdobry commented Aug 16, 2016

v1beta1/index.js: manually written
index.js and package.json: manually modified

  • v1beta1/index.js - does this really need to be written manually?
  • index.js - Check for string Language.v1beta1 = require('./v1beta1');, if not there append to file. It's okay for it to be after module.exports = Language.
  • package.json - NPM can update this for you, just have the generator run npm install --save arguejs@0.2.3 google-gax@0.6.0 in the language directory.

@jmuk
Copy link
Contributor Author

jmuk commented Aug 16, 2016

v1beta1/index.js - does this really need to be written manually?

This can be generated automatically, but I manually created to keep this discussion.
I'll set up a generator after we settle down the basic structure.

index.js - Check for string Language.v1beta1 = require('./v1beta1');, if not there append to file. It's okay for it to be after module.exports = Language.

I may miss your point, but modified index.js so that the edit is simply an addition of a line to index.js, and then this edit can also be automatically generated.

package.json - NPM can update this for you, just have the generator run npm install --save arguejs@0.2.3 google-gax@0.6.0 in the language directory.

Thanks. Will do in the next API (probably adding that in the code generation pipeline).

@jmdobry
Copy link
Contributor

jmdobry commented Aug 17, 2016

I may miss your point, but modified index.js so that the edit is simply an addition of a line to index.js, and then this edit can also be automatically generated.

Your edit should work fine.

jmuk added 7 commits August 19, 2016 19:22
- Move API service class to a toplevel object
- index.js creates a auth context (through GAX) and apply it to
  the generated code. This way, the same context will be shared
  among multiple instances if an API consists of multiple service
  classes.
@jmuk
Copy link
Contributor Author

jmuk commented Aug 20, 2016

Are there any other additional comments on #1476 (comment) or #1476 (comment)? Are there anything I'm missing?

@stephenplusplus
Copy link
Contributor

stephenplusplus commented Aug 22, 2016

I think we're set. We just need to find a way to keep our unit test coverage at 100%.

We will need a unit test that checks module.exports.v1beta1 is being exported properly from packages/language/src/index.js by creating a test in packages/language/test/index.js.

And since we aren't worried about writing tests for the generated files, we'll have to find a way to have our linter skip those files. The easiest way to do that will probably be from using a naming convention and adding it to .jshintignore

So maybe we want to add:

packages/*/src/v* // catch all version directories

Or to make it a little more robust, maybe we would place generated files in their own directory to allow:

packages/*/src/generated // or something similar

@jmdobry
Copy link
Contributor

jmdobry commented Aug 22, 2016

Wouldn't we want the generated code to pass JSHint? At the least it could catch undefined variables, etc., especially since we won't have actual tests for the generated code.

@stephenplusplus
Copy link
Contributor

Hmm, maybe I'm getting my dev tools mixed up. What's causing the coverage drop?

@@ -31,7 +31,7 @@
"lint": "jshint scripts/ packages/ system-test/ test/ && jscs packages/ system-test/ test/",
"test": "npm run docs && npm run bundle && mocha test/docs.js packages/*/test/*.js",
"system-test": "mocha packages/*/system-test/*.js --no-timeouts --bail",
"cover": "istanbul cover _mocha --report lcovonly -- --no-timeouts --bail packages/*/test/*.js -R spec",
"cover": "istanbul cover _mocha --report lcovonly -x 'packages/*/src/v[0-9]*/*.js' -- --no-timeouts --bail packages/*/test/*.js -R spec",

This comment was marked as spam.

This comment was marked as spam.

@jmuk
Copy link
Contributor Author

jmuk commented Aug 23, 2016

Note that JSHint (and JSCS) doesn't exclude the auto-generated files. I also found their output helpful for me to fix the code generator.

@stephenplusplus
Copy link
Contributor

It's all looking good to me. Travis failed because of our repo rename.

Anything left to do? // @jmdobry @jmuk @callmehiphop

@coveralls
Copy link

coveralls commented Aug 23, 2016

Coverage Status

Changes Unknown when pulling cddd4eb on jmuk:language into * on GoogleCloudPlatform:master*.

@jmdobry
Copy link
Contributor

jmdobry commented Aug 23, 2016

LGTM

@jmuk
Copy link
Contributor Author

jmuk commented Aug 23, 2016

LGTM too

@stephenplusplus stephenplusplus merged commit ea3dae8 into googleapis:master Aug 25, 2016
@stephenplusplus
Copy link
Contributor

Thanks! Now we just need to figure out the docs: #1492

jmuk added a commit to jmuk/gcloud-node that referenced this pull request Aug 31, 2016
jmuk added a commit to jmuk/packman that referenced this pull request Sep 7, 2016
- retire use_pbjs flag -- it's not used anymore, and it does not
  fit with the new pattern of Gapic code.
- update the grpc package template: this fits with the proto packages
  used by gcloud-node.
- update gax package template: this will work well with the new
  pattern of Gapic code, as you can see in googleapis/gapic-generator#392
  or googleapis/google-cloud-node#1476
jmuk added a commit to googleapis/packman that referenced this pull request Sep 8, 2016
- retire use_pbjs flag -- it's not used anymore, and it does not
  fit with the new pattern of Gapic code.
- update the grpc package template: this fits with the proto packages
  used by gcloud-node.
- update gax package template: this will work well with the new
  pattern of Gapic code, as you can see in googleapis/gapic-generator#392
  or googleapis/google-cloud-node#1476
- add 'env' parameter to dependency_out protoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants