Add support for line breaks in jupyter #27834

lc0 · 2019-04-14T13:16:01Z

jupyter cells could have a single line magic, but that splits a line with \.
This PR adds support for such cases.

lc0 · 2019-04-14T20:40:54Z

martinwicke · 2019-04-15T19:11:52Z

tensorflow/tools/compatibility/ipynb.py

      for line_idx, code_line in enumerate(cell_lines):

        # Sometimes, jupyter has more than python code
        # Idea is to comment these lines, for upgrade time
-        if code_line.startswith("%") or code_line.startswith("!") \
-            or code_line.startswith("?"):
+        if skip_magic(code_line, ['%', '!', '?']) or is_line_split:
          # Found a special character, need to "encode"
          code_line = "###!!!" + code_line


I don't understand how this works...

The idea behind is just to make it work for cases like

!gcloud ml-engine models create ${MODEL} \ --regions us-central1

So I just check if the "magic" code line ends with , so I also need to comment the next line, since it's not python 🙂

I also prepared a couple of test case, that I am planning to add somewhere, so it's testable - > https://colab.research.google.com/drive/1fCcmg8ZcbnR5dnG3KNl96QKN7u0FrQL-#scrollTo=jrxeKRCwogn6

Yes, would be good to have test cases for this stuff with the code here.

I see the logic to skip the next line, but what does the ###!!! do?

It's just a way to encode these sections with unique commenting prefix, so I can strip them after pasta.

###!!! it's just "unique pattern" that is less likely to be used by somebody else. Otherwise I would remove something, that I did not want to remove

martinwicke · 2019-04-17T00:30:40Z

tensorflow/tools/compatibility/ipynb.py

+
+def check_line_split(code_line):
+  r"""
+  Checks if a line was splitted with `\`.


splitted -> split

martinwicke · 2019-04-17T00:31:27Z

tensorflow/tools/compatibility/ipynb.py

+  Checks if the cell has magic, that is not python-based.
+
+   >>> skip_magic('!ls -laF', ['%', '!', '?'])
+  True


Can you add an args, returns section, and put the brief description on the same line as the opening """? (also below)

martinwicke · 2019-04-17T00:32:05Z

tensorflow/tools/compatibility/ipynb.py

