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

Python nightly (3.7 ?) remove bare strings. #11133

Open
Carreau opened this issue May 12, 2018 · 9 comments

Comments

Projects
None yet
4 participants
@Carreau
Copy link
Member

commented May 12, 2018

Python compile step seem to remove base string from AST leading to no outputs in ipython:

In [1]: "1"

In [2]:

Haven't tracked that down yet, I'd like to investigate that before Python 3.7 is out.

On 3.6 that would give

In [1]: "a"
Out[1]: 'a'

In [2]:
@Carreau

This comment has been minimized.

Copy link
Member Author

commented May 12, 2018

cc @minrk , cc @takluyver

@njsmith do you have any idea about what could have caused this ?

@Carreau

This comment has been minimized.

Copy link
Member Author

commented May 12, 2018

Ok, that seem to be because now the first string in a module is actually tagged as the module docstring. See 3.6:

In [1]: "q"
~/dev/ipython/IPython/core/interactiveshell.py:2800 | Module(body=[
    Expr(value=Str(s='q')),
  ])
Out[1]: 'q'

vs nightly:

In [2]: "q"
~/dev/ipython/IPython/core/interactiveshell.py:2800 | Module(body=[], docstring='q')

So we likely need to special case empty module with docstring ? Or is it something we need to report upstream ?

@njsmith

This comment has been minimized.

Copy link
Contributor

commented May 12, 2018

You should report it upstream, if only so they have a chance to decide whether to do it on purpose...

@serhiy-storchaka

This comment has been minimized.

Copy link

commented May 17, 2018

Can you use the "single" mode instead of the "exec" (default) mode when parse to AST?

@Carreau

This comment has been minimized.

Copy link
Member Author

commented May 17, 2018

Can you use the "single" mode instead of the "exec" (default) mode when parse to AST?

Unfortunately no, as a single cell in ipython/jupyter can be a sequence of statements/expressions.

If we try single we get a syntax errors:

In [1]: print(1+1)
   ...: print(2+2)
  File "<ipython-input-1-2cad5c9dfe23>", line 1
    print(1+1)
              ^
SyntaxError: multiple statements found while compiling a single statement

I modified the AST as a workaround in #11135

@serhiy-storchaka

This comment has been minimized.

Copy link

commented May 24, 2018

Would it help if first try single, and fallback to exec?

@Carreau

This comment has been minimized.

Copy link
Member Author

commented May 24, 2018

Well, single would return and Interactive node, while exec would return a Module, so that would require messing with AST anyway. Which not more complicated or simple than what I did in #11135.

I also though of unconditionally prepending an empty string to the source code.
Ideally I'd really like a clean way to compiling a file/module from a sequence of statement.

@njsmith

This comment has been minimized.

@minrk

This comment has been minimized.

Copy link
Member

commented May 25, 2018

I opened #11158 as a possibly simpler alternative. It recompiles empty modules with single and wraps that in a Module. It does technically 'mess with the ast' to get back a Module, but with a very minimal ast.Module(interactive.body). I also tested without wrapping it in a Module, and if we left it as Interactive everything would probably work because we really only ever do for node in module.body, but it's perhaps better to preserve the ast.Module assumption.

It's like @serhiy-storchaka's proposal, but the fallback is in the other direction, since most IPython inputs are multi-node inputs from notebooks these days, and currently this is only an issue for truly bare strings. This imposes the double-compile cost on the rarest and cheapest-compilation case, rather than the most-expensive long cells.

I don't view this as a major issue because executing cells that are just a string literal is pretty rare, and not displaying what you have necessarily just typed directly into the input isn't a huge regression for users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.