Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatible with nbdime version 4.0.0+ #59

Closed
renonat opened this issue Nov 21, 2023 · 7 comments
Closed

Incompatible with nbdime version 4.0.0+ #59

renonat opened this issue Nov 21, 2023 · 7 comments

Comments

@renonat
Copy link

renonat commented Nov 21, 2023

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
Copy link

cpascual commented Nov 21, 2023

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
Copy link
Owner

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
Copy link

cpascual commented Nov 21, 2023

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
Copy link

cpascual commented Nov 21, 2023

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
Copy link

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 jupyter/nbdime#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
Copy link
Contributor

amotl commented Nov 28, 2023

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
Copy link
Contributor

amotl commented Nov 28, 2023

Thanks for the quick release, Chris. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants