Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jupytext --sync comments out a line of my code #339

Closed
aarchiba opened this issue Oct 12, 2019 · 9 comments
Closed

jupytext --sync comments out a line of my code #339

aarchiba opened this issue Oct 12, 2019 · 9 comments
Milestone

Comments

@aarchiba
Copy link

I have a notebook that I want to sync between ipynb and py:percent formats. Every time I do --sync, one of the cells gets turned from code into markdown.

When I do explicit conversions forward and back with --test-strict, both pass, and the cell does not get converted. But when I do --sync, I get:

$ cp examples/Example\ of\ parameter\ usage.py examples/Example\ of\ parameter\ usage.py.orig
$ jupytext --sync examples/Example\ of\ parameter\ usage.py 
[jupytext] Reading examples/Example of parameter usage.py
[jupytext] Loading input cells from 'examples/Example of parameter usage.py'
[jupytext] Loading output cells from 'examples/Example of parameter usage.ipynb'
[jupytext] Updating 'examples/Example of parameter usage.ipynb'
[jupytext] Updating 'examples/Example of parameter usage.py'
$ diff -u examples/Example\ of\ parameter\ usage.py.orig examples/Example\ of\ parameter\ usage.py
--- "examples/Example of parameter usage.py.orig"       2019-10-12 10:00:36.922517919 +0100
+++ "examples/Example of parameter usage.py"    2019-10-12 10:00:55.170794464 +0100
@@ -295,7 +295,7 @@
 # ## How do "prefix parameters" and "mask parameters" work?
 
 # %%
-cat = pp.prefixParameter(parameter_type='float', name="CAT0", units=u.ml, long_double=True)
+# cat = pp.prefixParameter(parameter_type='float', name="CAT0", units=u.ml, long_double=True)
 
 # %%
 dir(cat)

and the line is commented out. I have deleted the ipynb file and used --to and --test-strict to recreate it, so there should be nothing in there that is not in the .py file. This also occurs with py:light.

Example of parameter usage.zip

@mwouts
Copy link
Owner

mwouts commented Oct 12, 2019

Hello @aarchiba , thank you for reporting this! That's very interesting. What happens here is that Jupytext classifies that line as a bash command, so it comments it out when writing it back to the Python form.

The regular expression used for this is here:

_PYTHON_MAGIC_CMD = re.compile(r"^(# |#)*({})(\s|$)".format('|'.join(
# posix
['cat', 'cp', 'mv', 'rm', 'rmdir', 'mkdir'] +
# windows
['copy', 'ddir', 'echo', 'ls', 'ldir', 'mkdir', 'ren', 'rmdir'])))

In the future I will make sure to provide a fix for your example. Meanwhile, what do you think of using another name for your variable?

@mwouts mwouts added this to the 1.3.0 milestone Oct 12, 2019
@aarchiba
Copy link
Author

Hello @aarchiba , thank you for reporting this! That's very interesting. What happens here is that Jupytext classifies that line as a bash command, so it comments it out when writing it back to the Python form.

The regular expression used for this is here:

_PYTHON_MAGIC_CMD = re.compile(r"^(# |#)*({})(\s|$)".format('|'.join(
# posix
['cat', 'cp', 'mv', 'rm', 'rmdir', 'mkdir'] +
# windows
['copy', 'ddir', 'echo', 'ls', 'ldir', 'mkdir', 'ren', 'rmdir'])))

In the future I will make sure to provide a fix for your example. Meanwhile, what do you think of using another name for your variable?

I think this code was written by someone else and so I really can't guarantee people won't choose variable names like cp or cat (short for "catalog" which comes up a lot in astronomy).

Bash code won't work in my notebook anyway. Why treat it specially? Surely if someone has administered the hack that lets them use bash code without the ! it's their job to make sure that's visible to jupytext?

@mwouts
Copy link
Owner

mwouts commented Oct 12, 2019

I think this code was written by someone else and so I really can't guarantee people won't choose variable names like cp or cat (short for "catalog" which comes up a lot in astronomy).

Sure. I think we will be able to fix the regexp to make sure that cat = ... does not get commented. However, cat will still get commented, so maybe that won't solve your issue.

If you are confident that your users don't use magic commands, or if you don't need to run the .py file itself, you could give a try to the comment_magics option in Jupytext:

 echo 'cat = 3' | jupytext --to ipynb --update-metadata '{"jupytext": {"comment_magics":false}}
' | jupytext --to py
# ---
# jupyter:
#   jupytext:
#     cell_metadata_filter: -all
#     comment_magics: false
#     text_representation:
#       extension: .py
#       format_name: light
#       format_version: '1.4'
#       jupytext_version: 1.2.4
# ---

cat = 3

@mwouts
Copy link
Owner

mwouts commented Oct 12, 2019

Also: the comment_magics option defaults to False for the md and py:hydrogen formats. The latter is the same as py:percent, except for these magic commands (as Hydrogen lets you execute them). Maybe you'd want to represent your notebooks in one of these formats?

@aarchiba
Copy link
Author

Well, I routinely use %load_ext autoreload and %autoreload 2, how would those be handled if we turned off comment_magics?

Since what we actually need is just a format that won't lead to endless git merge conflicts, and I intend users to interact with the notebooks purely through jupyter-lab, it sounds like (R?) markdown would be a good answer.

