Skip to content

Feature/issue 1274 notebook format check#1299

Open
rosspeili wants to merge 5 commits into
quantumlib:mainfrom
rosspeili:feature/issue-1274-notebook-format-check
Open

Feature/issue 1274 notebook format check#1299
rosspeili wants to merge 5 commits into
quantumlib:mainfrom
rosspeili:feature/issue-1274-notebook-format-check

Conversation

@rosspeili
Copy link
Copy Markdown

@rosspeili rosspeili commented May 6, 2026

Closes #1274

Adds check/nbformat to validate tracked .ipynb files with tensorflow_docs.tools.nbfmt (repo root, Cirq-style), wired into check/all, shellcheck, and CI. Adds tensorflow-docs to the pytest and dev lockfiles.

Notebook formatting is in #1322 and should merge before this PR.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new script, check/nbformat, to validate and fix Jupyter notebook formatting using tensorflow_docs.tools.nbformat, and updates the project's dependency requirements and lockfiles. The review highlights two critical issues: the inefficiency of invoking the format checker in a loop rather than passing all files at once, and the breaking of environment compatibility for older Python versions caused by regenerating lockfiles with Python 3.13 instead of the project's minimum supported version.

Comment thread check/nbformat Outdated
Comment thread dev_tools/requirements/envs/dev.env.txt
@rosspeili
Copy link
Copy Markdown
Author

Hey @mhucka any feedback on this also? No stress, just checking in. Thanks.

@mhucka
Copy link
Copy Markdown
Collaborator

mhucka commented May 30, 2026

@rosspeili Thank you for the contribution, and sorry for the long delay in getting back to this.

The merge conflicts are because files in the repo changed in the meantime, which is my fault for not reviewing this sooner, so I will take the time to fix it in a moment and push an updated version to your branch. (I hope you don't mind – it will save everyone time.)

A bigger problem is that this does not work. How did you test this? This is what happens when I run check/nbformat:

./check/nbformat
Found 15 notebook(s).
Running notebook format checker...
  Checking: docs/fqe/guide/introduction.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/fqe/tutorials/diagonal_coulomb_evolution.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/fqe/tutorials/fermi_hubbard.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/fqe/tutorials/fqe_vs_openfermion_quadratic_hamiltonians.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/fqe/tutorials/hamiltonian_time_evolution_and_expectation_estimation.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/tutorials/binary_code_transforms.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/tutorials/bosonic_operators.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/tutorials/circuits_1_basis_change.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/tutorials/circuits_2_diagonal_coulomb_trotter.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/tutorials/circuits_3_arbitrary_basis_trotter.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/tutorials/intro_to_openfermion.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/tutorials/intro_workshop_exercises.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: docs/tutorials/jordan_wigner_and_bravyi_kitaev_transforms.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: src/openfermion/resource_estimates/pbc/notebooks/isdf.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat
  Checking: src/openfermion/resource_estimates/pbc/notebooks/resource_estimates.ipynb
~/.pyenv/versions/o12/bin/python: No module named tensorflow_docs.tools.nbformat

@mhucka mhucka added area/devops Involves build systems, Make files, Bazel files, continuous integration, and or other DevOps topics area/docs Involves documentation, notebooks, README files, and similar area/tests Involves testing and test cases labels May 30, 2026
@mhucka mhucka self-assigned this May 30, 2026
Comment thread check/nbformat Outdated
Comment thread check/nbformat Outdated
@rosspeili
Copy link
Copy Markdown
Author

rosspeili commented May 30, 2026

Hey @mhucka no stress at all. Appreciate the review and comments as always. As disclosed also in previews PRs, the PR description indeed is formatted with AI, based on a dump, but will make it more concise or just drop the original blurp. Code is hand written and passed through AI for review before pr. I used existing tests with py 3.13, but will check again and come back to you asap.

Please do feel free to push to my branch if available. Or suggest and I'll approve. 🙏

rosspeili and others added 3 commits May 30, 2026 12:37
Add check/nbformat, a new bash script that checks the format of all
tracked Jupyter notebook (.ipynb) files using the TensorFlow Docs
notebook format checker (tensorflow_docs.tools.nbformat).

The script:
- Discovers all .ipynb files tracked by git via git ls-files
- Runs python -m tensorflow_docs.tools.nbformat on each notebook
- Defaults to check-only mode (non-destructive)
- Accepts --fix to reformat notebooks in place
- Accepts --help to print usage information
- Exits non-zero if any notebook fails the format check

Wire check/nbformat into:
- check/all: called unconditionally alongside check/mypy and
  check/shellcheck, since notebook format is structural and not
  related to which Python files changed
- check/shellcheck: added to the required_shell_scripts list
  (in sorted order) so shellcheck verifies the new script is present

Add tensorflow-docs to dev_tools/requirements/deps/pytest.txt
alongside the existing nbformat dep, since both are needed for
notebook-related checks and belong in the same env grouping.

Add a notebook-checks CI job to .github/workflows/ci.yaml that
triggers when .ipynb files change and runs check/nbformat using
the pytest.env.txt environment (which includes tensorflow-docs).
Also add the new job to report-results so it is a required check.
Co-authored-by: Michael Hucka <mhucka@google.com>
…1274)

