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

Fix notebook error formatting #2695

Merged
merged 2 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
"editor.rulers": [100],
},
"python.testing.pytestArgs": [
"tests"
"tests",
"positron/positron_ipykernel/tests"
],
"python.testing.unittestEnabled": false,
"python.testing.pytestEnabled": true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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__)


Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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}".
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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=[])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Loading