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.

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,

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

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
 

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

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=[])

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) {

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;
 }