The checker called a non-existent tensorflow_docs.tools.nbformat module.
Align with Cirq by using nbfmt, regenerate lockfiles on current main, and
format all tracked notebooks so CI can pass.
@rosspeili rosspeili force-pushed the feature/issue-1274-notebook-format-check branch from 39ff7df to b52511c Compare May 30, 2026 10:02
@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@rosspeili
Copy link
Copy Markdown
Author

@mhucka thanks again for the patience and tips.

Rebased onto current main and fixed check/nbformat, I had the wrong module (nbformat vs nbfmt, per Cirq). Ran nbfmt locally on py3.13 and formatted the notebooks and lockfiles regenerated on top of main.

Let me know if I am missing anything. <3

@rosspeili rosspeili requested a review from mhucka May 30, 2026 11:21
Copy link
Copy Markdown
Collaborator

@mhucka mhucka left a comment

Choose a reason for hiding this comment

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

Thanks for the quick work. This is better! There's just a small niggle in check/nbformat; please check the comment there.

I didn't anticipate that the notebooks would actually not conform to the format and would produce a lot of diffs. It is better to split out the formatting differences to a separate PR because then the PR commit can be adde to .git-blame-ignore-revs so that git blame output is less noisy. So, could we please proceed as follows?

  1. Pull out the tutorial file changes that result from running check/nbformat --apply and move them to a separate PR.
  2. I will review & merge that separate PR first.
  3. Then we will merge this PR (#1274).
  4. Then I will do a new PR to add the commit of PR #1 to .git-blame-ignore-revs.

Comment thread check/nbformat Outdated
Comment thread .github/workflows/ci.yaml
@mhucka
Copy link
Copy Markdown
Collaborator

mhucka commented May 31, 2026

One more minor thing. In the PR description, it's better to omit this statement:

Earlier version used the wrong module (nbformat) which was fixed to nbfmt with --apply for local fixes.

When this PR is merged, the changes will be squashed in the git history and it will not be easy for readers to figure out what this comment refers to. The description only needs to refer to the PR as it stands and not the history of the PR.

(I think you may have intended the comment to be helpful information for reviewers of the updated PR. That's kind, and I understand the impulse, but nevertheless, it turns out it's actually counter-productive in the long run. I used to do the same thing, and had to unlearn this particular habit …)

@rosspeili
Copy link
Copy Markdown
Author

Understood and respected. ✅️

rosspeili added 2 commits May 31, 2026 12:00
Pass . to nbfmt instead of a hard-coded path list (nbfmt is
recursive and only touches .ipynb). Add notebook_files output for
consistency with other paths-filter variables.
Notebook nbfmt diffs are in a separate PR to merge first for
git-blame-ignore-revs. This PR adds only the check script, CI, and deps.
@rosspeili
Copy link
Copy Markdown
Author

@mhucka split as requested with notebook formatting now in #1322, while this PR is only the check script, CI wiring, and lockfiles (7 files). Also applied your feedback on . for nbfmt and notebook_files in the paths filter.

Thanks so much for the feedback and patience <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/devops Involves build systems, Make files, Bazel files, continuous integration, and or other DevOps topics area/docs Involves documentation, notebooks, README files, and similar area/tests Involves testing and test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check scripts do not check tutorials, but should

2 participants