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

Python code generation for protobufs and gRPC #6974

Merged
merged 43 commits into from
Jan 18, 2019
Merged

Python code generation for protobufs and gRPC #6974

merged 43 commits into from
Jan 18, 2019

Conversation

marcinex7
Copy link
Contributor

Problem

I would like to have a task, to generate python code from protobufs and grpc services.

Solution

There is a new codegen task grpcio-run, to execute python's grpcio library https://grpc.io/ and generate python code from .proto files.

Example:

Respectful examples can be found in /examples/src/python/example/grpcio

to create a gRPC server execute
./pants run examples/src/python/example/grpcio/server

and when it's running, run client example:
./pants run examples/src/python/example/grpcio/client

generated code can be found as usual in pants output directory:
/.pants.d/gen/grpcio-run/current/examples.src.protobuf.org.pantsbuild.example.service.service/current/org/pantsbuild/example/service

@stuhood
Copy link
Member

stuhood commented Dec 28, 2018

@marcinex7 : Thanks for the contribution!

A few of the CI failures look relevant: you can fetch the failing targets and logs using: build-support/bin/get_failing_travis_targets_for_pr.sh 6974.

@marcinex7
Copy link
Contributor Author

Thank you @stuhood for that hint 👍

I'm still not sure, what's causing those failures. Need to play a bit with grpcio library versions

@marcinex7
Copy link
Contributor Author

marcinex7 commented Jan 3, 2019

Any thoughts, why at some tests I am getting wheel translation errors? It seems to work for most of the checks, but in 3 of them, tests are failing with the following error:

[...]
                     E   	    raise Untranslateable('Package %s is not translateable by %s' % (package, translator))
                     E
                     E   	Exception message: Package SourcePackage(u'file:///Users/marcinpodolski/.cache/pants/python_cache/requirements/CPython-2.7.10/grpcio-1.17.1.tar.gz') is not translateable by ChainedTranslator(WheelTranslator, EggTranslator, SourceTranslator)

this grpcio package, is prepared on PyPi for Python 2.7 and Python 3.x (https://grpc.io/docs/quickstart/python.html), and I was playing with different versions - all have the same error like mentioned above.

@stuhood
Copy link
Member

stuhood commented Jan 4, 2019

@marcinex7 : Thanks again for the contribution!

I've looked into this a bit: at least one of the tests that is failing seems to be failing to build the grpcio pex (and I'm able to reproduce the failure locally).

A fair amount has changed in master recently: the src/python/pants/backend/codegen/grpcio/grpcio_prep.py Task you added looks like it was modeled after some older code, and should be replaced with the facility that we're using for conan: PythonToolBase:
https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/native/subsystems/conan.py

I'm not sure that this will resolve the issue bootstrapping the tool for multiple platforms, but I have a suspicion that it should be a blocker for landing anyway, so worth fixing now and seeing whether it resolves the CI issue.

@stuhood stuhood requested review from stuhood and benjyw January 4, 2019 18:10
@stuhood
Copy link
Member

stuhood commented Jan 8, 2019

@marcinex7 : Looks like you had a few flaky tests, but CI is otherwise likely to be green. Huzzah!

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great! There are two substantial comments left from my last review: if you could resolve those, we can land this.

src/python/pants/backend/codegen/grpcio/grpcio.py Outdated Show resolved Hide resolved
src/python/pants/backend/codegen/grpcio/grpcio.py Outdated Show resolved Hide resolved
src/python/pants/backend/codegen/grpcio/grpcio_prep.py Outdated Show resolved Hide resolved
from pants.util.contextutil import pushd


class GrpcioRun(SimpleCodegenTask):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ this is still an issue.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great!

@stuhood
Copy link
Member

stuhood commented Jan 11, 2019

The OSX shard failure is odd. Have restarted it.

@marcinex7
Copy link
Contributor Author

image

@stuhood do you have any idea, why grpcio-prep phase takes more than 10 minutes on CI, while on my mac it takes around 3 seconds...? I am executing same command as on CI:
./build-support/bin/ci.sh

I was playing with different grpcio versions, and merged master just today.

@stuhood
Copy link
Member

stuhood commented Jan 15, 2019

@marcinex7 : Unfortunately I don't have time to look into this today, but to debug that shard, you should be able to edit .travis.yml to set PEX_VERBOSE=9. Locally, something like:

PEX_VERBOSE=9 ./pants gen examples/src/protobuf/org/pantsbuild/example/grpcio::

...results in the following gist, with additional output for the grpcio-prep section: https://gist.github.com/stuhood/e098b5d79bbd657e71b0c2a6f6b44dff

@marcinex7
Copy link
Contributor Author

@stuhood Thanks for the tip! 👍
I've added verbosity to PEX, and immediately build passed... But it's just because there is some log from time to time. Anyway, it's not normal, that this pex creation takes around 12 minutes... especially, that locally it's around few seconds...

@marcinex7
Copy link
Contributor Author

@stuhood finally I have all checks passed! 👍 Can you please, have a look once again into this PR, and merge it if it's fine?

@@ -15,3 +16,10 @@ class GrpcioInstance(PythonToolInstance):
class GrpcioPrep(PythonToolPrepBase):
tool_subsystem_cls = Grpcio
tool_instance_cls = GrpcioInstance

def execute(self):
targets = self.get_targets(lambda target: isinstance(target, PythonGrpcioLibrary))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh. I'm fine with this!

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for continuing to iterate on this PR!

@stuhood stuhood merged commit 32dc915 into pantsbuild:master Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants