🐛 FIX: nbfomat v4.5 cell IDs (#62)

v4.5 notebooks now contain `cell.id`.
To deal with this, we always hash the notebooks as v4.4 (with ids removed).
Merging cache into a notebook now also preserves the input notebook minor version,
adding or removing `cell.id` where required.

Additionally, tests for timeouts are improved, to be more deterministic.

Author: chrisjsewell

.circleci/config.yml

@@ -26,7 +26,7 @@ jobs:
     strategy:
       matrix:
         os: [ubuntu-latest]
-        python-version: [3.6, 3.7, 3.8]
+        python-version: [3.6, 3.7, 3.8, 3.9]
         include:
           - os: windows-latest
             python-version: 3.7

.github/workflows/tests.yml

@@ -15,9 +15,13 @@ repos:
     - id: check-yaml
     - id: end-of-file-fixer
     - id: trailing-whitespace
-    - id: flake8
 
   - repo: https://github.com/psf/black
     rev: 20.8b1
     hooks:
     - id: black
+
+  - repo: https://gitlab.com/pycqa/flake8
+    rev: 3.8.4
+    hooks:
+    - id: flake8

.pre-commit-config.yaml

@@ -29,7 +29,7 @@
    "metadata": {},
    "source": [
     "```{note}\n",
-    "The full Jupyter notebook for this page can accessed here; {jupyter-download:notebook}`api`.\n",
+    "The full Jupyter notebook for this page can accessed here; {nb-download:notebook.ipynb}`api`.\n",
     "Try it for yourself!\n",
     "```"
    ]

docs/using/api.ipynb

@@ -1,5 +1,5 @@
 # NOTE: never import anything here, in order to maintain CLI speed
-__version__ = "0.4.1"
+__version__ = "0.4.2a1"
 
 
 def get_cache(path, cache_cls=None):

jupyter_cache/__init__.py

@@ -195,7 +195,7 @@ def match_cache_file(self, path: str) -> NbCacheRecord:
 
         :raises KeyError: if no match is found
         """
-        notebook = nbf.read(path, NB_VERSION)
+        notebook = nbf.read(path, nbf.NO_CONVERT)
         return self.match_cache_notebook(notebook)
 
     @abstractmethod
@@ -229,7 +229,7 @@ def merge_match_into_file(
         :raises KeyError: if no match is found
         :return: pk, input notebook with cached code cells and metadata merged.
         """
-        nb = nbf.read(str(path), NB_VERSION)
+        nb = nbf.read(str(path), nbf.NO_CONVERT)
         return self.merge_match_into_notebook(nb, nb_meta, cell_meta)
 
     @abstractmethod
@@ -249,7 +249,7 @@ def diff_nbfile_with_cache(
 
         Note: this will not diff markdown content, since it is not stored in the cache.
         """
-        nb = nbf.read(path, NB_VERSION)
+        nb = nbf.read(path, nbf.NO_CONVERT)
         return self.diff_nbnode_with_cache(pk, nb, uri=path, as_str=as_str, **kwargs)
 
     @abstractmethod

jupyter_cache/base.py

@@ -112,54 +112,63 @@ def change_cache_limit(self, size: int):
         assert isinstance(size, int) and size > 0
         Setting.set_value(CACHE_LIMIT_KEY, size, self.db)
 
-    def _create_hashable_nb(
+    def create_hashed_notebook(
         self,
         nb: nbf.NotebookNode,
-        compare_nb_meta=("kernelspec",),
-        compare_cell_meta=None,
-    ):
-        """Create a notebook containing only content desired for hashing."""
+        nb_metadata: Optional[Iterable[str]] = ("kernelspec",),
+        cell_metadata: Optional[Iterable[str]] = None,
+    ) -> Tuple[nbf.NotebookNode, str]:
+        """Convert a notebook to a standard format and hash.
+
+        Note: we always hash notebooks as version 4.4,
+        to allow for matching notebooks of different versions
+
+        :param nb_metadata: The notebook metadata keys to hash (if None, use all)
+        :param cell_metadata: The cell metadata keys to hash (if None, use all)
+
+        :return: (notebook, hash)
+        """
+        # copy the notebook
         nb = copy.deepcopy(nb)
-        nb.metadata = nbf.from_dict(
+        # update the notebook to consistent version 4.4
+        nb = nbf.convert(nb, to_version=NB_VERSION)
+        if nb.nbformat_minor > 5:
+            raise CachingError("notebook version greater than 4.5 not yet supported")
+        # remove non-code cells
+        nb.cells = [cell for cell in nb.cells if cell.cell_type == "code"]
+        # create notebook for hashing, with selected metadata
+        hash_nb = nbf.from_dict(
             {
-                k: v
-                for k, v in nb.metadata.items()
-                if compare_nb_meta is None or (k in compare_nb_meta)
+                "nbformat": nb.nbformat,
+                "nbformat_minor": 4,  # v4.5 include cell ids, which we do not cache
+                "metadata": {
+                    k: v
+                    for k, v in nb.metadata.items()
+                    if nb_metadata is None or (k in nb_metadata)
+                },
+                "cells": [
+                    {
+                        "cell_type": cell.cell_type,
+                        "source": cell.source,
+                        "metadata": {
+                            k: v
+                            for k, v in cell.metadata.items()
+                            if cell_metadata is None or (k in cell_metadata)
+                        },
+                        "execution_count": None,
+                        "outputs": [],
+                    }
+                    for cell in nb.cells
+                    if cell.cell_type == "code"
+                ],
             }
         )
-        diff_cells = []
-        for cell in nb.cells:
-            if cell.cell_type != "code":
-                continue
-            diff_cell = nbf.from_dict(
-                {
-                    "cell_type": cell.cell_type,
-                    "source": cell.source,
-                    "metadata": {
-                        k: v
-                        for k, v in cell.metadata.items()
-                        if compare_cell_meta is None or (k in compare_cell_meta)
-                    },
-                    "execution_count": None,
-                    "outputs": [],
-                }
-            )
-            diff_cells.append(diff_cell)
-        nb.cells = diff_cells
-        return nb
 
-    def _hash_notebook(
-        self,
-        nb: nbf.NotebookNode,
-        compare_nb_meta=("kernelspec",),
-        compare_cell_meta=None,
-    ):
-        """Create a hashkey for a notebook bundle."""
-        nb = self._create_hashable_nb(
-            nb, compare_nb_meta=compare_nb_meta, compare_cell_meta=compare_cell_meta
-        )
-        string = nbf.writes(nb, NB_VERSION)
-        return hashlib.md5(string.encode()).hexdigest()
+        # hash notebook
+        string = nbf.writes(hash_nb, nbf.NO_CONVERT)
+        hash_string = hashlib.md5(string.encode()).hexdigest()
+
+        return (nb, hash_string)
 
     def _validate_nb_bundle(self, nb_bundle: NbBundleIn):
         """Validate that a notebook bundle should be cached.
@@ -182,13 +191,6 @@ def _validate_nb_bundle(self, nb_bundle: NbBundleIn):
             # TODO check for output exceptions?
         # TODO assets
 
-    def _prepare_nb_for_cache(self, nb: nbf.NotebookNode, deepcopy=False):
-        """Prepare in-place, we remove non-code cells."""
-        if deepcopy:
-            nb = copy.deepcopy(nb)
-        nb.cells = [cell for cell in nb.cells if cell.cell_type == "code"]
-        return nb
-
     def cache_notebook_bundle(
         self,
         bundle: NbBundleIn,
@@ -201,7 +203,9 @@ def cache_notebook_bundle(
 
         if check_validity:
             self._validate_nb_bundle(bundle)
-        hashkey = self._hash_notebook(bundle.nb)
+
+        hashed_nb, hashkey = self.create_hashed_notebook(bundle.nb)
+
         path = self._get_notebook_path_cache(hashkey)
         if path.exists():
             if not overwrite:
@@ -221,8 +225,7 @@ def cache_notebook_bundle(
             description=description,
         )
         path.parent.mkdir(parents=True)
-        self._prepare_nb_for_cache(bundle.nb)
-        path.write_text(nbf.writes(bundle.nb, NB_VERSION), encoding="utf8")
+        path.write_text(nbf.writes(hashed_nb, nbf.NO_CONVERT), encoding="utf8")
 
         # write artifacts
         artifact_folder = self._get_artifact_path_cache(hashkey)
@@ -260,7 +263,7 @@ def cache_notebook_file(
         :param overwrite: Allow overwrite of cached notebooks with matching hash
         :return: The primary key of the cache record
         """
-        notebook = nbf.read(str(path), NB_VERSION)
+        notebook = nbf.read(str(path), nbf.NO_CONVERT)
         return self.cache_notebook_bundle(
             NbBundleIn(
                 notebook,
@@ -289,7 +292,7 @@ def get_cache_bundle(self, pk: int) -> NbBundleOut:
             )
 
         return NbBundleOut(
-            nbf.reads(path.read_text(encoding="utf8"), NB_VERSION),
+            nbf.reads(path.read_text(encoding="utf8"), nbf.NO_CONVERT),
             record=record,
             artifacts=NbArtifacts(
                 [p for p in artifact_folder.glob("**/*") if p.is_file()],
@@ -325,15 +328,15 @@ def match_cache_notebook(self, nb: nbf.NotebookNode) -> NbCacheRecord:
 
         :raises KeyError: if no match is found
         """
-        hashkey = self._hash_notebook(nb)
+        _, hashkey = self.create_hashed_notebook(nb)
         cache_record = NbCacheRecord.record_from_hashkey(hashkey, self.db)
         return cache_record
 
     def merge_match_into_notebook(
         self,
         nb: nbf.NotebookNode,
-        nb_meta=("kernelspec", "language_info", "widgets"),
-        cell_meta=None,
+        nb_meta: Optional[Iterable[str]] = ("kernelspec", "language_info", "widgets"),
+        cell_meta: Optional[Iterable[str]] = None,
     ) -> Tuple[int, nbf.NotebookNode]:
         """Match to an executed notebook and return a merged version
 
@@ -342,10 +345,11 @@ def merge_match_into_notebook(
         :param cell_meta: cell metadata keys to merge from cached notebook (all if None)
         :raises KeyError: if no match is found
         :return: pk, input notebook with cached code cells and metadata merged.
+
         """
         pk = self.match_cache_notebook(nb).pk
         cache_nb = self.get_cache_bundle(pk).nb
-        nb = copy.deepcopy(nb)
+        nb = nbf.convert(copy.deepcopy(nb), NB_VERSION)
         if nb_meta is None:
             nb.metadata = cache_nb.metadata
         else:
@@ -355,13 +359,18 @@ def merge_match_into_notebook(
         for idx in range(len(nb.cells)):
             if nb.cells[idx].cell_type == "code":
                 cache_cell = cache_nb.cells.pop(0)
+                in_cell = nb.cells[idx]
                 if cell_meta is not None:
                     # update the input metadata with select cached notebook metadata
                     # then add the input metadata to the cached cell
-                    nb.cells[idx].metadata.update(
+                    in_cell.metadata.update(
                         {k: v for k, v in cache_cell.metadata.items() if k in cell_meta}
                     )
-                    cache_cell.metadata = nb.cells[idx].metadata
+                    cache_cell.metadata = in_cell.metadata
+                if nb.nbformat_minor >= 5:
+                    cache_cell.id = in_cell.id
+                else:
+                    cache_cell.pop("id", None)
                 nb.cells[idx] = cache_cell
         return pk, nb
 
@@ -376,7 +385,8 @@ def diff_nbnode_with_cache(
         from nbdime.prettyprint import pretty_print_diff, PrettyPrintConfig
 
         cached_nb = self.get_cache_bundle(pk).nb
-        nb = self._prepare_nb_for_cache(nb, deepcopy=True)
+        nb, _ = self.create_hashed_notebook(nb)
+
         diff = nbdime.diff_notebooks(cached_nb, nb)
         if not as_str:
             return diff
@@ -430,7 +440,7 @@ def get_staged_notebook(
                 "The URI of the staged record no longer exists: {}".format(uri_or_pk)
             )
         if converter is None:
-            notebook = nbf.read(uri_or_pk, NB_VERSION)
+            notebook = nbf.read(uri_or_pk, nbf.NO_CONVERT)
         else:
             notebook = converter(uri_or_pk)
         return NbBundleIn(notebook, uri_or_pk)
@@ -443,7 +453,7 @@ def get_cache_record_of_staged(
         else:
             record = NbStageRecord.record_from_uri(uri_or_pk, self.db)
         nb = self.get_staged_notebook(record.uri, converter=converter).nb
-        hashkey = self._hash_notebook(nb)
+        _, hashkey = self.create_hashed_notebook(nb)
         try:
             return NbCacheRecord.record_from_hashkey(hashkey, self.db)
         except KeyError:
@@ -456,7 +466,7 @@ def list_staged_unexecuted(
         records = []
         for record in self.list_staged_records():
             nb = self.get_staged_notebook(record.uri, converter).nb
-            hashkey = self._hash_notebook(nb)
+            _, hashkey = self.create_hashed_notebook(nb)
             try:
                 NbCacheRecord.record_from_hashkey(hashkey, self.db)
             except KeyError:

jupyter_cache/cache/main.py

@@ -55,6 +55,7 @@
             "numpy",
             "sympy",
             "pandas",
+            "nbformat>=5.1",  # to include v4.5 notebook
         ],
         "rtd": ["myst-nb~=0.7", "sphinx-copybutton", "pydata-sphinx-theme"],
     },