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

Kernel name is not displayed with Kernel Manager #2

Open
echarles opened this issue Dec 1, 2019 · 9 comments
Open

Kernel name is not displayed with Kernel Manager #2

echarles opened this issue Dec 1, 2019 · 9 comments

Comments

@echarles
Copy link
Member

echarles commented Dec 1, 2019

Running notebook with nbclassic, server and kernel_mgmt has a side effect: The name of the running kernel is no more displayed, instead a generic Kernel label is shown to the user.

This PR is opened to discuss if we need to solve this (I believe this should not be a blocker to release), when and how.

The link of the initial patch made by @takluyver is taken here: https://patch-diff.githubusercontent.com/raw/jupyter/notebook/pull/4170.diff

@kevin-bates has taken over the server part of it and has done an amazing job enriching it a lot (those are spread in various repos). The diff of /notebook/static/notebook/js have not been taken over. We could reapply the left diff to the notebook repo, but I guess this would break things if the sever is not equipped with jkm.

cc/ @Zsailer

@kevin-bates
Copy link
Member

Thanks for opening this issue @echarles. We definitely need the changes, but I agree it is not a release blocker. Are you able to enumerate what the changes actually are? (This is one of the remaining items in jupyter-server/jupyter_server#112 and it would be great to reference that enumeration.)

Having not looked at the changes and given that JupyterLab "just works" (although the provider is not displayed) I would hope the NB extension could work similarly such that it tolerates non-prefixed kernel names in the original form as well as those returned from provider-based frameworks.

@echarles
Copy link
Member Author

echarles commented Dec 1, 2019

Hi Kevin, I have applied the static diffs and kernel name is correctly displayed in both [1] notebook with nbclassic+jupyter_server+kernel_mgmt AND [2] also with plain normal notebook (for this last one, I had to manually apply the diff on one file which had evolved).

It is just now a matter to decide if we want to open a PR in notebook repo and maybe also take a bit more time to decide how the label should be (I have read a few discussion, eg but not only in takluyver/jupyter_kernel_mgmt#6).

So definitively not a blocker.

[1] notebook with nbclassic+jupyter_server+kernel_mgmt with patch

Screenshot 2019-12-01 at 10 55 35

[2] plain normal notebook with patch

Screenshot 2019-12-01 at 11 05 01

@echarles
Copy link
Member Author

echarles commented Dec 1, 2019

Are you able to enumerate what the changes actually are? (This is one of the remaining items in jupyter-server/jupyter_server#112 and it would be great to reference that enumeration.)

Well, the enumeration would be a few simple javascript changes in the following files:

  • notebook/static/notebook/js/kernelselector.js
  • notebook/static/services/sessions/session.js
  • notebook/static/tree/js/newnotebook.js

@kevin-bates
Copy link
Member

Beautiful! Yeah, I feel like using both ks.spec.display_name and the parenthesized ks.name is a bit redundant since ks.spec.display_name tends to be a function of ks.name. In addition, display real estate is at a premium.

I'd rather see the parenthesized value be only the provider id prefix - which implies we'd need some parsing logic or add provider_id to the kernelspec return value. However, we cannot remove the provider_id from ks.name since we need the provider_id for kernel launches. (Hmm, we could also set provider_id: alongside kernel_name: in the json body for /api/kernels.)

If we could make provider_id "a thing" in the front-end, then we get better implementation separation so front ends don't need to assume '/' is a provider_id kernel name separator. The front end would then add provider_id to the start kernel request only if the kernelspec contains that field.

@takluyver
Copy link
Member

My thinking is that frontends should include the full kernel type ID, e.g. spec/python3 or conda/env-foo, including both the provider ID and the kernel type name within that provider. From experience with kernel specs, it's easy to end up with multiple kernels from one provider having the same display name, and the current UI makes them impossible to distinguish.

I think it's up to the frontend what additional information it wants to display alongside that - display name, language name, mimetype... - but I would strongly recommend that as a minimum it's always easy to see the full kernel type ID when listing kernels.

@echarles
Copy link
Member Author

echarles commented Dec 1, 2019

PS: It is all over spread in various repos and prolly the nbclassic is not impacted at all by this discussion. In another way, it may be a good place to discuss this as independent of the various components that will come into picture. After all, this is aimed to be a shim for the other components...

To further take decision, I would rather see converging with the jlab+server+kmgmt combo stack.
We can make the thing with structure/vocabulary that would be share across all layers. Once done, we can revert back to this notebook patch as we are now fully confident we can control the notebook display.

I need to reload the kmgmt data structures in my mind and to be honest still a bit confused with the kernel specs.

Could you list a few examples from which we could deduce a structure which could look like:

kernel_type_id:
  provider_id: The developer/organisation that provides the kernel - Reserved for internal usage: spec
  kernel_name: The name of the kernel. This can be duplicate across multiple provider ids.
  separator: The separator used when sending the spec_name as a single string between architecture components.

@kevin-bates
Copy link
Member

@takluyver - I just realized that conda kernels have the ambiguity issue. Same display names, different kernel names (since they include the conda env), but same provider id (conda). I retract my idea of separating out provider_id from ks.name. In addition, the addition of the parenthesized value could be viewed as an enhancement (irrespective of JKM) since it resolves ambiguity.

@echarles - if we're going to always include (<ks.name>), then documenting the separator (which should always be '/' at this point) and provider-id are not as necessary and could be treated in an opaque manner.

I agree that "kernel specs" are a bit overloaded with JKM. There's the kernel spec that is returned by all providers vs. the kernel spec that is stored in the kernel.json file and used by kernelspec providers.

@takluyver
Copy link
Member

I suggest:

  • A kernel spec is a directory with a kernel.json file.
  • A kernel type is anything discovered through the JKM machinery (which includes kernel specs, exposed with the spec/ prefix). This has:
    • A kernel type ID (the qualified string, e.g. spec/python3)
    • kernel type metadata (the dictionary from find_kernels with more details about that kernel type)
  • kernel instances are specific processes created by launching a kernel type

Of course, for compatibility reasons, some interfaces named after kernel specs (e.g. HTTP endpoints) may end up dealing with kernel types as we adapt them. Some confusion like that is unavoidable.

@echarles
Copy link
Member Author

echarles commented Dec 1, 2019

I agree that "kernel specs" are a bit overloaded with JKM. There's the kernel spec that is returned by all providers vs. the kernel spec that is stored in the kernel.json file and used by kernelspec providers.

Hence maybe my confusion sometimes... If there are different things, it should have different names. I just see Thomas last comment here above. If we name in another way in the documentation, is there impact in the source code to make it more clear?

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