add grpc bookstore python samples #660

wenchenglu · 2016-11-16T06:36:17Z

No description provided.

wenchenglu · 2016-11-16T17:50:51Z

@jonparrott
PTAL

wenchenglu · 2016-11-16T17:51:56Z

just noticed the travis errors, will fix those and ping for review soon.

theacodes

Finished first pass. Please let me know when you're ready for another pass.

theacodes · 2016-11-16T17:52:50Z

endpoints/bookstore-grpc/Dockerfile

+ADD . /bookstore/
+
+RUN cd /bookstore \
+    && pip install --ignore-installed six \


Don't do this, use a virtualenv:

RUN virtualenv /env ENV VIRTUAL_ENV /env ENV PATH /env/bin:$PATH ADD . /bookstore/ WORKDIR /bookstore RUN pip install -r requirements.txt

theacodes · 2016-11-16T17:53:22Z

endpoints/bookstore-grpc/README.md

+You'll probably want to start by creating a virtual Python environment, so that
+you can install dependencies without installing them globally.  To do this, run:
+
+    mkdir bookstore-env


you don't need to do mkdir, virtualenv will do it for you.

theacodes · 2016-11-16T17:54:17Z

endpoints/bookstore-grpc/README.md

+
+## Installing Dependencies
+
+You'll probably want to start by creating a virtual Python environment, so that


Be definitive: Install the dependencies using virtualenv:

