add grpc bookstore python samples by wenchenglu · Pull Request #660 · GoogleCloudPlatform/python-docs-samples

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

+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

+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

+
+## 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

+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

+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

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


No need for the _.

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

+  return store
+
+
+def _serve():


No need for the _.

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

+
+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

+
+  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

@@ -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

@@ -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

+    && 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

+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

+      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

+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

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


Use single quotes for string throughout (except docstrings)

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

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


wow, nice catch :)

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

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


Move argpase stuff down here?

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

+  """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

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


grpc_context instead of ctx?

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

ptal

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

+
+    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

ghost

Getting close.

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

@@ -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?

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

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

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

removed grpc beta interface, PTAL

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

+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?

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

+
+  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

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

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


s/GRPC/gRPC.

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

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


You can remove this class now.

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

+"""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"?

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

+  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).

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

+
+  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.

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

@@ -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?

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

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


Still using beta here?

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

PTAL

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

+import grpc
+
+
+Code = grpc.StatusCode


Is this used anywhere?

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

+# 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

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

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

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

done, ptal

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

@dpebot can you merge when travis passes?

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

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

ghost · 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

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

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

ghost · 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.

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

@waprin

fixed lint errors as you suggested, PTAL

ghost · 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

Wencheng Lu 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

Wencheng Lu 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

Wencheng Lu 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

Comment thread endpoints/bookstore-grpc/status.py Outdated

import grpc

Code = grpc.StatusCode

ghost Nov 17, 2016

Copy link
Copy Markdown

Contributor

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

chalmerlowe mentioned this pull request Feb 18, 2026

chore: migrate code from googleapis/python-datastore #13837

Merged

9 tasks

chalmerlowe mentioned this pull request Apr 7, 2026

migrate code from googleapis/python-storage #13989

Merged

9 tasks


		## 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

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!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Nov 17, 2016

Choose a reason for hiding this comment