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

Inside of a notebook, magic cells should not cause an error in pylance #17058

Closed
rchiodo opened this issue Jan 28, 2021 · 22 comments · Fixed by #17057
Closed

Inside of a notebook, magic cells should not cause an error in pylance #17058

rchiodo opened this issue Jan 28, 2021 · 22 comments · Fixed by #17057
Assignees
Milestone

Comments

@rchiodo
Copy link

rchiodo commented Jan 28, 2021

Add a cell like so:

%connect_info

That is a valid magic command. It would be nice if Pylance would recognize these.

@jakebailey
Copy link
Member

I assume this would be blocked by "real" notebook cell support over the LSP, since we don't want to allow this in regular Python code (or maybe we special case the vscode-notebook schema sent for the fake concat docs). Right now, we aren't notebook aware at all (and don't know how to parse these).

@rchiodo
Copy link
Author

rchiodo commented Jan 28, 2021

Well actually we want them in regular python code if using the interactive window too (but that seems less important).

I was hoping we could just special case these somehow. Either in the concatenation layer or in pylance itself based on some way of detecting notebook cells.

@rchiodo
Copy link
Author

rchiodo commented Jan 28, 2021

Without having to go full blown notebook in the LSP

@jakebailey
Copy link
Member

This is silly, but one way to hack it would be to replace every line that starts with % with an equivalent number of spaces before sending it.

@jakebailey
Copy link
Member

Or, replace % with #.

@jakebailey
Copy link
Member

Unless the line is a continuation of the % operator. (I don't know what the syntax of magic cells are when distinguishing them from regular % uses.)

@rchiodo
Copy link
Author

rchiodo commented Jan 28, 2021

Well ideally autocomplete would work. % is actually the prefix for cell magics. There's like 100 of them that are allowed.

@jakebailey
Copy link
Member

Not having a syntax error is one thing, trying to autocomplete them is another. Does the notebook code already discard the syntax error or was that the main thing you were looking for?

@rchiodo
Copy link
Author

rchiodo commented Jan 28, 2021

No it doesn't discard the syntax error at the moment. The red squiggle comes up. I agree the first step would be to not generate the squiggle.

Second step is autocomplete.

@rchiodo
Copy link
Author

rchiodo commented Jan 29, 2021

Here's another example of something similar:
microsoft/vscode-jupyter#4554

Maybe there's something we can do for notebooks to remove diagnostics that are not relevant for a cell. I was thinking do the removal at the concat level (as it knows about notebooks). Similar to what @jakebailey suggested above about adding extra spaces

@jakebailey
Copy link
Member

Wild, I didn't know ! was a thing. Is there some reference somewhere that lists them so we don't forget any?

@rchiodo
Copy link
Author

rchiodo commented Jan 29, 2021

I haven't found an easy reference, but if you read the docs here it ends up listing all the goofy things you can do with jupyter:
https://ipython.readthedocs.io/en/stable/interactive/reference.html

Well at least in an ipython kernel.

@jakebailey
Copy link
Member

Wow, that's a lot. 🙁

@ScottNotFound
Copy link

Apparently in regular python files you do something like

#%%
import os
print(os.getcwd())
#! %cd ..
print(os.getcwd())

You get the magic executed and avoid the linting errors. Related: microsoft/vscode-jupyter#3263
I tried to use this as a workaround, but in a notebook this doesn't work. The magic won't execute when it is commented. Perhaps that's something for the jupyter extension to fix. I don't know if it would be best for the jupyter extension to just eat these or have them handled specifically by pylance, or any other language server for that matter. I guess magic autocompletion would be neat, but I don't really see more than a few line magics in notebooks anyway.

@rchiodo
Copy link
Author

rchiodo commented Jun 22, 2021

I think we were going to handle these in the python extension (where we parse the diagnostics as they come back) and skip ones for magics.

@alexdima
Copy link
Member

I wanted to try and make the case that this issue should be treated more like a bug.

I see how this can be considered an enhancement (it is a new scenario which was not considered initially), but for a jupyter notebook beginner like myself, a red squiggle in a notebook indicates that I did something wrong, and a squiggle on the first character of a notebook indicates that perhaps I have a setup issue. Despite the error, the cell actually executed without a problem. IMHO, in general, it is problematic if a tool reports a squiggle and I have no possibility to get rid of it; this erodes my trust in the tool. This can also be frustrating for users who want to have a clean Problems panel.

image

@dba-be
Copy link

dba-be commented Jul 6, 2021

Fully agree with @alexdima, since the new release of VS code as referenced here magic commands are shown as 'major errors' in VS Code, which should either be suppressible or shouldn't be detected in a notebook.

image

@nikitakuklev
Copy link

If this issue can be fixed, it would be awesome to include awareness of imports and functions in %run [bla.ipynb/.py]. I use shared notebooks with data loading helpers and imports for many types of analysis, and having half your cells get highlighted as undefined is no bueno.

@rchiodo rchiodo transferred this issue from microsoft/pylance-release Jul 12, 2021
@jakebailey
Copy link
Member

FWIW my suggestion here is to not ignore the diagnostic, but to replace the magic lines with an equivalent number of spaces when doing the translation, if possible. I'm not sure how exactly the cells are merged, so maybe that's not hard, but that would prevent follow-on errors that depend on how we recover from the invalid syntax that these magic lines have.

@timoklimmer
Copy link
Member

Happy to read that this issue is being addressed - the wiggles are really annoying. As far as I understand, ipykernel adds three syntax additions that are relevant and should be addressed here.

  1. line magics, eg. %pip. Those are by definition just single lines and can probably be handled by simply replacing % against # when the % is at the beginning of the line. I guess this would be a good start already, although this approach is not 100% exact yet -- depending on the options available here, it could be improved by testing if the %... token is part of a literal and/or by testing if the name of the magic is known. However, in case of checking against magics names, I would appreciate an algorithm which can cope with arbitrary magics and would not only test for some well-known magics names.

  2. cell magics., eg. %%writefile. Cell magics pass the entire content of the cell to the corresponding magic. The content can be any language. My suggestion here would be to just ignore the entire cell if it starts with %%, and if it is parsed at all, it should be parsed by the language server that belongs to the language of the magic but not by default by a Python language server.

  3. System shell access, eg. !ls. In principle the same as line magics, with the only difference that the content of the line is executed as a shell command if it starts with a !.

Sometimes, ipykernel also expands variables, ie. it replaces variable tokens like $my_var with corresponding variable values defined outside of the cell. However, Python cells seem not to do this variable expansion, hence can likely be neglected in this case.

@Olshansk
Copy link

A potential workaround found in this StackOverflow answer is to use iPythons's run_line_magic API:

For example:

from IPython import get_ipython
get_ipython().run_line_magic('connect_info')

@rchiodo rchiodo self-assigned this Aug 16, 2021
@rchiodo rchiodo transferred this issue from microsoft/vscode-jupyter Aug 20, 2021
@github-actions github-actions bot added the triage-needed Needs assignment to the proper sub-team label Aug 20, 2021
@rchiodo
Copy link
Author

rchiodo commented Aug 20, 2021

Moving to python since the fix will be there. I can now add a news entry

@rchiodo rchiodo added area-data science and removed triage-needed Needs assignment to the proper sub-team labels Aug 20, 2021
@rchiodo rchiodo added this to the August 2021 milestone Aug 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants