Add notebook formatting and CI check #124

rmlarose · 2021-01-11T14:25:22Z

Fixes #56.

mpharrigan

Oof that diff.

I'll need to download this and look at it in pycharm's differ (which handles the change in indent much more elegantly). Is it just changing the indent or are there additional changes?

How hard would it be to maintain a list of grandfathered-in 2-indent notebooks so we can kick this can down the road some more?

mpharrigan · 2021-01-13T01:13:18Z

dev_tools/nbformat

+#!/usr/bin/env bash
+
+###############################################################################
+# Formats ipython notebooks with tensorflow-docs nbformat tool.


Did you write this or take it from somewhere?

If the former: ick bash. I guess it's ok here since you aren't doing looping (which is my rule of thumb for when a script definitely needs to be in a real programming langauge)

If the latter: note where you got it from so we can keep them in sync

This is taken directly from Cirq: https://github.com/quantumlib/Cirq/blob/master/check/nbformat

In addition to indentation, metadata is also removed and probably a few other things - @lamberta may know more here.

Not sure why the diff is so large - may be because GH has trouble analyzing notebooks but I'll check. At least by quick glance the notebooks are very similar before/after formatting.

Try passing --indent=2 to nbfmt. The default is 4 but the quantum folks prefer 2

@lamberta I think the collab default is 2 but the jupyter default is 1 and we prefer 1

@rmlarose the diff is messed up because of the indentation of the json in the ipynb files. Currently recirq has some with 2 and some with 1

lamberta · 2021-01-13T18:21:44Z

dev_tools/nbformat

+cd "$(git rev-parse --show-toplevel)"
+
+# Install a pinned version of tf-docs
+TF_DOCS_COMMIT=b3dc8a922d8bdc6e998a8ad4f4953359dc6e576a


Pinning a commit makes sense, but we don't really do releases in tensorflow/docs so this commit is arbitrary.

It might be better to download the cirq dev_tools/nbformat script into your GitHub Actions VM so you're at least using the same thing and don't get too out of sync.

+1 downloading cirq's script

rmlarose · 2021-01-14T16:28:27Z

Diff is about 1/2 as large with indent=2. (But note Cirq sets indent=1.)

How hard would it be to maintain a list of grandfathered-in 2-indent notebooks so we can kick this can down the road some more?

I think it's fine to punt on this. This PR is the simplest fix for #56 using the tool already in Cirq, but it sounds like there's some things to decide on.

It might be better to download the cirq dev_tools/nbformat script into your GitHub Actions VM so you're at least using the same thing and don't get too out of sync.

Great point. So would we want to just grab this file from Cirq in CI to make sure they're the same? I guess this works as long as the file location doesn't change.

lamberta · 2021-01-14T17:31:20Z

Ah, you're right (and I misspoke): nbfmt indent defaults to 2 and the quantum folks mentioned they prefer 1.

So would we want to just grab this file from Cirq in CI to make sure they're the same?

Curl'ing the file is probably easiest for a one-off. Though you can checkout the entire Cirq repo (shallow clone) next to this one using the GitHub Actions checkout module—if there's additional tooling in that repo you needed to access.

mpharrigan · 2021-01-14T22:40:18Z

How hard would it be to maintain a list of grandfathered-in 2-indent notebooks

If we use Cirq's script (which I think we should) then we can't do this (without a lot of headache) so I'm fine "biting the bullet" now.

Like I said, pycharm seems to handle the change-in-indent diffs much better so I'll review the PR that way once it's finalized with indent=1 and using cirq's script

mpharrigan · 2021-01-14T22:41:13Z

Another fun trick is that you can append ?w=1 to a github diff url to omit whitespace changes:

https://github.com/quantumlib/ReCirq/pull/124/files?w=1

mpharrigan · 2021-01-20T23:11:27Z

@rmlarose what do you think about wgetting the script from Cirq?

rmlarose · 2021-01-20T23:31:47Z

@rmlarose what do you think about wgetting the script from Cirq?

I like it! I should be able to get to this by the end of the week, just haven't had time yet.

Only potential downside I see is a new contributor to ReCirq wondering how to format a notebook to pass CI. It can be figured out though, and I agree its a priority to be in sync with Cirq.

mpharrigan · 2021-01-20T23:53:01Z

Only potential downside I see is a new contributor to ReCirq wondering how to format a notebook to pass CI. It can be figured out though, and I agree its a priority to be in sync with Cirq.

Good point. Maybe we can have a simple shell script in this repo to optionally download and then execute the script

