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

Change id generation to be hash based to avoid problematic word combinations #217

Merged
merged 2 commits into from
Apr 2, 2021

Conversation

MSeal
Copy link
Contributor

@MSeal MSeal commented Apr 1, 2021

Implements Option C from the original JEP to resolve #216 where it was raised that our word generation scheme results in highly problematic word combinations. Until we have a wordlist we feel is safe to use in generating IDs, this will be the source of unset ids detected by nbformat.

I'm going to cross post to jupyter/enhancement-proposals#62 and leave this open for ~24 hours so there's a chance for any additional input before we release. I recognize this pattern choice was not chosen in the original JEP and therefore is subverting the decision therein somewhat without all of the original reviewer's inputs, but the severity of the issue warrants action and there's enough consensus on the issue thread from active folks in the community that this seems a warranted step to avoid the potential insensitive to the nature of the generated ids.

Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@willingc willingc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @MSeal

@ellisonbg
Copy link
Contributor

We talked about this yesterday in the Jupyter Notebook meeting and our thinking aligned with this PR for the reasons stated. @jasongrout @blink1073 @afshin

@@ -7,6 +7,11 @@ Changes in nbformat
In Development
==============

5.1.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5.1.2 is already released. The next one would be 5.1.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I'll fix that later this afternoon.

Copy link
Member

@jasongrout jasongrout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, except for one comment about the changelog version number. Thanks for fixing this quickly!

@choldgraf
Copy link
Contributor

I opened up #218 to track adding back in human-readable IDs if we wish. Thanks @blois for taking extra caution to inspect the word lists and identify that these were still problematic 🙏

@MSeal MSeal merged commit 75f4f44 into jupyter:master Apr 2, 2021
@MSeal
Copy link
Contributor Author

MSeal commented Apr 2, 2021

Thanks for the quick reviews everyone. I've pushed the release tag for this and 5.1.3 should finish building shortly.

@MSeal
Copy link
Contributor Author

MSeal commented Apr 2, 2021

The NPM Publish failed because the token was rejected. I refreshed the token and manually pushed the npmjs version (though it has no actual JS changes). I think it's incorrectly expecting an OTP token despite the auth key being an automation key that's not support to ask for it.... will keep an eye out on it next time there's a tag publish to see if it fails again.

@ellisonbg
Copy link
Contributor

From what I can see, this fix was released in version 5.1.3 of nbformat. Does it make sense to "yank" (this is the technical term pypi uses for this) the 5.1.2 version to prevent people from accidentally installing the older version?

@MSeal
Copy link
Contributor Author

MSeal commented Apr 15, 2021

@ellisonbg I think it makes sense, yes. Done, however I wasn't sure how to approach the conda-forge side of things. I posted conda-forge/admin-requests#257 to see if there was a recommended approach there.

@ellisonbg
Copy link
Contributor

@MSeal thanks, not sure how these things work with conda myself.

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

Successfully merging this pull request may close these issues.

Where did the adjective/noun corpus come from?
8 participants