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

Line numbers in errors in the interactive window don't match the editor #3309

Closed
rchiodo opened this issue Jun 27, 2019 · 6 comments
Closed
Assignees

Comments

@rchiodo
Copy link
Contributor

rchiodo commented Jun 27, 2019

Cause an error, it will point to the error in the cell, but the line number doesn't match the original code.

@DonJayamanne DonJayamanne self-assigned this Jan 2, 2020
@DonJayamanne
Copy link
Contributor

DonJayamanne commented Jan 3, 2020

Ignore This

Suggestion

  • Monkey patch IPython class methods
  • Extract original exception information
    • Printing it to stderr enclosed within a special delimiters (hardcoded GUID)
    • When we get the output from jupyter we'll strip text between those delimiters and extract stack trace, error message, etc and other info from there to display in monaco editor or other places.
  • Store this new info in a new property of the cell (lets call it metadata, lack of a better word)
    • This will not be persisted with the file (ipynb)
    • Will allow storing other metadata related to cells (linter errors, whether a cell is selected, etc)
    • TBDiscussed
from IPython.core.interactiveshell import InteractiveShell
from functools import wraps
import traceback
import sys

def change_function(func):
    @wraps(func)
    def showtraceback(*args, **kwargs):
        # extract exception type, value and traceback
        etype, evalue, tb = sys.exc_info()

        orig_value = func(*args, **kwargs)

        if issubclass(etype, Exception):
            # Do something with the original exception, such as
            # Printing it to stderr enclosed within a special delimiter (hardcoded GUID)
            # When we get the output from jupyter we'll strip text between those delimiters and extract stack trace, error message, etc and other info from there to display in monaco.
           # Everything we need is in `etype, evalue, tb`
            return orig_value
    return showtraceback

InteractiveShell.showtraceback = change_function(InteractiveShell.showtraceback)
InteractiveShell.showsyntaxerror = change_function....(InteractiveShell.showsyntaxerror)

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Jan 3, 2020

Ignore This

I think this issue might as well display errors returned by the language server.
After all, that's kinda what we're doing here.
Questions & Notes

  • When we display jupyter errors, when should that error information be cleared out?

    • When user edits that particular line?
    • When user re-executes that particular cell?
  • Assume we have 3 lines in a cell

    • The error is in line 3
    • The user deletes line 1, the error should not be displayed in line 2
    • VS Code automatically handles this and ensures errors/decorations handle line edits.
    • We'll need to check if Monaco editor handles this on behalf of VS Code.
    • If not what is to be done? Should we throw away all execution errors as soon as a cell is modified? Doesn't seem right.
  • Should we be displaying errors returned by the language server?

    • If yes, then we'd need a spec for this.
    • We might have to add indicators in the margins
    • We'd need to ensure it looks just like the standard VS code errors in the editor
  • Persisting errors?

    • If a cell is executed and it contains errors. We display them
    • Now if the user saves the notebook and re-loads the notebook. We'd need to ensure the same lines of code in the cell are highlighted accordingly (i.e. user shouldn't have to re-execute the cells).
    • After all, the errors are displayed in the output window, hence the corresponding code/lines must be highlighted.
      *** This would indicate we might have to revise how we extract errors from output, instead of monkeypatching IPython**

@DonJayamanne
Copy link
Contributor

Moving back to backlog for discussions @IanMatthewHuff @rchiodo @DavidKutu

@DonJayamanne DonJayamanne removed their assignment Jan 3, 2020
@DonJayamanne
Copy link
Contributor

Actually my proposed solution is completely wrong.
It's addressing something completely different.

@rchiodo rchiodo self-assigned this Feb 6, 2020
@ihnorton
Copy link

ihnorton commented Feb 8, 2020

I wish I had seen microsoft/vscode-python#9984 four hours ago :)

Anyway, I got annoyed enough at this to start digging, and came up with a hack "solution" by prepending newlines:

ip = get_ipython()
del ip.input_transformers_cleanup[0]

before and after:

before, wrong line number:

...
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
/private/tmp/test.py in 
      1 x = 1
----> 2 a.meta['abc'] = (1,2.0)

after, correct line number:

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
/private/tmp/test.py in 
      5 
      6 
----> 7 a.meta['abc'] = (1,2.0)

In the course of doing that and digging through IPython code, I noticed that another solution would be to use an AST transformer -- assuming the line number offset could somehow be communicated, it would walk the top-level nodes of IPython's parsed code and adjust the line number. Is that something you folks have considered? (a possible reason not to do that is IIUC ast is not considered a stable interface, but not 100% sure on that)

edit: just to round this out, I tried the AST transformer solution, and while it works, the problem is that the cached code which IPython refers to when generating tracebacks does not of course have the correct offset applied (so the traceback still points to wrong or blank lines).

Another, similar option is an input transformer, with the following patch:

diff --git a/src/client/datascience/interactive-common/interactiveBase.ts b/src/client/datascience/interactive-common/interactiveBase.ts
index 6a67364b..df183087 100644
--- a/src/client/datascience/interactive-common/interactiveBase.ts
+++ b/src/client/datascience/interactive-common/interactiveBase.ts
@@ -513,6 +513,8 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
                     await this._notebook.execute(`__file__ = '${file.replace(/\\/g, '\\\\')}'`, file, line, uuid(), undefined, true);
                 }

+                await this._notebook.execute(`__line__ = ${line}`, file, line, uuid(), undefined, true);
+
                 const observable = this._notebook.executeObservable(code, file, line, id, false);

                 // Indicate we executed some code

And then:

ip = get_ipython()
def prepender(lines):
    lines = ['\n' for _ in range(int(__line__)+1)] + lines
    return lines
ip.input_transformers_cleanup.append(prepender)

@DonJayamanne
Copy link
Contributor

Validated

@lock lock bot locked as resolved and limited conversation to collaborators Feb 17, 2020
@microsoft microsoft unlocked this conversation Nov 14, 2020
@DonJayamanne DonJayamanne transferred this issue from microsoft/vscode-python Nov 14, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants