Skip to content

Conversation

@mhucka
Copy link
Member

@mhucka mhucka commented Jan 4, 2025

In more recent versions of Python like 3.10, the base64 package changed APIs, and this in turn caused nbformat 4.4.0 to fail to work. Updating to the latest version (5.1.3) solved that problem. A minor tweak was also needed in scripts/format_ipynb.py to account for a different error class being returned when "!pip ..." commands are encountered in the notebook file.

After updating the version of nbformat, running scripts/format_ipynb.py changed the contents of all the ipython notebook files. Most frustratingly, one of the changes is that the indentation of the .ipynb files changed, making the diffs particularly noisy. Spot-inspecting files manually reveals a number of real changes, some being worthwhile (e.g., adding missing spaces in argument lists) and some being just differences in formatting such as where line breaks are introduced. The latter set of changes is puzzling because the style settings haven't changed. I tried but couldn't find a way to avoid these changes except by outright changing the style parameters, but then, doing that would make the TFQ .ipynb file formatting become non-standard, and that seems worse in the long run. So, for lack of a better solution, I'm checking in all the reformatting notebooks with the hope that future versions of yapf and nbformat don't keep introducing more .ipynb format changes.

mhucka added 2 commits January 4, 2025 19:24
In more recent versions of Python like 3.10, the `base64` package
changed APIs, and this in turn caused nbformat 4.4.0 to fail to work.
Updating to the latest version (5.1.3) solved that problem.

A minor tweak was also needed in scripts/format_ipynb.py to account
for a different error class being returned when "!pip ..." commands
are encountered in the notebook file.
After updating nbformat, running scripts/format_ipynb.py changed the
contents of all the ipython notebook files. Most frustratingly, one of
the changes is that the indentation of the .ipynb files changed,
making the diffs particularly noisy. Spot-inspecting files manually
reveals a number of real changes, some being worthwhile (e.g., adding
missing spaces in argument lists) and some being just differences in
formatting such as where line breaks are introduced. The latter set
of changes is puzzling because the style settings haven't changed. I
tried but couldn't find a way to avoid these changes except by
outright changing the style parameters, but then, that would mean the
TFQ .ipynb file formatting would become non-standard, and that seems
worse. So, for lack of a better solution, I'm checking in all the
reformatting notebooks with the hope that future versions of yapf and
nbformat don't keep introducing more .ipynb format changes.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@mhucka mhucka marked this pull request as ready for review January 4, 2025 20:16
@mhucka mhucka added kind/chore Maintenance & housekeeping tasks area/health Involves general matters of project configuration, health, maintenance, and similar concerns labels Jan 5, 2025
@mhucka mhucka self-assigned this Jan 5, 2025
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

This looks fine to me. There is no way of reading this whole diff. Did you simply bump the version and then run the formatter ?

I.e. you didn't re-run any of the notebooks that would alter the cell outputs etc. ?

@mhucka
Copy link
Member Author

mhucka commented Jan 10, 2025

This looks fine to me. There is no way of reading this whole diff. Did you simply bump the version and then run the formatter ?
I.e. you didn't re-run any of the notebooks that would alter the cell outputs etc. ?

That's right; didn't run the notebooks, only ran them through the formatter.

I did actually spot-check the diffs (a bit of text editing can get rid of the leading whitespace change, and makes it possible to get a better diff result). I didn't see any functional changes; it was all line breaks or comma spacing or similar.

Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

LGTM

@mhucka mhucka merged commit 9101a91 into tensorflow:master Jan 10, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/health Involves general matters of project configuration, health, maintenance, and similar concerns kind/chore Maintenance & housekeeping tasks

Development

Successfully merging this pull request may close these issues.

2 participants