Initialize icon_path when entry disabled

[guimou]

Hi! I am using the jupyter-lmod extension to dynamically load modules inside a Jupyter container. The module is able to show/hide icons depending on the availability of the module/executable. For example, this prevents people to try launching RStudio, because they see the icon, if it's not loaded. In this mechanism, the launcher entry has to be set to "disabled" at the beginning (as the module is not loaded). The module takes care of dynamically adding the launcher entry. However, with the current code, the icon path is not initialized when the module is not enabled from the beginning. And as it's impossible to add more handlers on the same path afterwards (unless I'm mistaken but I tried it multiple ways), the icon can never be displayed. Proposed modification is to always initialize the icon_path when it's present, even if the entry is disabled. It does not cost much in the end, even if this icon/path is never used. And if the launcher entry is there, even if disabled, it must be for a good reason, so why not initialize the icon_path?

[welcome[bot]]

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly. You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

[yuvipanda]

Thank you for opening this PR, @guimou. Will this still display the entries in the JupyterLab launcher, even if they are disabled?

[guimou]

In a standard configuration, yes. If the entries are disabled, the icons don't show (so standard behaviour). But with the jupyter-lmod extension, there is this code that sets it to enabled when you load the corresponding module (and re-disable when you unload the module). So workflow is: RStudio (or other kernel/app) module not loaded, icon is not visible -> Load the module with the extension, icon becomes visible automatically, you can launch RStudio -> Unload RStudio module (for whatever reason), icon is not visible again. I made the PR because if the icon_path is not initialized from the start in the handlers it does not work, I cannot add it later (or at least did not find a way). Only drawback is that you initialize all the icons path even if the entries are disabled. They don't appear in the Launcher, it's just that the path to get the image works (instead of a 404). You can see it here in action if my explanations don't make sense :smiley: https://youtu.be/NrUEBxjMtiU?t=267

[guimou]

@yuvipanda Do you need any other info?

[guimou]

@yuvipanda Hi! I'm bumping this because we are thinking/working on integrating the extension I mentioned into the Elyra project. Before proceeding I have some cleanup to do. And depending on the result of the PR, I will either just use the jupyter-server-proxy package, or proceed as I do currently which is forking/copying the code directly in my extension (which is not really efficient...). So, is anything else needed on my part? Anyway, no bad feelings if you decide not to accept this change, I just need to know. Thanks a lot!

[consideRatio]

I'm cautious about merging this PR as it seems like a breaking change that is a workaround to a situation that can be resolved in non-breaking ways I think.

We have enabled as a toggle to mean something very specific, and this change seems to bypass that. If you want to enable/disable based on some just-in-time information, I think it would be more reasonable to allow for the launcher_entry to be a callable.

Would having it be a callable meet your needs @guimou?

https://github.com/jupyterhub/jupyter-server-proxy/blob/ade470cbdb33563ce3e7669f7e35b99e5f7d23f9/docs/source/server-process.rst#L106-L122

Related

• #223

[consideRatio]

@guimou to help me as a maintainer and other maintainers focus our attention, I'll make a call here and close this PR for now. With that said, I'm personally very open to the idea of allowing launcher_entry to be a callable function and if you wish you can open a PR about it directly I think. Such PR would also need some documentation updates!

If a callable function don't meet your needs I would like to ask that you open an issue to discuss what could meet your needs and how/if that could be reasonable to do.

[guimou]

Hi @consideRatio ! No worries, it was ok closing this. Tbh I did not really think about it any more. For the moment I'm patching the installation in my deployments. As it's very specific that's OK. I will look into the callable function option when I have time. It sounds good, but the constraint we have here, unless I'm mistaken, is that all of this must happen when Tornado launches. After it's initialized and loaded you cannot modify this type of resource (at least that's what I understood). Thanks anyway for everything you and the other maintainers are doing!