Skip to content

Fix multiline magic#28160

Merged
tensorflow-copybara merged 1 commit intotensorflow:masterfrom
lc0:fix-multiline
Apr 26, 2019
Merged

Fix multiline magic#28160
tensorflow-copybara merged 1 commit intotensorflow:masterfrom
lc0:fix-multiline

Conversation

@lc0
Copy link
Copy Markdown
Contributor

@lc0 lc0 commented Apr 25, 2019

Unfortunately, the feature for multiline introduced by another contributor in 9355771#diff-db01673eb6e019b39689cd41bfae3d5e breaks notebook cell alignment.

The problem is that we skip the cell, during _get_code , but do not skip the same cells during _update_notebook . As a result, the diffs, that have such multiline magic, are aligned

This PR should fix the problem.

@tensorflow-bot tensorflow-bot bot added the size:XS CL Change Size: Extra Small label Apr 25, 2019
@lc0
Copy link
Copy Markdown
Contributor Author

lc0 commented Apr 25, 2019

I have a couple of questions:

  • what is the good way to bring a small examples of notebooks to cover such cases with the tests?
  • what is a good way to run these tests locally?

I tried specifying one test for bazel, but seems like bazel anyway tries to rebuild the universe:(

@lc0
Copy link
Copy Markdown
Contributor Author

lc0 commented Apr 25, 2019

cc @martinwicke

@lc0
Copy link
Copy Markdown
Contributor Author

lc0 commented Apr 25, 2019

Something to be aware of: with implemented approach to multiline-magic, the python tensorflow code, that is used in %%timeit would not be considered as python code, so might have TF1.x code

@martinwicke
Copy link
Copy Markdown
Member

This is better than not handling multi-line properly.

Is there a list of %% directives we should exclude? If timeit is the only real issue, we could special-case it.

@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 26, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 26, 2019
@tensorflow-copybara tensorflow-copybara merged commit cb93088 into tensorflow:master Apr 26, 2019
tensorflow-copybara pushed a commit that referenced this pull request Apr 26, 2019
PiperOrigin-RevId: 245482882
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes ready to pull PR ready for merge process size:XS CL Change Size: Extra Small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants