jupyter / nbclassic

Jupyter Notebook as a Jupyter Server extension
https://nbclassic.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
77 stars 62 forks source link

Kernel name is not displayed with Kernel Manager #2

Open echarles opened 5 years ago

echarles commented 5 years ago

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 commented 5 years ago

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 https://github.com/jupyter/jupyter_server/pull/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 commented 5 years ago

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 https://github.com/takluyver/jupyter_kernel_mgmt/issues/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 commented 5 years ago

Are you able to enumerate what the changes actually are? (This is one of the remaining items in jupyter/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:

kevin-bates commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

I suggest:

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 commented 5 years ago

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?