chrisjsewell / pytest-notebook

A pytest plugin for regression testing and regenerating Jupyter Notebooks
https://pytest-notebook.readthedocs.io
BSD 3-Clause "New" or "Revised" License
47 stars 9 forks source link

Incompatible with nbdime version 4.0.0+ #59

Closed renonat closed 7 months ago

renonat commented 7 months ago

Issue

pytest-notebook is incompatible with nbdime>=4.0.0 as nbdime.diffing.generic.diff dropped the predicates keyword argument.

Notes

Stack Trace

   def diff_notebooks(
        initial: NotebookNode, final: NotebookNode, initial_path: str = ""
    ) -> List[DiffEntry]:
        """Compare two notebooks.

        This is a simplified version of ``nbdime.diff_notebooks()``, where we replace
        ``nbdime.diff_sequence_multilevel()`` with ``diff_sequence_simple()``
        to diff the cell and output lists.
        ``diff_sequence_multilevel`` use 'snakes' computation, to guess where cells have
        been inserted/removed. However, this can lead to longer diffs, where cells with
        changed outputs are assigned as removed/inserted, rather than simply modified.
        Moreover, since we are comparing the same notebook before/after execution,
        we shouldn't need to worry about insertions.

        """
>       return diff(
            initial,
            final,
            path=initial_path,
            predicates=defaultdict2(lambda: [operator.__eq__], {}),
            differs=defaultdict2(
                lambda: diff,
                {
                    "/cells": diff_sequence_simple,
                    "/cells/*": diff,
                    "/cells/*/outputs": diff_sequence_simple,
                    "/cells/*/outputs/*": diff_single_outputs,
                    "/cells/*/attachments": diff_attachments,
                },
            ),
        )
E       TypeError: diff() got an unexpected keyword argument 'predicates'
/usr/local/lib/python3.11/site-packages/pytest_notebook/diffing.py:78: TypeError
cpascual commented 7 months ago

I just had the same issue breaking my CI.

As a workaround I pinned nbdime==3.2.1 in my project until this is solved in pytest-notebooks

chrisjsewell commented 7 months ago

Thanks for reporting. I probably won't have time to look at this straight away, so any PRs or suggestions on how to fix would be appreciated!

cpascual commented 7 months ago

If you want, I can make a quick PR just limiting the nbdime depend to <4 as a workaround to prevent other people being hit by this until a proper fix is done

cpascual commented 7 months ago

I just created the PR #60 . Its workflow is pending approval for running

Edit: I filed the PR against the default branch (master) if you prefer it against develop or something else, just let me know

krassowski commented 7 months ago

suggestions on how to fix would be appreciated!

well, that should be rather straightforward since now instead of predicates one should pass config which includes predicates. See the diff from https://github.com/jupyter/nbdime/pull/639, in particular:

-    predicates = defaultdict(lambda: [operator.__eq__], {
-        '/': [lambda a, b: a['id'] == b['id']],
-    })
+    config = DiffConfig(
+        predicates=defaultdict(lambda: [operator.__eq__], {
+            '/': [lambda a, b: a['id'] == b['id']],
+        })
+    )

-    ld = diff(b, l, path="", predicates=predicates)
+    ld = diff(b, l, path="", config=config)

Alternatively, nbdime could be patched for backward compatibility re-adding predicates as an optional, deprecated argument (which cannot be passed when config is passed).

amotl commented 7 months ago

Dear Reno, Carlos, and Michał,

thank you very much for your reports and suggestions how to fix this problem. We just submitted GH-62 to make pytest-notebook compatible with nbdime>=4.

With kind regards, Andreas.

amotl commented 7 months ago

Thanks for the quick release, Chris. Cheers!