+  True
+  """
+
+  if code_line.endswith('\\\n'):


Will this catch lines with trailing whitespace (which I think is allowed after \)?

martinwicke · 2019-04-17T00:33:49Z

tensorflow/tools/compatibility/ipynb.py

      for line_idx, code_line in enumerate(cell_lines):

        # Sometimes, jupyter has more than python code
        # Idea is to comment these lines, for upgrade time
-        if code_line.startswith("%") or code_line.startswith("!") \
-            or code_line.startswith("?"):
+        if skip_magic(code_line, ['%', '!', '?']) or is_line_split:
          # Found a special character, need to "encode"
          code_line = "###!!!" + code_line


Yes, would be good to have test cases for this stuff with the code here.

I see the logic to skip the next line, but what does the ###!!! do?

martinwicke · 2019-04-17T19:37:01Z

I still don't understand the logic behind commenting out the continued lines.

Is that encoding ever removed?

lc0 · 2019-04-17T19:57:23Z

@martinwicke right, it's removed once I update from updated code back to notebook. Just some lines below at

tensorflow/tensorflow/tools/compatibility/ipynb.py

Line 167 in 850d385

cell["source"] = "\n".join(new_code).replace("###!!!", "").replace(

martinwicke · 2019-04-18T05:48:32Z

Got it. Test cases would be nice, maybe just a test notebook that has cells containing stuff like this that you can run this on to make sure it works.

rthadur · 2019-04-18T17:18:08Z

@lc0 can you please check build failures.

lc0 · 2019-04-18T17:38:50Z

@rthadur I am not sure it's related to my changes

ERROR: /tmpfs/src/github/tensorflow/tensorflow/compiler/xla/BUILD:887:1: name 'cc_header_only_library' is not defined
ERROR: in target '//tensorflow/compiler/xla/service:friends', no such label '//tensorflow/compiler/xla:friends': Target '//tensorflow/compiler/xla:friends' contains an error and its package is in error
ERROR: /tmpfs/src/github/tensorflow/tensorflow/compiler/xla/service/BUILD:30:1: Target '//tensorflow/compiler/xla:xla_data_proto_py_genproto' contains an error and its package is in error and referenced by '//tensorflow/compiler/xla/service:hlo_proto_py_genproto'
ERROR: /tmpfs/src/github/tensorflow/tensorflow/core/protobuf/tpu/BUILD:61:1: Target '//tensorflow/compiler/xla:xla_data_proto_py_genproto' contains an error and its package is in error and referenced by '//tensorflow/core/protobuf/tpu:compilation_result_proto_py_genproto'
ERROR: /tmpfs/src/github/tensorflow/tensorflow/core/protobuf/tpu/BUILD:61:1: Target '//tensorflow/compiler/xla:xla_data_proto_py' contains an error and its package is in error and referenced by '//tensorflow/core/protobuf/tpu:compilation_result_proto_py'
ERROR: Analysis of target '//tensorflow/tools/pip_package:build_pip_package' failed; build aborted: Analysis failed

lc0 · 2019-04-18T17:45:36Z

seems like there are some issues with pilling xla as dependencies, must be coming from a different place cc @rthadur

lc0 · 2019-04-20T10:42:31Z

Would you mind to try to re-run the tests? XLA fails have nothing to do with my changes.

PiperOrigin-RevId: 244888116

Add support for line splits

62d12da

googlebot added the cla: yes label Apr 14, 2019

Update doc test

8fb20dd

lc0 force-pushed the converter-line-split branch from 6787ed1 to 8fb20dd Compare April 14, 2019 20:19

lc0 marked this pull request as ready for review April 14, 2019 20:19

rthadur requested a review from ymodak April 15, 2019 00:54

rthadur self-assigned this Apr 15, 2019

rthadur added this to Assigned Reviewer in PR Queue via automation Apr 15, 2019

rthadur added the size:S CL Change Size: Small label Apr 15, 2019

martinwicke reviewed Apr 15, 2019

View reviewed changes

lc0 added 2 commits April 15, 2019 22:05

Comment line breaks only for magic lines

fab6ba6

Continue line break only if one has started

f57940f

rthadur requested a review from martinwicke April 15, 2019 20:46

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Apr 17, 2019

martinwicke requested changes Apr 17, 2019

View reviewed changes

Switch to regex; update styles

850d385

rthadur requested a review from martinwicke April 17, 2019 15:19

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Apr 18, 2019

martinwicke approved these changes Apr 18, 2019

View reviewed changes

rthadur added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 18, 2019

kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 18, 2019

dynamicwebpaige added the kokoro:force-run Tests on submitted change label Apr 22, 2019

kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 22, 2019

tensorflow-copybara merged commit 850d385 into tensorflow:master Apr 23, 2019

PR Queue automation moved this from Approved by Reviewer to Merged Apr 23, 2019

tensorflow-copybara pushed a commit that referenced this pull request Apr 23, 2019

Merge pull request #27834 from lc0:converter-line-split

df72d99

PiperOrigin-RevId: 244888116

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for line breaks in jupyter #27834

Add support for line breaks in jupyter #27834

lc0 commented Apr 14, 2019 •

edited

lc0 commented Apr 14, 2019

martinwicke Apr 15, 2019

lc0 Apr 16, 2019

lc0 Apr 16, 2019

martinwicke Apr 17, 2019

lc0 Apr 17, 2019

martinwicke Apr 17, 2019

martinwicke Apr 17, 2019

martinwicke Apr 17, 2019

martinwicke Apr 17, 2019

martinwicke commented Apr 17, 2019

lc0 commented Apr 17, 2019

martinwicke commented Apr 18, 2019

rthadur commented Apr 18, 2019

lc0 commented Apr 18, 2019

lc0 commented Apr 18, 2019

lc0 commented Apr 20, 2019

Add support for line breaks in jupyter #27834

Add support for line breaks in jupyter #27834

Conversation

lc0 commented Apr 14, 2019 • edited

lc0 commented Apr 14, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinwicke commented Apr 17, 2019

lc0 commented Apr 17, 2019

martinwicke commented Apr 18, 2019

rthadur commented Apr 18, 2019

lc0 commented Apr 18, 2019

lc0 commented Apr 18, 2019

lc0 commented Apr 20, 2019

lc0 commented Apr 14, 2019 •

edited