From 814e0a1661e14919550b532bddfab1cb0c38333b Mon Sep 17 00:00:00 2001 From: Jonathan Sick Date: Wed, 4 Sep 2024 14:45:59 -0400 Subject: [PATCH] Report check run error for bad notebook jinja 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. --- src/timessquare/domain/githubcheckrun.py | 20 +++++++++++++++++++- src/timessquare/domain/page.py | 15 ++++++++++++--- src/timessquare/exceptions.py | 18 ++++++++++++++++++ src/timessquare/services/githubcheckrun.py | 15 ++++++++++----- 4 files changed, 59 insertions(+), 9 deletions(-) diff --git a/src/timessquare/domain/githubcheckrun.py b/src/timessquare/domain/githubcheckrun.py index 4ca686d..9727128 100644 --- a/src/timessquare/domain/githubcheckrun.py +++ b/src/timessquare/domain/githubcheckrun.py @@ -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 ( @@ -29,7 +30,7 @@ RecursiveGitTreeModel, RepositoryNotebookTreeRef, ) -from .page import PageExecutionInfo +from .page import PageExecutionInfo, PageModel @dataclass(kw_only=True) @@ -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: diff --git a/src/timessquare/domain/page.py b/src/timessquare/domain/page.py index 4a0c0c7..70a08ee 100644 --- a/src/timessquare/domain/page.py +++ b/src/timessquare/domain/page.py @@ -19,6 +19,7 @@ from jsonschema import Draft202012Validator from timessquare.exceptions import ( + PageJinjaError, PageNotebookFormatError, PageParameterError, PageParameterValueCastingError, @@ -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 @@ -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 @@ -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: diff --git a/src/timessquare/exceptions.py b/src/timessquare/exceptions.py index ccf6bfc..7edfddf 100644 --- a/src/timessquare/exceptions.py +++ b/src/timessquare/exceptions.py @@ -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.""" diff --git a/src/timessquare/services/githubcheckrun.py b/src/timessquare/services/githubcheckrun.py index 7717e04..1f33948 100644 --- a/src/timessquare/services/githubcheckrun.py +++ b/src/timessquare/services/githubcheckrun.py @@ -24,6 +24,7 @@ GitHubConfigsCheck, NotebookExecutionsCheck, ) +from timessquare.exceptions import PageJinjaError from ..domain.page import PageExecutionInfo from ..storage.noteburst import NoteburstJobStatus @@ -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",