Compatibility with ServerApp backend

[pdstack]

edit I have realised this is actually a ServerApp backend compatibility issue. See later post in this thread for update.

Appmode doesn't work with newer versions of jupyter_client, this pull request attempts to resolve these issuses. A temp_dir option is also added to allow for custom app mode directories, within the current jupyter path.

There are a number of changes in this pull request that may require discussion or further work, happy to amend if there are better ways of doing this.

Compatibility changes:

• Newer versions of the content manager do not allow for hidden files unless an option is set, which then shows all hidden files. To resolve this the temporary notebooks are no longer hidden.
• Content manager and session manager are now async, so check for Awaitables and await them if present. Removed older style tornado.gen and replaced with native async functions. This means it is no longer Python 2 compatible. Could rewrite this to maintain Python 2 compatibility. This still works with older versions of jupyter_client.
• Launching notebooks using command line jupyter notebook is now causing tornado exceptions when interacting with widgets. This may be fixed in future, but it is preferable to work with the jupyter lab command anyway, which doesn't have this problem. The jupyter lab command results in the notebook.html and page.html templates not being served and available for appmode to use, to get around this I have copied the latest version of these templates from nbclassic into the repo, with very small changes. This is an ugly way of fixing this problem, if anyone has a better idea on how to do this please let me know.

[oschuett]

Thank you so much for this pull request!

The server_extension.py looks good.

I'm just a bit worried about forking the templates. Perhaps there is a better way. What do you think @unkcpz?

[unkcpz]

Thanks @pdstack for the contribution! I have some questions and need some more details.

Appmode doesn't work with newer versions of jupyter_client

Which jupyter_client version you are using? We use jupyter_client==7.3.5 in our AiiDAlab docker stack and the notebook open in Appmode works all fine.

Can you describe a bit of your jupyter environment? Which backend you are using, jupyterLab or the notebook6, or nbclassic?

The jupyter lab command results in the notebook.html and page.html templates not being served and available for appmode to use, to get around this I have copied the latest version of these templates from nbclassic into the repo, with very small changes. This is an ugly way of fixing this problem, if anyone has a better idea on how to do this please let me know.

We didn't go this way yet since we think it is more straightforward to test on notebook7 when it is ready. It is under development as described in JEP https://jupyter.org/enhancement-proposals/79-notebook-v7/notebook-v7.html. It seems the nbclassic is for the interim period before notebook-v7 is released.
If you need this in your work, it is for sure worth to have a close look and merging to support for nbclassic. And I am happy to give it a test on our AiiDAlab environment.
For coping the template, I am worried about it as well. Is that possible that we more aggressively support the developing version of notebook-v7 where they have the latest alpha version 7.0.0a13? https://github.com/jupyter/notebook/tree/v7.0.0a13

[pdstack]

Thanks for having a look at this pull request and your comments!

I have been testing with the latest version of jupyter_client==8.0.3 which produces the issues I've listed above. I believe version 8.0.0 is where the swap over from notebook 6 to nbclassic occurs. I have been trying to regularly rebuild my python environments and keep up with the latest versions of packages wherever possible. From my testing the current version of appmode works just fine if you pin:

jupyter_client<8.0.0
notebook<=6.4.11

edit I'm not actually sure how to tell which backend I'm running. I've been typically installing jupyter + jupyterlab, then launching the server using jupyter notebook. This allows for both use of /lab for lab mode and for appmode apps to work since the templates are available. My understanding is that won't work in future, and the server will need to be launched using jupyter lab if you want to use lab mode, which doesn't serve out the html templates in the same way. jupyter_client==0.8.3 also has some errors with tornado if launched using jupyter notebook instead of jupyter lab.

If the plan was to wait until notebook-v7 lands then I am happy to wait until then. As you say it probably makes more sense to try and support notebook-v7 development, rather than forking the notebook html templates, I'm just not sure how much work that would involve.

This fork serves the purpose I need for now, including this version in builds will resolve the current compatibility issues as a temporary fix until notebook v7 is released. Did you want me to close the pull request? Or are there any parts you think it would make sense to merge in now, like the server_extension.py changes? I could move the notebook templates to a branch to allow for a merge without them.

[unkcpz]

Thanks for the detail, can you try to run following code in a notebook you start? (FYI, find out if my code runs inside a notebook or jupyter lab)

import psutil
[i for i in psutil.Process().parent().cmdline()]