This behaviour is still an unpleasant surprise! I'm also puzzled why it doesn't fail the --test-strict, or even happen there, even though using --sync it plainly does not round-trip correctly - leaving the python code entirely aside, the Jupyter notebook has a code cell converted to a markdown cell. It seems like this would be a problem even (especially!) if I were using the secretly-a-bash-command magic?

@mwouts
Copy link
Owner

mwouts commented Oct 12, 2019

Well, I routinely use %load_ext autoreload and %autoreload 2, how would those be handled if we turned off comment_magics?

Well, the text would be unchanged. That can be a problem if you require your .py file to be PEP8 compliant... And also, if you plan to run the .py files with something else than IPython.

it sounds like (R?) markdown would be a good answer.

Markdown is probably a good answer. In R Markdown, the python code is meant to be executed with reticulate, so magics commands are commented.

This behaviour is still an unpleasant surprise! I'm also puzzled why it doesn't fail the --test-strict, or even happen there, even though using --sync it plainly does not round-trip correctly - leaving the python code entirely aside, the Jupyter notebook has a code cell converted to a markdown cell. It seems like this would be a problem even (especially!) if I were using the secretly-a-bash-command magic?

Sure, that deserves a separate investigation, I will have a look.

mwouts added a commit that referenced this issue Oct 13, 2019
Code cell was transformed to a markdown cell in py format
#339
mwouts added a commit that referenced this issue Oct 13, 2019
Code cell was transformed to a markdown cell in py format
#339
mwouts added a commit that referenced this issue Oct 13, 2019
mwouts added a commit that referenced this issue Oct 13, 2019
mwouts added a commit that referenced this issue Oct 13, 2019
Rather than on its notebook representation (#339)
mwouts added a commit that referenced this issue Oct 13, 2019
mwouts added a commit that referenced this issue Oct 13, 2019
mwouts added a commit that referenced this issue Oct 13, 2019
Rather than on its notebook representation (#339)
@mwouts
Copy link
Owner

mwouts commented Oct 13, 2019

@aarchiba , thank you again for raising that subject.

Here is what I found:

  • The pattern for magic commands was a bit too wide. Now cat=... will not be classified as a magic command any more.
  • The pattern for decoding magic commands was not exactly the same as the one for encoding them. That was causing the transformation of the code cell to a Markdown cell when using the py:light format.
  • The round trip was tested on the notebook representation rather than on the text file, as we would have expected when the input is a text file.

Now I have prepared a release candidate with all the corresponding fixes. Can you give it a try and let me know if it solves the issue you reported? It's available on pypi:

pip install jupytext==1.3.0rc0

@aarchiba
Copy link
Author

Well, when I try to run it with --test-strict both ways I get unhelpful tracebacks:

$ jupytext --test-strict Example\ of\ parameter\ usage.
Example of parameter usage.ipynb  Example of parameter usage.py     
(jupytext-test) archibald@dop307:~/projects/pint/jupytext-problem$ jupytext --test-strict Example\ of\ parameter\ usage.py 
[jupytext] Reading Example of parameter usage.py
Traceback (most recent call last):
  File "/home/archibald/.virtualenvs/jupytext-test/bin/jupytext", line 8, in <module>
    sys.exit(jupytext())
  File "/home/archibald/.virtualenvs/jupytext-test/local/lib/python2.7/site-packages/jupytext/cli.py", line 228, in jupytext
    exit_code += jupytext_single_file(nb_file, args, log)
  File "/home/archibald/.virtualenvs/jupytext-test/local/lib/python2.7/site-packages/jupytext/cli.py", line 367, in jupytext_single_file
    if dest_fmt['extension'] != '.ipynb':
TypeError: 'NoneType' object has no attribute '__getitem__'

and

$ jupytext --test-strict Example\ of\ parameter\ usage.ipynb
[jupytext] Reading Example of parameter usage.ipynb
Traceback (most recent call last):
  File "/home/archibald/.virtualenvs/jupytext-test/bin/jupytext", line 8, in <module>
    sys.exit(jupytext())
  File "/home/archibald/.virtualenvs/jupytext-test/local/lib/python2.7/site-packages/jupytext/cli.py", line 228, in jupytext
    exit_code += jupytext_single_file(nb_file, args, log)
  File "/home/archibald/.virtualenvs/jupytext-test/local/lib/python2.7/site-packages/jupytext/cli.py", line 359, in jupytext_single_file
    stop_on_first_error=args.stop_on_first_error)
  File "/home/archibald/.virtualenvs/jupytext-test/local/lib/python2.7/site-packages/jupytext/compare.py", line 242, in test_round_trip_conversion
    text = writes(notebook, fmt)
  File "/home/archibald/.virtualenvs/jupytext-test/local/lib/python2.7/site-packages/jupytext/jupytext.py", line 290, in writes
    ext = fmt['extension']
KeyError: 'extension'

But if I use --sync or provide --to it does the right thing, and no longer mangles my input file.

@mwouts
Copy link
Owner

mwouts commented Oct 20, 2019

Oh, I see... the --to argument is mandatory here: you want to test the round trip to/from a given format. I'll try to enforce this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants