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

Format for kernel type ID? #5

Closed
takluyver opened this issue Oct 26, 2018 · 6 comments
Closed

Format for kernel type ID? #5

takluyver opened this issue Oct 26, 2018 · 6 comments

Comments

@takluyver
Copy link
Owner

Kernel type identifiers are currently slash delimited, provider/id, e.g. spec/python3. Everything after the first slash is passed to the provider to look up; I had thought this could support delegation, such as a kernel provider which can talk to different providers on a remote host, like ssh/mydesktop/spec/python3.

However, in working on supporting kernel type identifiers in the notebook server, it's come up that slashes are not very convenient in either URLs or HTML ids. The problems are not insurmountable, but if we want to change or restrict this interface, it's much easier to do while it's relatively new.

Options:

  1. Switch to another delimiter. Dots . or colons : are obvious candidates, but are still awkward to use in HTML IDs because they have meaning in CSS selectors. Hyphens - and underscores _ are technically easier, but they don't feel like delimiters, and people may want to use them in the delimited names.
  2. Restrict the characters allowed between delimiters, so there's an obvious way to escape a kernel type identifier (e.g. if underscores are disallowed, spec/python3 can be escaped as spec_python3). There are plenty of unicode possibilities if we don't mind making the escaped versions hard to type manually.
  3. Limit identifiers to one delimiter - this would allow URL routing to distinguish the kernel ID from any following URL components.
  4. Leave it as is, and work around the issues some other way (e.g. for URLs, percent-encode slashes as %2F).

I think I lean towards 4 or 2.

cc @minrk @Carreau for input.

For reference, HTML 5 IDs can contain any characters except space. The practical restrictions for IDs come more from CSS selectors and jQuery - jQuery needs escapes for :.[],=@. The reserved characters for URIs from RFC 3986 are:

":" / "/" / "?" / "#" / "[" / "]" / "@"
"!" / "$" / "&" / "'" / "(" / ")"
"*" / "+" / "," / ";" / "="
@minrk
Copy link
Contributor

minrk commented Oct 26, 2018

I agree that either 2 or 4 is best. Most logical delimiters (:, /) do need escaping in a lot of contexts, though, so I'd probably go with 4, since going with 2 would likely need escaping anyway unless we picked an unnatural delimiter like _, as you said.

@takluyver
Copy link
Owner Author

For my WIP notebook branch, I've so far gone with 4, and it hasn't been as painful as I feared. I've made two changes to work around this:

  • The entries in the new notebook menu are selected by a data-kernel-type attribute rather than their ID.
  • The URLs for accessing kernelspec resources (icons etc.) are escaped using urllib to convert / into %2F.

@kevin-bates
Copy link
Collaborator

@takluyver and @minrk, I've been working on including jupyter_kernel_mgmt into jupyter_server (jupyter-server/jupyter_server#112) and have found issues with the use of '/' as the separator.

In Thomas' original Notebook WIP (referenced in previous comment) changes were required in the "front-end" code to accommodate kernel type IDs. However, because jupyter_server has no "front-end" code, I think it would be good, if possible, to not require front-end extensions to change.

The issues have been posted into the jupyter_server WIP PR and are two-fold. 1) although the kernel starts and runs fine, the actual kernel name is not displayed - only 'kernel' appears. 2) the resource files are not pulled so things like icons, etc. do not display.

I suspect this is due to using '/' and the idea that there may be code somewhere that is side-affecting its meaning - probably an extra encoding or something - I don't know. However, I thought I'd experiment with changing the separator to ':' instead, for one because it's less common, but I also believe it to be less ambiguous from a separator context. Once the 4 or 5 locations were fixed up (I introduced a PROVIDER_SEPARATOR variable), I found both the kernel display name and icon resources both displayed immediately - no changes were necessary on the "front-end" nor were additional changes required in the server aside from the actual change.

Granted, there are two python 3 entries on the kernel drop down list, but I figure that kind of change is acceptable (and, technically speaking, optional).

I'm not familiar with the areas in the "front-end" code like you are, but it seems like having something that works "as expected" w/o changes is important, especially in the context of jupyter_server. I don't know anything about CSS or jQuery selectors, but assuming that's an issue, might there be another character we could use (I don't think '_' or '-' are good choices either). How often would fully qualified kernel names be used in CSS/jQuery contexts (sorry for my naïveté on that)?

What are your thoughts?

@takluyver
Copy link
Owner Author

My thoughts boil down to: let's just change the frontend code. I already did it in jupyter/notebook#4170, it didn't require major changes, and if jupyter_server uses jkm, then the adoption of jupyter_server for notebook is an obvious point to apply that change.

Adopting jkm is going to cause some disruption - there's no way around that. Slash-delimited kernel type IDs are now present in examples in quite a few places, so I think it will be more confusing to switch to colons now than continue with slashes.

Looking back at the original issue here, colons don't avoid the fundamental issues that I was thinking about - they still need to be escaped in URLs, and they are meaningful in CSS selectors, so frontend code will still need to escape them.

@kevin-bates
Copy link
Collaborator

ok. Learning that the Lab server extension didn't require any changes when '/' is used eased my concerns considerably. I'll note this as an incompatibility in the PR and that when Notebook extension is finalized, this is something that will be needed. Thanks.

@kevin-bates
Copy link
Collaborator

Since it sounds like we're sticking with '/' as the separator, I'm going to close this issue.

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

3 participants