Generate deterministic ids when formatting notebooks #9359
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zanieb
commented
Jan 2, 2024
zanieb
commented
Jan 2, 2024
|
zanieb
force-pushed
the
zb/notebook-id-fmt
branch
from
27a8726
to
411f988
Compare
zanieb
force-pushed
the
zb/notebook-id-fmt
branch
from
411f988
to
f4edaa6
Compare
crates/ruff_notebook/src/notebook.rs
Outdated
| if raw_notebook.nbformat == 4 && raw_notebook.nbformat_minor >= 5 { | ||
| // We use a mock random number generator to generate deterministic uuids | ||
| let mut rng = rand::rngs::mock::StepRng::new(0, 1); |
There was a problem hiding this comment.
Could we use a seeded number generator rather than the StepRng one, so that the IDs are deterministic but look random rather than structured as they do now? Or was this the only option for deterministic UUIDs?
There was a problem hiding this comment.
I'm sure there's another option for a seeded number generator, this one was just the most obvious way I saw.
There was a problem hiding this comment.
Sure we can use the StdRng seeded with 0
7fb27b94-1602-401d-9154-2211134fc71a
acae54e3-7e7d-407b-bb7b-55eff062a284
9a63283c-baf0-4dbc-ab1f-6479b197f3a8
8dd0d809-2fe7-4a7c-9628-1538738b07e2
72eea511-9410-473a-a328-ad9291626812
charliermarsh
approved these changes
Jan 3, 2024
konstin
approved these changes
Jan 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When formatting notebooks, we populate the
idfield for cells that do not have one. Previously, we generated a UUID v4 which resulted in non-deterministic formatting. Here, we generate the UUID from a seeded random number generator instead of using true randomness. For example, here are the first five ids it would generate:We also add a check that an id is not present in another cell to prevent accidental introduction of duplicate ids.
The specification is lax, and we could just use incrementing integers e.g.
0,1, ... but I have a minor preference for retaining the UUID format. Some discussion here — I'm happy to go either way though.Discovered via #9293