if [[ check/nbfmt doesn't exist ]]
  wget it from cirq
fi

check/nbfmt

which the CI can just call (as well as contributors). [and add the downloaded file to .gitignore so people don't commit it]

lamberta · 2021-01-21T00:06:29Z

Only potential downside I see is a new contributor to ReCirq wondering how to format a notebook to pass CI.

True. Depending how fancy you want to get, you can add a GitHub Action to automatically post a comment with instructions to the pull request, for example: tensorflow/docs-l10n#474 (comment)
But the simplest thing is to add a saved reply to your GitHub settings and use that if you're monitoring PRs. And if you have a open source contributing guide you can add instructions there (at least you'll have a consistent place to point).

We actually have a bot set up to automatically format new pull requests. It's nice but requires auth setup for GitHub with a Cloud Function. Probably not worth it for now

…-format

…i-format

rmlarose · 2021-01-22T18:44:38Z

Good point. Maybe we can have a simple shell script in this repo to optionally download and then execute the script
if [[ check/nbfmt doesn't exist ]]
  wget it from cirq
fi

check/nbfmt
which the CI can just call (as well as contributors). [and add the downloaded file to .gitignore so people don't commit it]

Went with this method.

One can do dev_tools/nbfmt [--apply] locally which will grab the Cirq nbformat script (if it doesn't already exist) then run it.
nbformat added to gitignore.
Back to large --indent=1 diff, but on par with Cirq.

Works well I think, but I guess we're kind of crossing our fingers that no one moves or renames nbformat in Cirq.

mpharrigan · 2021-01-22T18:49:31Z

we're kind of crossing our fingers that no one moves or renames nbformat in Cirq.

This would break the CI but not any user-facing functionality, i.e. the least-bad breakage. I'll keep an eye on Cirq PRs with this in mind anyways though :)

mpharrigan · 2021-01-25T20:01:20Z

.gitignore

@@ -39,3 +39,6 @@ docs/generated

 # Default pycharm virtual env
 .venv/
+
+# Notebook formatting script.
+nbformat


but it downloads into dev_tools/, right? I guess this still works but may be overzealous. Probably doesn't matter too much

Indeed downloads into dev_tools

mpharrigan · 2021-01-25T20:02:14Z

docs/quantum_chess/concepts.ipynb

-        }
-      },
-      "source": [
-        "king_moves = cirq.Circuit(\n",


is this deleting code?

I guess it's moving it down lower

mpharrigan

Checked the diff offline. Looks good; quantum chess notebook is a little wonky. Why are we saving outputs there? And if we are, we should run it through "fresh" so the indices are contiguous from zero

rmlarose · 2021-01-26T23:56:46Z

quantum chess notebook is a little wonky. Why are we saving outputs there? And if we are, we should run it through "fresh" so the indices are contiguous from zero

Not sure. Maybe there was a good reason for saving output? The first cell starting at 97 is certainly not necessary - at least this doesn't show on the website or on colab though.

See #135 which follows up on this. I can address any remaining comments from this PR there if desired.

rmlarose added 3 commits January 11, 2021 09:01

Add nbformat from Cirq (https://github.com/quantumlib/Cirq/blob/maste…

e66013e

…r/check/nbformat).

Format notebooks.

77303fd

Add nbformat check.

cf7ce06

mpharrigan reviewed Jan 13, 2021

View reviewed changes

lamberta reviewed Jan 13, 2021

View reviewed changes

rmlarose added 3 commits January 14, 2021 10:48

Indent=2.

580b395

Merge branch 'master' into ci-format

bbd5c43

Format after merge.

50ca419

rmlarose requested a review from mpharrigan January 14, 2021 16:28

rmlarose added 5 commits January 22, 2021 13:20

wget cirq's nbformat script.

b7ba1a3

(Re)format docs.

88e131c

Merge branch 'master' into ci-format

0388d5f

Merge branch 'master' of https://github.com/quantumlib/ReCirq into ci…

1f94c74

…-format

Merge branch 'ci-format' of https://github.com/rmlarose/recirq into c…

a422272

…i-format

mpharrigan reviewed Jan 25, 2021

View reviewed changes

mpharrigan approved these changes Jan 25, 2021

View reviewed changes

mpharrigan merged commit 048b099 into quantumlib:master Jan 25, 2021

rmlarose mentioned this pull request Jan 26, 2021

Better error message for nbfmt #135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add notebook formatting and CI check #124

Add notebook formatting and CI check #124

rmlarose commented Jan 11, 2021

mpharrigan left a comment

mpharrigan Jan 13, 2021

rmlarose Jan 13, 2021

lamberta Jan 13, 2021

mpharrigan Jan 14, 2021

lamberta Jan 13, 2021

mpharrigan Jan 14, 2021

rmlarose commented Jan 14, 2021

lamberta commented Jan 14, 2021

mpharrigan commented Jan 14, 2021

mpharrigan commented Jan 14, 2021

mpharrigan commented Jan 20, 2021

rmlarose commented Jan 20, 2021

mpharrigan commented Jan 20, 2021

lamberta commented Jan 21, 2021

rmlarose commented Jan 22, 2021

mpharrigan commented Jan 22, 2021

mpharrigan Jan 25, 2021

rmlarose Jan 26, 2021

mpharrigan Jan 25, 2021

mpharrigan Jan 25, 2021

mpharrigan left a comment

rmlarose commented Jan 26, 2021

Add notebook formatting and CI check #124

Add notebook formatting and CI check #124

Conversation

rmlarose commented Jan 11, 2021

mpharrigan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmlarose commented Jan 14, 2021

lamberta commented Jan 14, 2021

mpharrigan commented Jan 14, 2021

mpharrigan commented Jan 14, 2021

mpharrigan commented Jan 20, 2021

rmlarose commented Jan 20, 2021

mpharrigan commented Jan 20, 2021

lamberta commented Jan 21, 2021

rmlarose commented Jan 22, 2021

mpharrigan commented Jan 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpharrigan left a comment

Choose a reason for hiding this comment

rmlarose commented Jan 26, 2021