-
Notifications
You must be signed in to change notification settings - Fork 6.6k
Endpoints GRPC Python Sample #852
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall.
|
||
# Copy Server | ||
COPY . /hello/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, dont use ADD
. Should look something like:
COPY requirements.txt /tmp/
RUN pip install --requirement /tmp/requirements.txt
COPY . /tmp/
see: https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/#add-or-copy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
RUN pip install -r requirements.txt | ||
|
||
EXPOSE 8000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this container is exposing 50051, but listing it is not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1 @@ | |||
[This code's documentation lives on the grpc.io site.](http://www.grpc.io/docs/quickstart/python.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why keep the code under helloworld/
? Doesn't make sense with only one sample. Either way this README is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the helloworld/ subdirectory and moved files to getting-started-grpc/ directory
@@ -0,0 +1,37 @@ | |||
# Copyright 2017, Google Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think Python devs use bazel too much. Makes sense to remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
option java_package = "io.grpc.examples.helloworld"; | ||
option java_outer_classname = "HelloWorldProto"; | ||
option objc_class_prefix = "HLW"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove these options for java and objc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ENV VIRTUAL_ENV -p python3.5 /env | ||
ENV PATH /env/bin:$PATH | ||
|
||
COPY requirements.txt /tmp/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use /app instead of /tmp.
The base image actually sets workdir to /app anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
1. Generate the `out.pb` from the proto file: | ||
|
||
```bash | ||
python -m grpc_tools.protoc --include_imports --include_source_info -I protos protos/helloworld.proto --descriptor_set_out out.pb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeffmendoza is out.pb
the same filename we're using in the other examples? It seems like a bad name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonparrott out.pb is what is used in the Java canonical sample (https://github.com/GoogleCloudPlatform/java-docs-samples/tree/master/endpoints/getting-started-grpc0). do you wanna change it to something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine, I was just questioning Jeff, but he's on leave now so it's whatever. :)
|
||
"""The Python implementation of the GRPC helloworld.Greeter client.""" | ||
|
||
from __future__ import print_function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
metadata.append(('x-api-key', api_key)) | ||
response = stub.SayHello( | ||
helloworld_pb2.HelloRequest(name='you'), metadata=metadata) | ||
print("Greeter client received: " + response.message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
helloworld_pb2_grpc.add_GreeterServicer_to_server(Greeter(), server) | ||
server.add_insecure_port('[::]:50051') | ||
server.start() | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here:
# gRPC starts a new thread to service requests. Just make the main thread sleep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
while True: | ||
time.sleep(_ONE_DAY_IN_SECONDS) | ||
except KeyboardInterrupt: | ||
server.stop(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the 0
here mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://www.grpc.io/grpc/python/grpc.html#grpc.Server.stop
"grace – A duration of time in seconds or None. If a duration of time in seconds, the time to allow existing RPCs to complete before being aborted by this Server’s stopping. If None, all RPCs will be aborted immediately and this method will block until this Server is completely stopped."
Let me know what you want to do with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use stop(grace=0)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@ryanmats nox should pass lint now. :) |
@dpebot will you merge when travis is green? pretty please? |
* chore(deps): update all dependencies * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Note: There are linting issues withe helloworld_pb2.py and helloworld_pb2_grpc.py but we plan on ignoring these issues since they are generated code.