Merge pull request #54 from lsst-sqre/tickets/DM-40188

DM-40188: Replace first code cells with parameters

CHANGELOG.md

@@ -8,7 +8,20 @@ Collect fragments into this file with: scriv collect --version X.Y.Z
 
 <!-- scriv-insert-here -->
 
+<a id='changelog-0.9.0'></a>
+
+## 0.9.0 (2023-07-27)
+
+### Backwards-incompatible changes
+
+- New treatment for templating in notebooks. Times Square no longer treats all cells as Jinja templates. Instead, the first cell is considered to be under the control of Times Square for setting variables. When a parameters are rendered into a notebook, the first code cell is replaced with new source that sets the parameter variables to their values. This means that notebook authors can now use that first cell to set and experiment with parameter values for interactive debugging, knowing that Times Square will replace that cell with new values when the notebook is rendered.
+
+### Bug fixes
+
+- Parameter names are now validated against Python keyword names using `keyword.iskeyword()`. This prevents parameter names from shadowing Python keywords.
+
 <a id='changelog-0.8.0'></a>
+
 ## 0.8.0 (2023-07-27)
 
 ### New features

docs/user-guide/environment-variables.rst

@@ -2,7 +2,7 @@
 Environment variables
 #####################
 
-Noteburst uses environment variables for configuration.
+Times Square uses environment variables for configuration.
 In practice, these variables are typically set as Helm values and 1Password/Vault secrets that are injected into the container as environment variables.
 See the `Phalanx documentation for Times Square <https://phalanx.lsst.io/applications/times-square/index.html>`__ for more information on the Phalanx-specific configurations.
 
@@ -73,4 +73,4 @@ See the `Phalanx documentation for Times Square <https://phalanx.lsst.io/applica
 
 .. envvar:: TS_GITHUB_ORGS
 
-   (string) A comma-separated list of GitHub organizations that Times Square will sync notebooks from. This is used to filter out GitHub App installations from the general public.
+   (string) A comma-separated list of GitHub organizations that Times Square will sync notebooks from. This is used to filter out incidental GitHub App installations from the general public.

src/timessquare/domain/page.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import json
+import keyword
 import re
 from base64 import b64encode
 from collections.abc import Mapping
@@ -391,8 +392,16 @@ def create_and_validate_page_parameter(
 
     @staticmethod
     def validate_parameter_name(name: str) -> None:
+        """Validate a parameter's name.
+
+        Parameters must be valid Python variable names, which means they must
+        start with a letter and contain only letters, numbers and underscores.
+        They also cannot be Python keywords.
+        """
         if parameter_name_pattern.match(name) is None:
             raise ParameterNameValidationError(name)
+        if keyword.iskeyword(name):
+            raise ParameterNameValidationError(name)
 
     def resolve_and_validate_values(
         self, requested_values: Mapping[str, Any]
@@ -457,7 +466,19 @@ 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:
+            if cell.cell_type == "code":
+                if processed_first_cell is False:
+                    # Handle first code cell specially by replacing it with a
+                    # cell that sets Python variables to their values
+                    cell.source = self._create_parameters_template(values)
+                    processed_first_cell = True
+                else:
+                    # Only process the first code cell
+                    continue
+
+            # Render the templated cell
             template = jinja_env.from_string(cell.source)
             cell.source = template.render()
 
@@ -469,6 +490,18 @@ def render_parameters(
         # Render notebook back to a string and return
         return PageModel.write_ipynb(notebook)
 
+    def _create_parameters_template(self, values: Mapping[str, Any]) -> str:
+        """Create a Jinja-tempalated source cell value that sets Python
+        variables for each parameter to their values.
+        """
+        sorted_variables = sorted(values.keys())
+        code_lines = [
+            f"{variable_name} = {{{{ params.{variable_name} }}}}"
+            for variable_name in sorted_variables
+        ]
+        code_lines.insert(0, "# Parameters")
+        return "\n".join(code_lines)
+
 
 @dataclass
 class PersonModel:

tests/data/demo-invalid-params.ipynb

@@ -14,6 +14,18 @@
     "- Wavelength: lambd = {{ params.lambd}}"
    ]
   },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "70241f00",
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "A = 2\n",
+    "y0 = 4\n",
+    "lambd = 1"
+   ]
+  },
   {
    "cell_type": "code",
    "execution_count": null,
@@ -51,7 +63,7 @@
    "metadata": {},
    "outputs": [],
    "source": [
-    "plot({{ params.y0 }}, {{ params.A }}, {{ params.lambd }})"
+    "plot(y0, A, lambd)"
    ]
   }
  ],

tests/data/demo.ipynb

@@ -14,6 +14,18 @@
     "- Wavelength: lambd = {{ params.lambd}}"
    ]
   },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "f3c27327",
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "A = 2\n",
+    "y0 = 4\n",
+    "lambd = 1"
+   ]
+  },
   {
    "cell_type": "code",
    "execution_count": null,
@@ -51,7 +63,7 @@
    "metadata": {},
    "outputs": [],
    "source": [
-    "plot({{ params.y0 }}, {{ params.A }}, {{ params.lambd }})"
+    "plot(y0, A, lambd)"
    ]
   }
  ],

tests/domain/page_test.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+from pathlib import Path
 from typing import Any
 
 import pytest
@@ -23,7 +24,10 @@ def test_parameter_name_validation() -> None:
 
     with pytest.raises(ParameterNameValidationError):
         PageModel.validate_parameter_name(" M")
+    with pytest.raises(ParameterNameValidationError):
         PageModel.validate_parameter_name("1p")
+    with pytest.raises(ParameterNameValidationError):
+        PageModel.validate_parameter_name("lambda")
 
 
 def test_parameter_default_exists() -> None:
@@ -78,3 +82,36 @@ def test_parameter_casting() -> None:
     assert False is schema.cast_value("false")
     assert True is schema.cast_value(True)
     assert False is schema.cast_value(False)
+
+
+def test_render_parameters() -> None:
+    """Test PageModel.render_parameters()."""
+    ipynb_path = Path(__file__).parent.parent / "data" / "demo.ipynb"
+    ipynb = ipynb_path.read_text()
+    nb = PageModel.read_ipynb(ipynb)
+    page = PageModel.create_from_api_upload(
+        ipynb=ipynb,
+        title="Demo",
+        uploader_username="testuser",
+    )
+    rendered = page.render_parameters(values={"A": 2, "y0": 1.0, "lambd": 0.5})
+    rendered_nb = PageModel.read_ipynb(rendered)
+
+    # Check that the markdown got rendered
+    assert rendered_nb["cells"][0]["source"] == (
+        "# Times Square demo\n"
+        "\n"
+        "Plot parameters:\n"
+        "\n"
+        "- Amplitude: A = 2\n"
+        "- Y offset: y0 = 1.0\n"
+        "- Wavelength: lambd = 0.5"
+    )
+
+    # Check that the first code cell got replaced
+    assert rendered_nb["cells"][1]["source"] == (
+        "# Parameters\nA = 2\nlambd = 0.5\ny0 = 1.0"
+    )
+
+    # Check that the second code cell was unchanged
+    assert rendered_nb["cells"][2]["source"] == nb["cells"][2]["source"]