From 7cbd63ae211501183d90a2b03b24e0a4db3dc275 Mon Sep 17 00:00:00 2001 From: Wasim Lorgat Date: Fri, 12 Apr 2024 12:47:23 +0200 Subject: [PATCH] Fix notebook error formatting (#2695) Don't modify tracebacks in notebook mode. The notebook frontend assumes the format of tracebacks and adds links to them. If we modify the traceback, the frontend's formatting breaks. Also minor improvements to Positron notebook UI error styling. --- .../python_files/.vscode/settings.json | 3 +- .../positron_ipykernel/positron_ipkernel.py | 51 ++++++++++++++++++- .../positron_ipykernel/tests/conftest.py | 23 ++++++--- .../tests/test_positron_ipkernel.py | 46 ++++++++++++++++- .../positron/positron_language_server.py | 24 ++++----- .../browser/getOutputContents.ts | 2 +- .../browser/notebookCells/CellTextOutput.css | 2 + 7 files changed, 127 insertions(+), 24 deletions(-) diff --git a/extensions/positron-python/python_files/.vscode/settings.json b/extensions/positron-python/python_files/.vscode/settings.json index 480631710e8..165fbc8b9fc 100644 --- a/extensions/positron-python/python_files/.vscode/settings.json +++ b/extensions/positron-python/python_files/.vscode/settings.json @@ -9,7 +9,8 @@ "editor.rulers": [100], }, "python.testing.pytestArgs": [ - "tests" + "tests", + "positron/positron_ipykernel/tests" ], "python.testing.unittestEnabled": false, "python.testing.pytestEnabled": true, diff --git a/extensions/positron-python/python_files/positron/positron_ipykernel/positron_ipkernel.py b/extensions/positron-python/python_files/positron/positron_ipykernel/positron_ipkernel.py index d3356795800..01b81885ead 100644 --- a/extensions/positron-python/python_files/positron/positron_ipykernel/positron_ipkernel.py +++ b/extensions/positron-python/python_files/positron/positron_ipykernel/positron_ipkernel.py @@ -11,7 +11,7 @@ import re import warnings from pathlib import Path -from typing import Any, Callable, Container, Dict, List, Optional, Type +from typing import Any, Callable, Container, Dict, List, Optional, Type, cast import traitlets from ipykernel.comm.manager import CommManager @@ -45,6 +45,26 @@ class _CommTarget(str, enum.Enum): Connections = "positron.connection" +class SessionMode(str, enum.Enum): + """ + The mode that the kernel application was started in. + """ + + Console = "console" + Notebook = "notebook" + Background = "background" + + Default = Console + + def __str__(self) -> str: + # Override for better display in argparse help. + return self.value + + @classmethod + def trait(cls) -> traitlets.Enum: + return traitlets.Enum(sorted(cls), help=cls.__doc__) + + logger = logging.getLogger(__name__) @@ -138,6 +158,18 @@ class PositronShell(ZMQInteractiveShell): help="Class to use to instantiate the shell inspector", ).tag(config=True) + # Positron-specific attributes: + session_mode: SessionMode = SessionMode.trait() # type: ignore + + def __init__(self, *args, **kwargs): + # Set custom attributes from the parent object. + # It would be better to pass these as explicit arguments, but there's no easy way + # to override the parent to do that. + parent = cast(PositronIPyKernel, kwargs["parent"]) + self.session_mode = parent.session_mode + + super().__init__(*args, **kwargs) + def init_events(self) -> None: super().init_events() @@ -250,6 +282,11 @@ def _showtraceback(self, etype, evalue: Exception, stb: List[str]): # type: ign """ Enhance tracebacks for the Positron frontend. """ + if self.session_mode == SessionMode.Notebook: + # Don't modify the traceback in a notebook. The frontend assumes that it's unformatted + # and applies its own formatting. + return super()._showtraceback(etype, evalue, stb) # type: ignore IPython type annotation is wrong + # Remove the first two lines of the traceback, which are the "---" header and the repeated # exception name and "Traceback (most recent call last)". # Also remove the last line of the traceback, which repeats f"{etype}: {evalue}". @@ -305,7 +342,16 @@ class PositronIPyKernel(IPythonKernel): klass=InteractiveShell, ) + # Positron-specific attributes: + session_mode: SessionMode = SessionMode.trait() # type: ignore + def __init__(self, **kwargs) -> None: + # Set custom attributes from the parent object. + # It would be better to pass these as explicit arguments, but there's no easy way + # to override the parent to do that. + parent = cast(PositronIPKernelApp, kwargs["parent"]) + self.session_mode = parent.session_mode + super().__init__(**kwargs) # Create Positron services @@ -383,6 +429,9 @@ class PositronIPKernelApp(IPKernelApp): # Use the PositronIPyKernel class. kernel_class: Type[PositronIPyKernel] = traitlets.Type(PositronIPyKernel) # type: ignore + # Positron-specific attributes: + session_mode: SessionMode = SessionMode.trait() # type: ignore + # # OSC8 functionality diff --git a/extensions/positron-python/python_files/positron/positron_ipykernel/tests/conftest.py b/extensions/positron-python/python_files/positron/positron_ipykernel/tests/conftest.py index 8bac479aaec..66bbb6affc9 100644 --- a/extensions/positron-python/python_files/positron/positron_ipykernel/tests/conftest.py +++ b/extensions/positron-python/python_files/positron/positron_ipykernel/tests/conftest.py @@ -3,13 +3,19 @@ # from typing import Iterable -from unittest.mock import Mock +from unittest.mock import MagicMock, Mock import comm import pytest +from traitlets.config import Config from positron_ipykernel.connections import ConnectionsService from positron_ipykernel.data_explorer import DataExplorerService -from positron_ipykernel.positron_ipkernel import PositronIPyKernel, PositronShell +from positron_ipykernel.positron_ipkernel import ( + PositronIPKernelApp, + PositronIPyKernel, + PositronShell, + SessionMode, +) from positron_ipykernel.variables import VariablesService @@ -43,10 +49,15 @@ def kernel() -> PositronIPyKernel: """ The Positron kernel, configured for testing purposes. """ - # Create a Positron kernel. The kernel calls shell_class.instance() to get the globally - # registered shell instance, and IPython registers a TerminalInteractiveShell instead of a - # PositronShell. This causes a traitlets validation error unless we pass the shell_class explicitly. - kernel = PositronIPyKernel.instance(shell_class=PositronShell) + # Mock the application object. We haven't needed to use it in tests yet, but we do need it to + # pass our custom attributes down to the shell. + app = MagicMock(PositronIPKernelApp) + app.config = Config() # Needed to avoid traitlets errors + + # Positron-specific attributes: + app.session_mode = SessionMode.Console + + kernel = PositronIPyKernel.instance(parent=app) return kernel diff --git a/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_positron_ipkernel.py b/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_positron_ipkernel.py index 966027c8599..a47cd53aa20 100644 --- a/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_positron_ipkernel.py +++ b/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_positron_ipkernel.py @@ -10,6 +10,7 @@ import pandas as pd import pytest from IPython.utils.syspathcontext import prepended_to_syspath +from positron_ipykernel.positron_ipkernel import SessionMode from positron_ipykernel.help import help from positron_ipykernel.utils import alias_home @@ -81,7 +82,12 @@ def g(): """ -def test_traceback(shell: PositronShell, tmp_path: Path, mock_displayhook: Mock) -> None: +def test_console_traceback( + shell: PositronShell, tmp_path: Path, mock_displayhook: Mock, monkeypatch +) -> None: + # Ensure that we're in console mode. + monkeypatch.setattr(shell, "session_mode", SessionMode.Console) + # We follow the approach of IPython's test_ultratb.py, which is to create a temporary module, # prepend its parent directory to sys.path, import it, then run a cell that calls a function # from it. @@ -167,6 +173,44 @@ def test_traceback(shell: PositronShell, tmp_path: Path, mock_displayhook: Mock) ) +def test_notebook_traceback( + shell: PositronShell, tmp_path: Path, mock_displayhook: Mock, monkeypatch +) -> None: + # Ensure that we're in notebook mode. + monkeypatch.setattr(shell, "session_mode", SessionMode.Notebook) + + # We follow the approach of IPython's test_ultratb.py, which is to create a temporary module, + # prepend its parent directory to sys.path, import it, then run a cell that calls a function + # from it. + + # Create a temporary module. + file = tmp_path / "test_traceback.py" + file.write_text(code) + + # Temporarily add the module to sys.path and call a function from it, which should error. + with prepended_to_syspath(str(tmp_path)): + shell.run_cell("import test_traceback; test_traceback.g()") + + # Check that a single message was sent to the frontend. + call_args_list = mock_displayhook.session.send.call_args_list + assert len(call_args_list) == 1 + + call_args = call_args_list[0] + + # Check that the message was sent over the "error" stream. + assert call_args.args[1] == "error" + + exc_content = call_args.args[2] + + # Check that we haven't removed any frames. + # We don't check the traceback contents in this case since that's tested in IPython. + assert len(exc_content["traceback"]) == 6 + + # Check that we haven't modified any other contents. + assert exc_content["ename"] == "Exception" + assert exc_content["evalue"] == "This is an error!" + + def assert_ansi_string_startswith(actual: str, expected: str) -> None: """ Assert that an ansi-formatted string starts with an expected string, in a way that gets pytest diff --git a/extensions/positron-python/python_files/positron/positron_language_server.py b/extensions/positron-python/python_files/positron/positron_language_server.py index 77da0b54b0c..ed1f304f712 100644 --- a/extensions/positron-python/python_files/positron/positron_language_server.py +++ b/extensions/positron-python/python_files/positron/positron_language_server.py @@ -9,9 +9,8 @@ import os import sys -from positron_ipykernel.positron_ipkernel import PositronIPKernelApp +from positron_ipykernel.positron_ipkernel import PositronIPKernelApp, SessionMode from positron_ipykernel.positron_jedilsp import POSITRON -from traitlets.config import Config logger = logging.getLogger(__name__) @@ -57,9 +56,9 @@ def parse_args() -> argparse.Namespace: parser.add_argument( "--session-mode", help="session mode in which the kernel is to be started", - type=str, - default="console", - choices=["console", "notebook", "background"], + type=SessionMode, + default=SessionMode.Default, + choices=sorted(SessionMode), ) args = parser.parse_args() args.loglevel = args.loglevel.upper() @@ -108,20 +107,17 @@ def parse_args() -> argparse.Namespace: } } - # Start Positron's IPyKernel as the interpreter for our console. # IPKernelApp expects an empty string if no connection_file is provided. if args.connection_file is None: args.connection_file = "" - config = Config( - IPKernelApp={ - "connection_file": args.connection_file, - "log_level": args.loglevel, - "logging_config": logging_config, - }, + # Start Positron's IPyKernel as the interpreter for our console. + app: PositronIPKernelApp = PositronIPKernelApp.instance( + connection_file=args.connection_file, + log_level=args.loglevel, + logging_config=logging_config, + session_mode=args.session_mode, ) - - app: PositronIPKernelApp = PositronIPKernelApp.instance(config=config) # Initialize with empty argv, otherwise BaseIPythonApplication.initialize reuses our # command-line arguments in unexpected ways (e.g. logfile instructs it to log executed code). app.initialize(argv=[]) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/getOutputContents.ts b/src/vs/workbench/contrib/positronNotebook/browser/getOutputContents.ts index 7d02fd0bfe4..e8002c03538 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/getOutputContents.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/getOutputContents.ts @@ -122,7 +122,7 @@ export function parseOutputData(output: ICellOutput['outputs'][number]): ParsedO } if (mime === 'application/vnd.code.notebook.error') { - return { type: 'error', content: parsedMessage.message }; + return { type: 'error', content: parsedMessage.stack }; } } catch (e) { diff --git a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellTextOutput.css b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellTextOutput.css index 6c96a8dc199..4e15d70b414 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellTextOutput.css +++ b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellTextOutput.css @@ -4,4 +4,6 @@ .positron-notebook-text-output { font-family: var(--monaco-monospace-font, monospace); + font-size: var(--notebook-cell-output-font-size, 12px); + white-space: pre-wrap; }