Update with feedback from gunan

3 files changed, 31 insertions, 14 deletions

diff --git a/tensorflow/tools/dockerfiles/Dockerfile b/tensorflow/tools/dockerfiles/Dockerfile
index e8ca012298..1d0dc3247d 100644
--- a/tensorflow/tools/dockerfiles/Dockerfile
+++ b/tensorflow/tools/dockerfiles/Dockerfile
@@ -1,4 +1,8 @@
-FROM hadolint/hadolint:latest-debian
+# TensorFlow Dockerfile Development Container
+#
+# You can use this image to quickly develop changes to the Dockerfile assembler
+# or set of TF Docker partials. See README.md for usage instructions.
+FROM debian:stretch
 LABEL maintainer="Austin Anderson <angerson@google.com>"
 
 RUN apt-get update && apt-get install -y python3 python3-pip bash
diff --git a/tensorflow/tools/dockerfiles/README.md b/tensorflow/tools/dockerfiles/README.md
index 4786f8ec81..ed026c20a0 100644
--- a/tensorflow/tools/dockerfiles/README.md
+++ b/tensorflow/tools/dockerfiles/README.md
@@ -19,10 +19,19 @@ in the Dockerfile itself.
 
 ## Running
 
-After building the image with the tag `tf` (for example):
+After building the image with the tag `tf` (for example), use `docker run` to
+run the images. Examples are below.
+
+Note for new Docker users: the `-v` and `-u` flags share directories between
+the Docker container and your machine, and very important. Without
+`-v`, your work will be wiped once the container quits, and without `-u`, files
+created by the container will have the wrong file permissions on your host
+machine. If you are confused, check out the [Docker run
+documentation](https://docs.docker.com/engine/reference/run/).
 
 ```bash
-# A volume mount is optional but highly recommended, especially for Jupyter
+# A volume mount (-v) is optional but highly recommended, especially for Jupyter
+# User permissions (-u) are required if you use (-v).
 
 # CPU-based images
 $ docker run -u $(id -u):$(id -g) -v $(PWD):/my-devel -it tf
@@ -32,13 +41,12 @@ $ docker run --runtime=nvidia -u $(id -u):$(id -g) -v $(PWD):/my-devel -it tf
 
 # Images with Jupyter run on port 8888, and needs a volume for notebooks
 $ docker run --user $(id -u):$(id -g) -p 8888:8888 -v $(PWD):/notebooks -it tf
-
-# Development images
-$ docker run --user $(id -u):$(id -g) -it tf
-docker$ git clone https://github.com/tensorflow/tensorflow
 ```
 
-## Maintaining
+These images do not come with the TensorFlow source code -- but the development
+images have git included, so you can `git clone` it yourself.
+
+## Contributing
 
 To make changes to TensorFlow's Dockerfiles, you'll update `spec.yml` and the
 `*.partial.Dockerfile` files in the `partials` directory, then run
diff --git a/tensorflow/tools/dockerfiles/assembler.py b/tensorflow/tools/dockerfiles/assembler.py
index 8e0e5923d6..8dd6cea720 100644
--- a/tensorflow/tools/dockerfiles/assembler.py
+++ b/tensorflow/tools/dockerfiles/assembler.py
@@ -34,8 +34,8 @@ flags.DEFINE_string(
 
 flags.DEFINE_string(
     'output_dir',
-    '.', ('Path to an output directory for Dockerfiles. '
-          'Will be created if it doesn\'t exist.'),
+    'dockerfiles', ('Path to an output directory for Dockerfiles. '
+                    'Will be created if it doesn\'t exist.'),
     short_name='o')
 
 flags.DEFINE_string(
@@ -130,8 +130,9 @@ images:
 class TfDockerValidator(cerberus.Validator):
   """Custom Cerberus validator for TF dockerfile spec.
 
-  Note that each custom validator's docstring must end with a segment describing
-  its own validation schema.
+  Note: Each _validate_foo function's docstring must end with a segment
+  describing its own validation schema, e.g. "The rule's arguments are...". If
+  you add a new validator, you can copy/paste that section.
   """
 
   def _validate_ispartial(self, ispartial, field, value):
@@ -275,12 +276,16 @@ def construct_contents(partial_specs, image_spec):
         default = ''
       partial_contents = re.sub(r'ARG {}.*'.format(arg), 'ARG {}{}'.format(
           arg, default), partial_contents)
+
+    # Store updated partial contents
     processed_partial_strings.append(partial_contents)
+
+  # Join everything together
   return '\n'.join(processed_partial_strings)
 
 
-# Create a directory and its parents, even if it already exists
 def mkdir_p(path):
+  """Create a directory and its parents, even if it already exists."""
   try:
     os.makedirs(path)
   except OSError as e:
@@ -486,7 +491,7 @@ def construct_dockerfiles(tf_spec):
 
 def main(argv):
   if len(argv) > 1:
-    raise app.UsageError('Too many command-line arguments.')
+    raise app.UsageError('Unexpected command line args found: {}'.format(argv))
 
   with open(FLAGS.spec_file, 'r') as spec_file:
     tf_spec = yaml.load(spec_file)