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

Support cell IDs #553

Closed
vidartf opened this issue Dec 3, 2020 · 9 comments · Fixed by #566 or #639
Closed

Support cell IDs #553

vidartf opened this issue Dec 3, 2020 · 9 comments · Fixed by #566 or #639

Comments

@vidartf
Copy link
Collaborator

vidartf commented Dec 3, 2020

See recent JEP and implementation:

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

vidartf commented Jan 14, 2021

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.

@minrk
Copy link
Member

minrk commented Jan 15, 2021

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.

@vidartf
Copy link
Collaborator Author

vidartf commented Jan 15, 2021

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.

@krassowski
Copy link
Member

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

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?

@vidartf
Copy link
Collaborator Author

vidartf commented Mar 31, 2021

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

@vidartf vidartf reopened this Apr 12, 2021
@vidartf
Copy link
Collaborator Author

vidartf commented Apr 12, 2021

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

@mlucool
Copy link
Contributor

mlucool commented Sep 21, 2021

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

@vidartf
Copy link
Collaborator Author

vidartf commented Sep 23, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants
@minrk @vidartf @mlucool @krassowski and others