Skip to content

Commit

Permalink
Report check run error for bad notebook jinja
Browse files Browse the repository at this point in the history
If the Jinja templating in a notebook is incorrect, the
PageModel.render_parameters method raises a new exception,
PageJinjaError. This is captured in the notebook execution check run and
turned into useful user feedback.
  • Loading branch information
jonathansick committed Sep 4, 2024
1 parent a9dc7b6 commit 814e0a1
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 9 deletions.
20 changes: 19 additions & 1 deletion src/timessquare/domain/githubcheckrun.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from yaml import YAMLError

from timessquare.config import config
from timessquare.exceptions import PageJinjaError

from ..storage.noteburst import NoteburstJobResponseModel, NoteburstJobStatus
from .githubcheckout import (
Expand All @@ -29,7 +30,7 @@
RecursiveGitTreeModel,
RepositoryNotebookTreeRef,
)
from .page import PageExecutionInfo
from .page import PageExecutionInfo, PageModel


@dataclass(kw_only=True)
Expand Down Expand Up @@ -440,6 +441,23 @@ def __init__(
self.notebook_paths_checked: list[str] = []
super().__init__(check_run=check_run, repo=repo)

def report_jinja_error(
self, page: PageModel, error: PageJinjaError
) -> None:
"""Report an error rendering a Jinja template in a notebook cell."""
path = page.repository_source_path
if path is None:
raise RuntimeError("Page execution has no notebook path")
annotation = Annotation(
path=path,
start_line=1,
message=str(error),
title="Notebook Jinja templating error",
annotation_level=GitHubCheckRunAnnotationLevel.failure,
)
self.annotations.append(annotation)
self.notebook_paths_checked.append(path)

def report_noteburst_failure(
self, page_execution: PageExecutionInfo
) -> None:
Expand Down
15 changes: 12 additions & 3 deletions src/timessquare/domain/page.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from jsonschema import Draft202012Validator

from timessquare.exceptions import (
PageJinjaError,
PageNotebookFormatError,
PageParameterError,
PageParameterValueCastingError,
Expand Down Expand Up @@ -462,6 +463,11 @@ def render_parameters(
-------
ipynb : str
JSON-encoded notebook source.
Raises
------
PageJinjaError
Raised if there is an error rendering the Jinja template.
"""
# Build Jinja render context
# Turn off autoescaping to avoid escaping the parameter values
Expand All @@ -474,7 +480,7 @@ def render_parameters(
# Read notebook and render cell-by-cell
notebook = PageModel.read_ipynb(self.ipynb)
processed_first_cell = False
for cell in notebook.cells:
for cell_index, cell in enumerate(notebook.cells):
if cell.cell_type == "code":
if processed_first_cell is False:
# Handle first code cell specially by replacing it with a
Expand All @@ -486,8 +492,11 @@ def render_parameters(
continue

# Render the templated cell
template = jinja_env.from_string(cell.source)
cell.source = template.render()
try:
template = jinja_env.from_string(cell.source)
cell.source = template.render()
except Exception as e:
raise PageJinjaError(str(e), cell_index) from e

# Modify notebook metadata to include values
if "times-square" not in notebook.metadata:
Expand Down
18 changes: 18 additions & 0 deletions src/timessquare/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,24 @@ def for_value(
return cls(message, location=location, field_path=field_path)


class PageJinjaError(Exception):
"""An error occurred while rendering a template in a notebook cell.
This error is raised duirng the notebook check run.
"""

def __init__(self, message: str, cell_index: int) -> None:
"""Create an exception with a message and cell index."""
super().__init__(message)
self.cell_index = cell_index

def __str__(self) -> str:
return (
"Error rendering template in "
f"cell {self.cell_index + 1}: {self.args[0]}"
)


class ParameterSchemaValidationError(TimesSquareClientError):
"""Error related to a parameter."""

Expand Down
15 changes: 10 additions & 5 deletions src/timessquare/services/githubcheckrun.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
GitHubConfigsCheck,
NotebookExecutionsCheck,
)
from timessquare.exceptions import PageJinjaError

from ..domain.page import PageExecutionInfo
from ..storage.noteburst import NoteburstJobStatus
Expand Down Expand Up @@ -179,12 +180,16 @@ async def run_notebook_check_run( # noqa: C901 PLR0912 PLR0915
notebook=notebook,
commit_sha=check_run.head_sha,
)
page_execution_info = (
await self._page_service.execute_page_with_defaults(
page,
enable_retry=False, # fail quickly for CI
try:
page_execution_info = (
await self._page_service.execute_page_with_defaults(
page,
enable_retry=False, # fail quickly for CI
)
)
)
except PageJinjaError as e:
check.report_jinja_error(page, e)

if page_execution_info.noteburst_error_message is not None:
self._logger.debug(
"Got immediate noteburst error",
Expand Down

0 comments on commit 814e0a1

Please sign in to comment.