Merge pull request #80 from lsst-sqre:tickets/DM-46095

DM-46095: Improve GitHub check run feedback

CHANGELOG.md

@@ -8,6 +8,27 @@ Collect fragments into this file with: scriv collect --version X.Y.Z
 
 <!-- scriv-insert-here -->
 
+<a id='changelog-0.12.0'></a>
+
+## 0.12.0 (2024-09-04)
+
+### New features
+
+- Improved feedback in GitHub Check Runs:
+
+  - If a YAML file has bad syntax causing a parsing error, we now post the YAML error as a check run annotation — including the location of the error in the file.
+
+  - If the notebook has incorrect Jinja templating in its Markdown cells, we now post a check run annotation with the error message and cell number.
+
+### Other changes
+
+- Adopt uv and tox-uv for pinning and installing dependencies
+- Pin the tox dependencies with a `requirements/tox.[in|txt]` file
+- Adapt configuration for tox-docker version 5's randomized connection ports
+- Adopt [ruff-shared.toml](https://github.com/lsst/templates/blob/main/project_templates/fastapi_safir_app/example/ruff-shared.toml) for common SQuaRE ruff configuration.
+
+- The GitHub Check Run service methods are now part of a new `GitHubCheckRunService` class, separate from the `GitHubRepoService`. This internal change helps clarify what functionality is needed for the GitHub Checks functionality, as opposed to syncing data with GitHub repositories.
+
 <a id='changelog-0.11.0'></a>
 
 ## 0.11.0 (2024-03-27)

Dockerfile

@@ -14,7 +14,7 @@
 #   - Runs a non-root user.
 #   - Sets up the entrypoint and port.
 
-FROM python:3.12.5-slim-bullseye as base-image
+FROM python:3.12.5-slim-bookworm as base-image
 
 # Update system packages
 COPY scripts/install-base-packages.sh .

src/timessquare/domain/githubcheckrun.py

@@ -18,8 +18,10 @@
     GitHubCheckRunStatus,
     GitHubRepositoryModel,
 )
+from yaml import YAMLError
 
 from timessquare.config import config
+from timessquare.exceptions import PageJinjaError
 
 from ..storage.noteburst import NoteburstJobResponseModel, NoteburstJobStatus
 from .githubcheckout import (
@@ -28,7 +30,7 @@
     RecursiveGitTreeModel,
     RepositoryNotebookTreeRef,
 )
-from .page import PageExecutionInfo
+from .page import PageExecutionInfo, PageModel
 
 
 @dataclass(kw_only=True)
@@ -51,6 +53,7 @@ class Annotation:
     def from_validation_error(
         cls, path: str, error: ValidationError
     ) -> list[Annotation]:
+        """Create a list of annotations from a Pydantic validation error."""
         annotations: list[Annotation] = []
         for item in error.errors():
             title = cls._format_title_for_pydantic_item(item["loc"])
@@ -65,6 +68,34 @@ def from_validation_error(
             )
         return annotations
 
+    @classmethod
+    def from_yaml_error(cls, path: str, error: YAMLError) -> list[Annotation]:
+        """Create a list of annotations from a YAML syntax error."""
+        if hasattr(error, "problem_mark"):
+            # The YAML syntax error has a problem mark pointing to the
+            # location of the error.
+            start_line = error.problem_mark.line + 1
+            column = error.problem_mark.column + 1
+            return [
+                Annotation(
+                    path=path,
+                    start_line=start_line,
+                    message=str(error),
+                    title=f"YAML syntax error ({start_line}:{column})",
+                    annotation_level=GitHubCheckRunAnnotationLevel.failure,
+                )
+            ]
+        return [
+            # The exact location is unknown
+            Annotation(
+                path=path,
+                start_line=1,
+                message=str(error),
+                title="YAML syntax error",
+                annotation_level=GitHubCheckRunAnnotationLevel.failure,
+            )
+        ]
+
     @staticmethod
     def _format_title_for_pydantic_item(locations: Sequence[str | int]) -> str:
         title_elements: list[str] = []
@@ -234,7 +265,7 @@ async def create_check_run_and_validate(
                 "external_id": cls.external_id,
             },
         )
-        check_run = GitHubCheckRunModel.parse_obj(data)
+        check_run = GitHubCheckRunModel.model_validate(data)
         return await cls.validate_repo(
             check_run=check_run,
             github_client=github_client,
@@ -258,6 +289,7 @@ async def validate_repo(
         check = cls(check_run, repo)
         await check.submit_in_progress(github_client)
 
+        # Check out the repository and validate the times-square.yaml file.
         try:
             checkout = await GitHubRepositoryCheckout.create(
                 github_client=github_client,
@@ -270,6 +302,12 @@ async def validate_repo(
             )
             check.annotations.extend(annotations)
             return check
+        except YAMLError as e:
+            annotations = Annotation.from_yaml_error(
+                path="times-square.yaml", error=e
+            )
+            check.annotations.extend(annotations)
+            return check
 
         # Validate each notebook yaml file
         tree = await checkout.get_git_tree(github_client)
@@ -303,14 +341,19 @@ async def validate_sidecar(
             repo.blobs_url,
             url_vars={"sha": notebook_ref.sidecar_git_tree_sha},
         )
-        sidecar_blob = GitHubBlobModel.parse_obj(data)
+        sidecar_blob = GitHubBlobModel.model_validate(data)
         try:
             NotebookSidecarFile.parse_yaml(sidecar_blob.decode())
         except ValidationError as e:
             annotations = Annotation.from_validation_error(
                 path=notebook_ref.sidecar_path, error=e
             )
             self.annotations.extend(annotations)
+        except YAMLError as e:
+            annotations = Annotation.from_yaml_error(
+                path=notebook_ref.sidecar_path, error=e
+            )
+            self.annotations.extend(annotations)
         self.sidecar_files_checked.append(notebook_ref.sidecar_path)
 
     @property
@@ -398,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:

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:

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."""