nbdime icon indicating copy to clipboard operation
nbdime copied to clipboard

Support cell IDs

Open vidartf opened this issue 4 years ago • 9 comments

See recent JEP and implementation:

  • JEP:https://github.com/jupyter/enhancement-proposals/blob/master/62-cell-id/cell-id.md
  • nbformat implementation: https://github.com/jupyter/nbformat/pull/189/files?file-filters%5B%5D=.py&file-filters%5B%5D=.rst

For nbdime, we then need to consider whether we should use IDs as a high level indicator of cell identity when diffing. Questions that need to be resolved are:

  • Should IDs take precedence over content? Just using IDs would make some things a lot simpler, but as the content is the only attribute that is exposed to the user, content equality should probably still be the strongest measure?
  • If IDs are not the highest precedence, should we display changed IDs? If so, how (terminal seems straight forward, web less so).
  • If IDs are not the highest precedence, how do we resolve potential ID conflicts when merging? Some applications are likely to want to associate content with cells via their IDs, so if they conflict, we are unlikely to be able to auto-resolve. We would also need to figure out how to display such conflicts.

vidartf avatar Dec 03 '20 11:12 vidartf

The fresh release seems to have broken a lot of tests (cf. #551). This should be fixed, or at the very least be verified to be a test-only issue. If this affects the current releases, a patch release based on the last release should be cut.

vidartf avatar Jan 14 '21 19:01 vidartf

My guess is that it's a test-only issue and that the added field doesn't factor into user-facing output other than possibly seeing the cell ids.

I'd suggest: step 1, ignore cell id and make a release with unchanged behavior that is aware of cell ids, as if it were any other opaque metadata field. Since cell IDs are hidden from the user, I think showing them in diffs should also be done at the changed-metadata level, maybe hidden by default.

Then I think we can explore using cell ids as highest priority for alignment. Since that would potentially simplify things, I think it's a good thing to try, and eliminates cell id conflict issues (right?). Obviously, a contrived example can be created to generate an equivalent-content notebook with the same ids in different order where a big diff would be a bad experience, but I don't think that's likely to happen, at least not often. Then I'd say only downgrade cell id precedence again if this proves to be a poor experience in practice, and only at that point are cell id conflicts needed to be resolved, where I think a typical "use theirs/use ours" strategy followed by an assert-uniqueness pass on the whole cell list to regenerate later occurrences of any duplicate cell ids should suffice.

minrk avatar Jan 15 '21 07:01 minrk

I agree with the plan 👍

Here are some cases that should be considered as part of the exploration for using cell ids as the highest priority:

  • A user duplicating a cell, and then deleting the "original" (the one with the original cell id) and keeping the copy.
  • A cell having been split both locally and remotely (in an identical manner). This would then be two inserts that are identical except for the IDs.

vidartf avatar Jan 15 '21 14:01 vidartf

I'd suggest: step 1, ignore cell id and make a release with unchanged behavior that is aware of cell ids, as if it were any other opaque metadata field. Since cell IDs are hidden from the user, I think showing them in diffs should also be done at the changed-metadata level, maybe hidden by default.

What should happen for merge action in the initial phase of the plan?

krassowski avatar Feb 27 '21 15:02 krassowski

Hi, I just wanted to let you know I opened #566 and #574 to facilitate the final 3.0 release. I also see #572 and #576 as handy additions to 3.0. Is there anything else we could do?

krassowski avatar Mar 30 '21 09:03 krassowski

Thanks, I'll do my very best to get it out next week!

vidartf avatar Mar 31 '21 09:03 vidartf

This was only partially resolved by #566, but GH was a little over eager 😄

vidartf avatar Apr 12 '21 00:04 vidartf

Hi @vidartf and @krassowski is there any planned work in this area?

mlucool avatar Sep 21 '21 23:09 mlucool

When time permits. But if you or someone else want to help push this, please do so (or reach out to help coordinate it).

vidartf avatar Sep 23 '21 09:09 vidartf