In our AiiDAlab deployment, we need to set JUPYTERHUB_SINGLEUSER_APP: "notebook.notebookapp.NotebookApp" in the k8s config file to force using the NotebookApp backend.
I think in the terminal, jupyter notebook command will start with the notebook mode (Check jupyter/docker-stacks#1575 for discussion, I think it is relevant), maybe it uses the nbclassic by default. I'll check in my environment.

This fork serves the purpose I need for now, including this version in builds will resolve the current compatibility issues as a temporary fix until notebook v7 is released. Did you want me to close the pull request? Or are there any parts you think it would make sense to merge in now, like the server_extension.py changes? I could move the notebook templates to a branch to allow for a merge without them.

If you can pin the jupyter-client version, I advise to do it for now so you can use the App mode without isuses. I'll come back with the test if it possible to using the jupyter-client v8 with using the notebook backend and running appmode without issue.
As for the asyncio/tornado issue, I think your change is anyway valuable. Could you separate it and make a new PR?

[unkcpz]

I did a test which try to use minimal change to run the appmode in jupyter-client. You can find my change at #66, simply as you mentioned, using the flag to allow create the hidden file.
If you build the container and run the example, it is all fine. Did I miss something?

[pdstack]

Thanks for your comment about the backend, while the psutil command didn't really tell me anything, the other link was quite informative. The terminal window running the jupyter command makes it easy to tell which backend is being used, the logging messages are prefixed with NotebookApp or ServerApp.

As a result I realised what I am actually trying to acheive here is getting appmode to work with the ServerApp backend. While jupyter_server<2.0.0 can run the /lab mode using the NotebookApp backend, newer jupyter_server>=2.0.0 versions require the ServerApp backend to do so.

The issues with jupyter_client>8.0.0 weren't really related to appmode specifically, but rather the NotebookApp backend, and interactivity with some of the apps I have been working on when using jupyter_client>8.0.0. Using ServerApp resolves these problems. See https://discourse.jupyter.org/t/jupyter-notebook-zmq-message-arrived-on-closed-channel-error/17869 for the error I was encountering. There is probably something else going on here, but I would prefer to use the ServerApp backend anyway.

I had copied the notebook html templates because they are not available in the ServerApp templates, I managed to get them working by using --ServerApp.extra_template_paths='***site-packages***\notebook\templates', but this is obviously not an ideal solution. What I have done in the end is add them to the templates path in the load_jupyter_server_extension function, similar to how the current appmode templates are added. This is a more elegant solution than what I had done previously.

I have also added another configuration option: hidden_temp_files, which adds/removes the . at the start of the file name. I don't want to enable ContentsManager.allow_hidden=True in my environment, there a number of other hidden folders that are related to the container setup and python virtual environment which would clutter up the jupyter lab interface if shown. I have updated the readme to let people know they will have to either enable ContentsManager.allow_hidden=True or Appmode.hidden_temp_files=False for appmode to work with newer notebook versions. It would make more sense for me that Appmode.hidden_temp_files defaults to True, so no additional configuration is required for appmode to work, but I'm not sure if you have specific reasons to prefer using ContentsManager.allow_hidden=True ala #66, so I have left the default as False to maintain current behaviour.

I think this pull request is in a good spot now, and enabling ServerApp backend compatibility should make Appmode more usable generally going forward.

In terms of the other pull request #66, I think it may eventually run into the same tornado/zmq issues as I was having before with jupyter_client>8.0.0. I don't have a simple example to replicate the bug, it wasn't occuring in simple test apps, only the more complicated one with many dependencies, so it is likely some interaction with one of the widgets I have displayed. It wasn't causing the apps to break completely, just the interaction kept being interupted by the exceptions raised in the backend, and it would take a second or two to catch up every few seconds.

[unkcpz]

@pdstack thanks a lot for further investigation. I think the changes look all good to me. I just have a minor request on the duplicate pattern for all await clause changes. Let me know if it is possible to simplify in context manager. If it is to complex I am okay with the current change. I'll make a test after.

[unkcpz]

I have also added another configuration option: hidden_temp_files, which adds/removes the . at the start of the file name. I don't want to enable ContentsManager.allow_hidden=True in my environment, there a number of other hidden folders that are related to the container setup and python virtual environment which would clutter up the jupyter lab interface if shown. I have updated the readme to let people know they will have to either enable ContentsManager.allow_hidden=True or Appmode.hidden_temp_files=False for appmode to work with newer notebook versions.

I am okay with this change, for the AiiDAlab, it is always easy to make changes on deployment options, so I don't have a preference.

@oschuett can you double-check this PR? I think it is simplified a lot and okay to have a go. BTW, I don't have permission on this repository, please let me know if you need me to approve it or if it is possible to give me written permission to merge.

[unkcpz]

Thanks a lot for having a generic function for the await pattern @pdstack
Just one minor request and then I think it is good to go.

[unkcpz]

The name is a bit confusing with the asyncio.ensure_future. I would suggest call the function def await_if_awaitable(obj).

[pdstack]

I've renamed it to await_if_awaitable. The name ensure_async was what jupyter_core used for the function, but I don't have any strong opinions on it.

[unkcpz]

Okay, then I think they are all fine, since you already changed them, I won't ask you to change them back 😸 . Thanks!

[pdstack]

No worries, easy enough to change back if you think the other name was better, or you think of another one. Thanks for the quick reviews!

[unkcpz]

I test locally and it works neatly. 👍

[unkcpz]

Okay, merged. Thanks again @pdstack

I plan to clean up the release flow and make a new release. Although it is backward compatible, maybe still good to make a minor version release to 0.9.0? What do you think @pdstack @oschuett @yakutovicha?

[pdstack]

I think a minor release would make sense, there are 2 configuration flags added, as well as python 2 support being dropped.

[yakutovicha]

Okay, merged. Thanks again @pdstack

I plan to clean up the release flow and make a new release. Although it is backward compatible, maybe still good to make a minor version release to 0.9.0? What do you think @pdstack @oschuett @yakutovicha?

@unkcpz makes sense to me.