(Note: I will eventually replace this README with an autogenerated one, but I'll keep the protoc instructions and such)

theacodes · 2016-11-16T17:56:29Z

endpoints/bookstore-grpc/bookstore_client.py

+import bookstore_pb2
+
+from google.protobuf import empty_pb2
+from grpc.beta import implementations


Just import grpc, don't use the beta interface anymore.

theacodes · 2016-11-16T17:57:27Z

endpoints/bookstore-grpc/bookstore_client.py

+from grpc.beta import implementations
+
+
+def _add_endpoints_metadata(api_key):


Instead of doing nested functions and closures, prefer a callable class:

class ApiKeyMetadata(object): def __init__(self, api_key): self.api_key = api_key def __call__(self, metadata): ...

theacodes · 2016-11-16T18:08:34Z

endpoints/bookstore-grpc/bookstore_server.py

+      return struct_pb2.Value()
+
+
+def _create_sample_bookstore():


No need for the _.

theacodes · 2016-11-16T18:08:41Z

endpoints/bookstore-grpc/bookstore_server.py

+  return store
+
+
+def _serve():


No need for the _.

theacodes · 2016-11-16T18:09:00Z

endpoints/bookstore-grpc/bookstore_server.py

+
+def _serve():
+  """Configures and runs the bookstore API server."""
+  parser = argparse.ArgumentParser(


Apply all of my comments about the argparse section in the client to this argparse section as well.

theacodes · 2016-11-16T18:09:13Z

endpoints/bookstore-grpc/bookstore_server.py

+
+  store = _create_sample_bookstore()
+  server = bookstore_pb2.beta_create_Bookstore_server(BookstoreServicer(store))
+  server.add_insecure_port('[::]:%d' % args.port)


prefer format over %.

theacodes · 2016-11-16T18:09:46Z

endpoints/bookstore-grpc/requirements.txt

@@ -0,0 +1,3 @@
+# The sample bookstore has been built and tested with GRPC version
+# 1.0.0  It may work with other versions.
+grpcio == 1.0.0


Use the latest version.

wenchenglu · 2016-11-17T00:13:34Z

@jonparrott

Jon, thanks for you review, really nice comments. Ready for the second round of review now :)

theacodes · 2016-11-17T00:18:22Z

endpoints/bookstore-grpc/Dockerfile

@@ -0,0 +1,20 @@
+FROM debian:jessie


Our other endpoints sample uses the python runtime, any strong reason for not using it here?

theacodes · 2016-11-17T00:19:01Z

endpoints/bookstore-grpc/Dockerfile

+    && apt-get install -y python-dev python-pip virtualenv \
+    && rm -rf /var/lib/apt/lists/*
+
+RUN virtualenv /env


I should've mentioned this during the last review, but use python 3, please. virtualenv -p python3.4. If you use the Python runtime, use -p python3.5.

done, is this what you meant?

theacodes · 2016-11-17T00:19:49Z

endpoints/bookstore-grpc/bookstore.py

+class Bookstore(object):
+  """Stores and manipulates bookstore data."""
+
+  class ShelfDict(dict):


Don't do this, just catch the KeyErrors.

there are many places used that ShelfDict, catch all over the places?

There's three options:

Catch everywhere you access the dictionary.

Have a top-level catch of all exceptions and map them to gRPC errors (you're already kind of doing this)

Ignore exceptions and rely on gRPC's default behavior.

I played with this a bit, and think option 2 is the best which gives better errors with having to catch everywhere.

Let me know how you think.

theacodes · 2016-11-17T00:20:10Z

endpoints/bookstore-grpc/bookstore.py

+      raise status.StatusException(status.Code.NOT_FOUND,
+                                   'Unable to find book "%s"' % key)
+
+  class ShelfInfo(object):


Don't do nested classes, there is rarely a need for it.

do you mean move ShelfInfo to the top-level class?

theacodes · 2016-11-17T00:20:37Z

endpoints/bookstore-grpc/bookstore.py

+import status
+
+class Bookstore(object):
+  """Stores and manipulates bookstore data."""


Maybe a better description here? "An in-memory backend for storing Bookstore data."

theacodes · 2016-11-17T00:26:01Z

endpoints/bookstore-grpc/bookstore_server.py

+  store = bookstore.Bookstore()
+
+  shelf = bookstore_pb2.Shelf()
+  shelf.theme = "Fiction"


Use single quotes for string throughout (except docstrings)

theacodes · 2016-11-17T00:26:12Z

endpoints/bookstore-grpc/bookstore_server.py

+  (_, fiction) = store.create_shelf(shelf)
+
+  book = bookstore_pb2.Book()
+  book.title = "REAMDE"


wow, nice catch :)

theacodes · 2016-11-17T00:26:24Z

endpoints/bookstore-grpc/bookstore_server.py

+    server.stop(args.shutdown_grace_duration)
+
+
+if __name__ == '__main__':


Move argpase stuff down here?

theacodes · 2016-11-17T00:26:50Z

endpoints/bookstore-grpc/status.py

+  """A context manager that automatically handles StatusException."""
+  try:
+    yield
+  except StatusException as se:


how about just exc instead of se?

theacodes · 2016-11-17T00:27:09Z

endpoints/bookstore-grpc/status.py

+
+
+@contextmanager
+def context(ctx):


grpc_context instead of ctx?

wenchenglu · 2016-11-17T02:39:25Z

ptal

lizan · 2016-11-17T05:24:59Z

endpoints/bookstore-grpc/README.md

+
+    pip install -r requirements.txt
+
+Install the [GRPC libraries and tools](https://github.com/grpc/grpc/blob/v1.0.x/INSTALL.md)


Follow prequisites in GRPC Quickstart is better than the INSTALL.md

theacodes

Getting close.

theacodes · 2016-11-17T17:59:33Z

endpoints/bookstore-grpc/Dockerfile

@@ -0,0 +1,19 @@
+FROM gcr.io/google_appengine/python


Definitely link to the Python runtime page on github: https://github.com/GoogleCloudPlatform/python-runtime

do you mean adding a comment in this Dockerfile? something like this:

https://github.com/GoogleCloudPlatform/python-runtime

Or adding this link in the README.md?

theacodes · 2016-11-17T18:15:05Z

I still want to drop the usage of the beta interface.

wenchenglu · 2016-11-17T20:15:26Z

removed grpc beta interface, PTAL

theacodes · 2016-11-17T20:20:16Z

endpoints/bookstore-grpc/bookstore.py

+class Bookstore(object):
+  """An in-memory backend for storing Bookstore data."""
+
+  class ShelfDict(dict):


I thought we were going to remove ShelfDict and BookDict?

yes, you mentioned that. then you provided 3 options, and I think the option 2 is the best to avoid duplicate catch statement and still returning the useful errors. Do you have a better suggestion to do that without using wrapper dict?

theacodes · 2016-11-17T20:20:37Z

endpoints/bookstore-grpc/bookstore.py

+
+  def delete_book(self, shelf_id, book_id):
+    with self._lock:
+      _ = self._shelves[shelf_id]._books[book_id]


You can probably just remove this line.

I think so, done

theacodes · 2016-11-17T20:20:46Z

endpoints/bookstore-grpc/bookstore_client.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""The Python GRPC Bookstore Client Example."""


s/GRPC/gRPC.

theacodes · 2016-11-17T20:21:00Z

endpoints/bookstore-grpc/bookstore_client.py

+import bookstore_pb2
+
+
+class ApiKeyMetadata(object):


You can remove this class now.

theacodes · 2016-11-17T20:21:13Z

endpoints/bookstore-grpc/bookstore_client.py

+"""The Python GRPC Bookstore Client Example."""
+
+import argparse
+import grpc


grpc goes into the second section (third-party imports)

here? or next to "from google.protobuf import empty_pb2"?

theacodes · 2016-11-17T20:21:53Z

endpoints/bookstore-grpc/bookstore_client.py

+  channel = grpc.insecure_channel('{}:{}'.format(host,port))
+
+  stub = bookstore_pb2.BookstoreStub(channel)
+  shelves = stub.ListShelves(empty_pb2.Empty(), timeout,


In general, put a newline after the opening ( to avoid hanging indents. (please do this throughout).

theacodes · 2016-11-17T20:22:19Z

endpoints/bookstore-grpc/bookstore_server.py

+
+  shelf = bookstore_pb2.Shelf()
+  shelf.theme = 'Fiction'
+  (_, fiction) = store.create_shelf(shelf)


No need for these parens (there's a few more instances of this, please do it throughout)

done, and another place

after grep, only find two for statement using that now:
for (_, s) in self.shelves.iteritems()]
for (, book) in self._shelves[shelf_id]._books.iteritems()]

not sure if it is recommended to remove parens there

Yep, go ahead.

theacodes · 2016-11-17T20:22:42Z

endpoints/bookstore-grpc/requirements.txt

@@ -0,0 +1,3 @@
+# The sample bookstore has been built and tested with GRPC version


This comment really doesn't add a lot of value, remove it?

theacodes · 2016-11-17T20:22:51Z

endpoints/bookstore-grpc/status.py

+
+from contextlib import contextmanager
+
+from grpc.beta import interfaces


Still using beta here?

wenchenglu · 2016-11-17T22:34:11Z

PTAL

theacodes · 2016-11-17T22:47:35Z

endpoints/bookstore-grpc/status.py

+import grpc
+
+
+Code = grpc.StatusCode


Is this used anywhere?

theacodes · 2016-11-17T22:47:57Z

endpoints/bookstore-grpc/status.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import status


imports should be:

from contextlib import contextmanager import grpc import status

theacodes · 2016-11-17T22:48:16Z

A few final tiny things and this is good to go.

wenchenglu · 2016-11-17T22:51:55Z

done, ptal

theacodes · 2016-11-17T23:20:05Z

@dpebot can you merge when travis passes?

dpebot · 2016-11-17T23:20:06Z

Okay! I'll merge when all statuses are green.

wenchenglu · 2016-11-18T04:01:47Z

@jonparrott

I saw there are many flake8 lint errors. Do I have to manually fix them one by one? or is there some tool I can use? thanks

jeffmendoza · 2016-11-18T17:22:24Z

@ryanmats Jon is sick today, do you know about any flake8 tools to fix errors?

ryanmats · 2016-11-18T18:14:03Z

@jeffmendoza @wlu2016 i'm not aware of any flake8 tools to automatically fix errors, but there might be. Jon said he would be on chat from time to time today so maybe he'd know.

wenchenglu · 2016-11-18T19:51:09Z

@waprin

fixed lint errors as you suggested, PTAL

theacodes · 2016-11-19T04:17:24Z

@waprin's suggestion was great. Thanks!

wenchenglu added 2 commits November 15, 2016 22:06

Add bookstore grpc python samples

832e133

add bookstore.proto

e47fbf6

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 16, 2016

theacodes suggested changes Nov 16, 2016

View reviewed changes

Updated comments and clean up the code following the review comments

54f2273

theacodes suggested changes Nov 17, 2016

View reviewed changes

address review comments

18e58b1

wenchenglu added 2 commits November 16, 2016 20:17

fix a typo

7590e45

remove unnecessary ()

c06814f

lizan reviewed Nov 17, 2016

View reviewed changes

update grpc python quickstart

b9f1fdc

theacodes suggested changes Nov 17, 2016

View reviewed changes

wenchenglu added 2 commits November 17, 2016 11:11

update the code to address review comments

506b4cb

remove grpc beta interface

1dbcc66

Jon Wayne Parrott added 2 commits November 17, 2016 12:18

Expand comment in Dockerfile

65f7d68

Fix gRPC casing in README

1a19f36

theacodes suggested changes Nov 17, 2016

View reviewed changes

wenchenglu added 2 commits November 17, 2016 14:32

update code to address review comments

d811495

Merge branch 'master' of github.com:wlu2016/python-docs-samples

667e4c0

wenchenglu and others added 3 commits November 17, 2016 14:38

remove unnecessary import

ca82046

Fix small lint issue

d8f83e3

Fixing import order and small nits

aa5bff7

theacodes reviewed Nov 17, 2016

View reviewed changes

endpoints/bookstore-grpc/status.py

import grpc

Code = grpc.StatusCode

Copy link

Contributor

theacodes Nov 17, 2016

Choose a reason for hiding this comment

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

Is this used anywhere?

theacodes suggested changes Nov 17, 2016

View reviewed changes

fix import order

f4fab1c

theacodes approved these changes Nov 17, 2016

View reviewed changes

dpebot added the automerge Merge the pull request once unit tests and other checks pass. label Nov 17, 2016

fix lint errors

74da41a

dpebot merged commit 9036dcb into GoogleCloudPlatform:master Nov 18, 2016


		## Installing Dependencies

		You'll probably want to start by creating a virtual Python environment, so that

		from grpc.beta import implementations


		def _add_endpoints_metadata(api_key):

		server.stop(args.shutdown_grace_duration)


		if __name__ == '__main__':


		pip install -r requirements.txt

		Install the [GRPC libraries and tools](https://github.com/grpc/grpc/blob/v1.0.x/INSTALL.md)

		@@ -0,0 +1,3 @@
		# The sample bookstore has been built and tested with GRPC version


		from contextlib import contextmanager

		from grpc.beta import interfaces

add grpc bookstore python samples #660

add grpc bookstore python samples #660

Uh oh!

Conversation

wenchenglu commented Nov 16, 2016

Uh oh!

wenchenglu commented Nov 16, 2016

Uh oh!

wenchenglu commented Nov 16, 2016

Uh oh!

theacodes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenchenglu commented Nov 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenchenglu commented Nov 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theacodes left a comment

Choose a reason for hiding this comment

Uh oh!