Autograph guide #4733

MarkDaoust · 2018-07-10T16:34:14Z

Pull AutoGraph Workshop notebook into tensorflow.org/guide

Staging link:

https://colab.research.google.com/github/markdaoust/models/blob/autopgraph-guide/samples/core/guide/autograph_control_flow.ipynb

From tensorflow/tensorflow/blob/8a6ef2cb4f98bacc1f821f60c21914b4bd5faaef

lamberta · 2018-07-10T16:55:20Z

This is meant to be the guide page, yes? (Not a tutorial)
If so, can you rename autograph_control_flow.ipynb to autograph.ipynb.
Also, save notebook with open TOC.
Some of the heading hierarchy looks off.

mdanatg · 2018-07-10T16:58:55Z

samples/core/guide/autograph_control_flow.ipynb

+      },
+      "cell_type": "markdown",
+      "source": [
+        "Into graph-compatible functions like this:"


or graph-building, if you prefer (it feels difficult to explain what is a graph-compatible function)

Agree with Dan for using "graph-building".
Also, should avoid using "eager mode" and "graph mode" in this doc. Prefer "eager execution" and "graph execution".

mdanatg

Very nice! A few cosmetic details to double check.

mdanatg · 2018-07-10T17:00:09Z

samples/core/guide/autograph_control_flow.ipynb

+      },
+      "cell_type": "markdown",
+      "source": [
+        "You can take code written for eager execution and run it in graph mode. You get the same results, but with all the benfits of graphs:"


This block might be confusing, because we're not in eager mode in this cell. The code does run with the input 9.0, but it doesn't run any ops - it just does computations in Python. It might be simpler to just omit the cell and only leave the text note.

Good point.

Given that we're using Graph and Session contexts everywhere... it looks like we can just switch on eager_execution, and wrap these in tf.constants.

Is that a reasonable approach?

mdanatg · 2018-07-10T17:01:10Z

samples/core/guide/autograph_control_flow.ipynb

+        "  # You can inspect the graph using tf.get_default_graph().as_graph_def()\n",
+        "  g_ops = tf_g(tf.constant(9.0))\n",
+        "  with tf.Session() as sess:\n",
+        "    print('Autograph value: %2.2f\\n' % sess.run(g_ops))  "


Alternatively, could be also "Tensor value", if we want to avoid overbranding (at any rate, should be consistent with the subsequent cells).

Switched these to "Eager result" and "Graph result".

mdanatg · 2018-07-10T17:03:51Z

samples/core/guide/autograph_control_flow.ipynb

+        "with tf.Graph().as_default():  \n",
+        "  # The result works like a regular op: takes tensors in, returns tensors.\n",
+        "  # You can inspect the graph using tf.get_default_graph().as_graph_def()\n",
+        "  input = tf.placeholder(tf.int32)\n",


Nit: "input" is a python keyword, perhaps name the symbol "input_"?

Switched to "num" to match the argument name.

mdanatg · 2018-07-10T17:05:04Z

samples/core/guide/autograph_control_flow.ipynb

+      "source": [
+        "### Lists\n",
+        "\n",
+        "Appending to lists in loops also works (we create a `TensorArray` for you behind the scenes)"


This is no longer always the case. Rephrasing to this would be ok: "(we create tensor list ops for you behind the scenes)"

mdanatg · 2018-07-10T17:06:05Z

samples/core/guide/autograph_control_flow.ipynb

+        "def f(n):\n",
+        "  z = []\n",
+        "  # We ask you to tell us the element dtype of the list\n",
+        "  z = autograph.utils.set_element_type(z, tf.int32)\n",


Bit of a last-minute update: we no longer require to assign to z. So this would be better:

autograph.utils.set_element_type(z, tf.int32)

Side note: we're rolling out a change to allow the shorter form:

autograph.set_element_type(z, tf.int32)

With the current tf-nightly this works:

z = [] autograph.utils.set_element_type(z, tf.int32)

but this fails:

z=[] autograph.set_element_type(z, tf.int32)

b/111372661

mdanatg · 2018-07-10T17:07:29Z

samples/core/guide/autograph_control_flow.ipynb

+        "\n",
+        "with tf.Graph().as_default():  \n",
+        "  with tf.Session():\n",
+        "    print(tf_f(tf.constant(3)).eval())"


Any thought about using eval() / sess.run() consistently? I'm ok if you feel both should be exercised.

Any thought about using eval() / sess.run() consistently? I'm ok if you feel both should be exercised.

Consistency, sure! Do you have a preferred style? I have no strong feelings on this.

I generally use sess.run(), because eval() is not as idiomatic.

mdanatg · 2018-07-10T17:09:05Z

samples/core/guide/autograph_control_flow.ipynb

+      },
+      "cell_type": "markdown",
+      "source": [
+        "### Nested If statement"


"Nested if statements"? (plural, not capitalized)

mdanatg · 2018-07-10T17:09:53Z

samples/core/guide/autograph_control_flow.ipynb

+      },
+      "cell_type": "markdown",
+      "source": [
+        "## Advanced example: A training, loop in-graph\n",


Unnecessary comma?

mdanatg · 2018-07-10T17:11:23Z

samples/core/guide/autograph_control_flow.ipynb

+        "  while i < hp.max_steps:\n",
+        "    train_x, train_y = get_next_batch(train_ds)\n",
+        "    test_x, test_y = get_next_batch(test_ds)\n",
+        "    # add get next\n",


This comment feels unnecessary.

mdanatg · 2018-07-10T17:14:15Z

samples/core/guide/autograph_control_flow.ipynb

+        "\n",
+        "Writing control flow in AutoGraph is easy, so running a training loop in a TensorFlow graph should be easy as well!  \n",
+        "\n",
+        "Here, we show an example of training a simple Keras model on MNIST, where the entire training process -- loading batches, calculating gradients, updating parameters, calculating validation accuracy, and repeating until convergence -- is done in-graph."


WDYT about a sentence somewhere ahead of this that mentions something in the lines of "oh, and you can use Keras libraries in AutoGraph as well"? Not sure if it's something the user would expect or not.

Done. I'll link to the other notebook here when it's ready.

Add note about keras layers and models.

MarkDaoust · 2018-07-12T16:10:12Z

This is meant to be the guide page, yes? (Not a tutorial)

I'm open to suggestions. I was thinking this notebook could go in the "Low Level APIs" section of the guide, since that's the only place where we expect users to understand the details of graph mode.

I'm (still) working on a keras-model example that could go in tutorials.

If so, can you rename autograph_control_flow.ipynb to autograph.ipynb.

done.

Also, save notebook with open TOC.

Done.

Some of the heading hierarchy looks off.

Fixed.

lamberta · 2018-07-12T22:47:34Z

New staging: https://colab.sandbox.google.com/github/markdaoust/models/blob/autopgraph-guide/samples/core/guide/autograph.ipynb

lamberta · 2018-07-12T23:45:41Z

Copy edit: MarkDaoust#7
Stage: https://colab.sandbox.google.com/github/lamberta/models/blob/mark-autograph/samples/core/guide/autograph.ipynb

mdanatg · 2018-07-13T13:56:38Z

Latest draft looks good. A note about the import lines - the reader might be confused into thinking AutoGraph requires eager mode to be enabled. Could we add a note that we only enable it for comparison purposes?

lamberta · 2018-07-13T15:13:37Z

I was kinda wondering about that ...
Done, added note about eager and graph in my PR: MarkDaoust#7

Stage: https://colab.sandbox.google.com/github/lamberta/models/blob/mark-autograph/samples/core/guide/autograph.ipynb

mdanatg · 2018-07-13T15:25:54Z

Looks great! I'll be able to assist with debugging the convergence TODO later today if you wish.

Format, language updates

lamberta · 2018-07-13T17:06:07Z

samples/core/guide/autograph.ipynb

+      "source": [
+        "We'll enable  [eager execution](https://www.tensorflow.org/guide/eager) for demonstration purposes, but AutoGraph works in both eager and [graph execution](https://www.tensorflow.org/guide/graphs) environments:",
+        "",
+        "Important: The converted code will _work_ directly in eager mode. But it runs as an eager-python function. To run in-graph use explicit graphs, as in this doc, or use `tf.contrib.eager.defun`."


I think we added similiar notes, can you remove this one?

lamberta · 2018-07-13T17:51:15Z

Updated note in PR: MarkDaoust#8

Fix note about eager

MarkDaoust · 2018-07-13T17:55:48Z

Cool, I linked you to an internal bug for it.

…

On Fri, Jul 13, 2018 at 8:26 AM Dan Moldovan ***@***.***> wrote: Looks great! I'll be able to assist with debugging the convergence TODO later today if you wish. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4733 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABWWteXOpIQO-b0Pnnd7wobx1MVoR4PDks5uGLwPgaJpZM4VJvdh> .

MarkDaoust added 2 commits July 10, 2018 09:19

Pull AutoGraph workshop into docs.

b2136ab

From tensorflow/tensorflow/blob/8a6ef2cb4f98bacc1f821f60c21914b4bd5faaef

Updated from workshop to doc format.

e3a6768

MarkDaoust requested a review from lamberta as a code owner July 10, 2018 16:34

googlebot added the cla: yes label Jul 10, 2018

MarkDaoust requested review from mdanatg and wolffg July 10, 2018 16:34

mdanatg reviewed Jul 10, 2018

View reviewed changes

MarkDaoust added 3 commits July 11, 2018 18:20

Resolving review comments.

e1be4e9

Fix review comments.

a03c1ac

Add note about keras layers and models.

rename to autograph/ipynb

2c91b18

Format, language updates

3738025

Add note about eager. Add web button.

ff482a0

MarkDaoust added 4 commits July 13, 2018 09:40

Add note about eager-defun compat.

cf8bd4d

typo in my fix

3deea5d

Merge pull request #7 from lamberta/mark-autograph

bd1a74a

Format, language updates

clarification on how to use in-graph

6e2eac3

lamberta reviewed Jul 13, 2018

View reviewed changes

lamberta added 2 commits July 13, 2018 10:42

Move eager note

5df66e7

Fix eager note

444c2f0

MarkDaoust added 2 commits July 13, 2018 10:51

Merge pull request #8 from lamberta/mark-autograph

fac62b7

Fix note about eager

delete TODO

fceeb46

lamberta approved these changes Jul 13, 2018

View reviewed changes

MarkDaoust merged commit 78e871c into tensorflow:master Jul 13, 2018

Autograph guide #4733

Autograph guide #4733

Uh oh!

Conversation

MarkDaoust commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lamberta commented Jul 10, 2018

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!

mdanatg 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!

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!

MarkDaoust commented Jul 12, 2018

Uh oh!

lamberta commented Jul 12, 2018

Uh oh!

lamberta commented Jul 12, 2018

Uh oh!

mdanatg commented Jul 13, 2018

Uh oh!

lamberta commented Jul 13, 2018

Uh oh!

mdanatg commented Jul 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarkDaoust commented Jul 10, 2018 •

edited

Loading