Only build common protos with --grpc option for LRO. #109

bjwatson · 2016-09-07T20:32:17Z

Updates googleapis/google-cloud-python#1893 (comment)

grpc/grpc#8056 is a proposal to fix this issue in gRPC (FYI @jmuk)

bjwatson · 2016-09-07T20:35:06Z

@jmuk @garrettjonesgoogle @jskeet @vchudnov-g @pongad, do any of your languages have standalone googleapis-common-protos packages? If so, and if you're adding LRO service methods to it, please check in with me. I've been through a few battles with it already in Python, which might be relevant to you.

codecov-io · 2016-09-07T20:35:23Z

Current coverage is 91.73% (diff: 75.00%)

Merging #109 into master will increase coverage by 0.03%

@@             master       #109   diff @@
==========================================
  Files             3          3          
  Lines           711        714     +3   
  Methods         150        150          
  Messages          0          0          
  Branches        111        113     +2   
==========================================
+ Hits            652        655     +3   
  Misses           59         59          
  Partials          0          0

Powered by Codecov. Last update 498dfc7...cc1f8bc

jmuk · 2016-09-07T20:45:38Z

lib/api_repo.js

-          } else {
-            args.push('--grpc_out=' + outDir);
+          if (!opts.buildCommonProtos ||
+              _.includes(COMMON_GRPC_PROTOS, protoPath)) {


isn't this the path from the proto's rootdir (i.e. 'google/longrunning/operations.proto')?

No, it's just the last element. pkg has the path.

jmuk · 2016-09-07T20:47:32Z

This looks a bit weird, could be a bug of grpc tools for Python. Are there any bugs in grpc?
Might be worth filing a new one if not, and noted the link as a comment.

bjwatson · 2016-09-07T21:40:42Z

@jmuk Does gRPC codegen make the grpc imports conditional on actually using gRPC for other languages?

bjwatson · 2016-09-07T22:28:33Z

@jmuk I checked Ruby and saw that it generates the message and services in different files, which avoids this problem. @garrettjonesgoogle told me the same thing for Java, thanks to their rigid one-class-per-file policy.

jmuk · 2016-09-07T23:21:03Z

That's right (although I didn't know what Java does). If Python generator needs to generate single file containing both message and service definition -- then, Python generator should add grpc import line conditionally.
To be clear, I'm not against this PR, this PR will be good under the current situation. I'm saying this situation would be better to be recognized by the grpc-python team too.

jskeet · 2016-09-08T05:44:23Z

@bjwatson: Our "common types" (Google.Api.CommonProtos) package includes the LRO definition, but not any gRPC code for it. I'd anticipate putting that in a separate package which then depends on Google.Api.CommonProtos. At the moment, I don't think any of these packages are produced by the pipeline, although we may well want to change this at some point...

garrettjonesgoogle · 2016-09-08T16:31:35Z

@bjwatson , java has google-grpc-common-protos, which is including the Operations Grpc generated code now.

bjwatson · 2016-09-08T17:53:35Z

@garrettjonesgoogle So do you have two packages for the Java common protos: one with just the messages, and one with the full gRPC build? Where can I find them?

bjwatson · 2016-09-08T17:55:38Z

@jskeet A potential problem with that approach is if the Google.Api.CommonProtos already has the LRO messages (like the Python googleapis-common-protos package does), and then the gRPC generated code attempts to duplicate the messages in another package that depends on Google.Api.CommonProtos (the Python gRPC compiler generates both messages and service methods in the same file for a given .proto).

jskeet · 2016-09-08T18:40:26Z

The gRPC proto plugin for C# only generates the services - we should be fine.

garrettjonesgoogle · 2016-09-08T18:58:25Z

@bjwatson , we have a single package with both the common protos and the gRPC-generated class for Operations. http://search.maven.org/#artifactdetails%7Ccom.google.api.grpc%7Cgrpc-google-common-protos%7C0.0.9%7Cjar

bjwatson · 2016-09-08T23:50:48Z

@garrettjonesgoogle So googleapis-common-protos has been abandoned?

I assume you don't have any concerns about adding a gRPC dependency, because gRPC works fine with the Java version of GAE Standard, right?

garrettjonesgoogle · 2016-09-08T23:58:45Z

@bjwatson Yes, the java repo googleapis-common-protos is abandoned now.

I haven't tried this with GAE standard; theoretically it shouldn't cause an issue to have dependencies that are unused (although if someone tried to use the gRPC dependency directly, I think it wouldn't work).

bjwatson · 2016-09-09T00:06:46Z

@garrettjonesgoogle I was having problems with our gcloud-python users on GAE Standard (e.g. googleapis/google-cloud-python#1893 (comment)), so I'm having to do more to make gRPC a soft dependency in the Python version of googleapis-common-protos.

Hopefully you don't run into the same problems. Has gcloud-java done a release yet that uses your new package? I ran into my issue promptly since I released a new version of the same package, and gcloud-python pulled it in.

garrettjonesgoogle · 2016-09-09T00:10:37Z

@bjwatson google-cloud-java has had an indirect dependency on grpc through gax-java since April: googleapis/google-cloud-java@2cd4b0c - so, I'm guessing it hasn't been a problem.

jskeet · 2016-09-09T06:07:13Z

Including the gRPC dependency in C# would be less desirable, I think - it's a large package due to the native binaries involved, so anywhere you don't need it, you really don't want it. That said, if all the other platforms take that approach, there are benefits in consistency, and there are relatively few places you'd want the common protos without gRPC at the moment...

bjwatson · 2016-09-09T15:40:32Z

@jskeet The primary driver right now for this in Python and other languages is supporting Speech v1beta1. The AsyncRecognize method is the primary method for the API that we need to support, and it returns an LRO object. Our gcloud counterparts need the gRPC service methods for LRO to make this work.

There will probably also be other APIs that drive the need for LRO.

bjwatson · 2016-09-09T15:47:17Z

@garrettjonesgoogle That's good to hear. google-cloud-python has always had a soft dependency on gax-python, to avoid pulling in gRPC for HTTP1.1-only clients. Seems like Java doesn't have that requirement.

jskeet · 2016-09-09T15:53:20Z

@bjwatson: Yes, we need LRO in C# as well. The options are:

Include the LRO and IAM gRPC APIs within Google.Api.CommonProtos (required Grpc.Core dependency in Google.Api.CommonProtos).
Have separate Google.Longrunning and Google.Iam.V1 packages

We definitely need it one way or another; it's just which route we go down. But maybe that would be better discussion away from this PR.

bjwatson · 2016-09-09T17:54:34Z

@jskeet Sounds good. We've been wrestling with both LRO and IAM in the other languages, too. Your choice of venue to continue this conversation. :)

bjwatson · 2016-09-10T00:11:55Z

@jmuk @geigerj I made your suggested changes, and fixed the code coverage error. PTAL

geigerj · 2016-09-12T20:44:04Z

lib/api_repo.js

+              _.includes(COMMON_GRPC_PROTOS, protoPath)) {
+            if (language === 'python') {
+              // required per https://github.com/grpc/grpc/issues/7857
+              args.push('--grpc_python_out=' + outDir);


I thought we decided not to do this in order to support GAE?

My understanding was that this required a more complex change to generate both gRPC and gRPC-less modules.

After offline discussion, I understand that this is actually an improvement over the current state of master, where every common proto is being built with the gRPC plugin (and hence is potentially GAE-incompatible).

jmuk · 2016-09-12T20:50:28Z

LGTM, with a nit.

geigerj · 2016-09-12T20:54:31Z

LGTM, although would like to get to a state without hand-edits as soon as we can to reduce the risk of accidents when regenerating the package.

bjwatson · 2016-09-12T20:56:22Z

@geigerj Agreed. #112 covers this, and it would be a good idea to fix it before we do our next release of common-protos.

bjwatson · 2016-09-12T20:57:56Z

@jmuk I fixed your nit. PTAL

jmuk · 2016-09-12T22:25:16Z

LGTM

Only build common protos with --grpc option for LRO.

1755918

bjwatson assigned jmuk and geigerj Sep 7, 2016

geigerj mentioned this pull request Sep 7, 2016

Support Cloud Client Libraries on Google App Engine standard googleapis/google-cloud-python#1893

Closed

jmuk reviewed Sep 7, 2016
View reviewed changes

jmuk mentioned this pull request Sep 7, 2016

Bump version for new release #108

Merged

Increase test coverage

7c1ba99

bjwatson force-pushed the selective-grpc branch from 84f6383 to 7c1ba99 Compare September 9, 2016 23:48

Brian J. Watson added 2 commits September 9, 2016 16:53

Latest released version of googleapis-common-protos is 1.3.4

6d0e042

Add bytestream.proto per request from @jmuk

b571542

geigerj reviewed Sep 12, 2016
View reviewed changes

Fix a nit

cc1f8bc

bjwatson merged commit 5763ff7 into googleapis:master Sep 12, 2016

bjwatson deleted the selective-grpc branch September 12, 2016 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only build common protos with --grpc option for LRO. #109

Only build common protos with --grpc option for LRO. #109

bjwatson commented Sep 7, 2016 •

edited

Loading

bjwatson commented Sep 7, 2016

codecov-io commented Sep 7, 2016 •

edited

Loading

jmuk Sep 7, 2016

bjwatson Sep 7, 2016

jmuk commented Sep 7, 2016

bjwatson commented Sep 7, 2016

bjwatson commented Sep 7, 2016

jmuk commented Sep 7, 2016

jskeet commented Sep 8, 2016

garrettjonesgoogle commented Sep 8, 2016

bjwatson commented Sep 8, 2016

bjwatson commented Sep 8, 2016

jskeet commented Sep 8, 2016 •

edited

Loading

garrettjonesgoogle commented Sep 8, 2016

bjwatson commented Sep 8, 2016 •

edited

Loading

garrettjonesgoogle commented Sep 8, 2016

bjwatson commented Sep 9, 2016

garrettjonesgoogle commented Sep 9, 2016

jskeet commented Sep 9, 2016

bjwatson commented Sep 9, 2016 •

edited

Loading

bjwatson commented Sep 9, 2016

jskeet commented Sep 9, 2016

bjwatson commented Sep 9, 2016

bjwatson commented Sep 10, 2016

geigerj Sep 12, 2016

geigerj Sep 12, 2016 •

edited

Loading

jmuk commented Sep 12, 2016

geigerj commented Sep 12, 2016

bjwatson commented Sep 12, 2016

bjwatson commented Sep 12, 2016

jmuk commented Sep 12, 2016

Only build common protos with --grpc option for LRO. #109

Only build common protos with --grpc option for LRO. #109

Conversation

bjwatson commented Sep 7, 2016 • edited Loading

bjwatson commented Sep 7, 2016

codecov-io commented Sep 7, 2016 • edited Loading

Current coverage is 91.73% (diff: 75.00%)

jmuk Sep 7, 2016

Choose a reason for hiding this comment

bjwatson Sep 7, 2016

Choose a reason for hiding this comment

jmuk commented Sep 7, 2016

bjwatson commented Sep 7, 2016

bjwatson commented Sep 7, 2016

jmuk commented Sep 7, 2016

jskeet commented Sep 8, 2016

garrettjonesgoogle commented Sep 8, 2016

bjwatson commented Sep 8, 2016

bjwatson commented Sep 8, 2016

jskeet commented Sep 8, 2016 • edited Loading

garrettjonesgoogle commented Sep 8, 2016

bjwatson commented Sep 8, 2016 • edited Loading

garrettjonesgoogle commented Sep 8, 2016

bjwatson commented Sep 9, 2016

garrettjonesgoogle commented Sep 9, 2016

jskeet commented Sep 9, 2016

bjwatson commented Sep 9, 2016 • edited Loading

bjwatson commented Sep 9, 2016

jskeet commented Sep 9, 2016

bjwatson commented Sep 9, 2016

bjwatson commented Sep 10, 2016

geigerj Sep 12, 2016

Choose a reason for hiding this comment

geigerj Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

jmuk commented Sep 12, 2016

geigerj commented Sep 12, 2016

bjwatson commented Sep 12, 2016

bjwatson commented Sep 12, 2016

jmuk commented Sep 12, 2016

bjwatson commented Sep 7, 2016 •

edited

Loading

codecov-io commented Sep 7, 2016 •

edited

Loading

jskeet commented Sep 8, 2016 •

edited

Loading

bjwatson commented Sep 8, 2016 •

edited

Loading

bjwatson commented Sep 9, 2016 •

edited

Loading

geigerj Sep 12, 2016 •

edited

Loading