Bump traitlets for better performance

[danielhollas]

traitlets>=5.7.0 offer a huge performance improvements when constructing the widgets, and thus significantly improve the initial page load of the app. See ipython/traitlets#777 and https://github.com/ipython/traitlets/blob/main/CHANGELOG.md#570

In my brief testing of importing and initializing my app, this update saves around 600ms.

@unkcpz @yakutovicha would you mind giving this a test for QeApp and / or EMPA apps to verify that things still work?
I went through the changelog between our current version (5.4) and new 5.9 and there doesn't appear to be any breaking changes so we should be good.
Note that when we bump Python version in the docker image in #337, the new traitlets version is already there. But I want to do a little more testing there, so this PR is a hotfix in the meantime which will let us test this out independently.

[danielhollas]

Here's the relevant cProfile output when initializing my app with traitlets 5.4

     ncalls  tottime  percall  cumtime  percall filename:lineno(function)
879/362       0.004    0.000    1.954    0.005 /opt/conda/lib/python3.9/site-packages/ipywidgets/widgets/widget.py:474(__init__)
879/464       0.008    0.000    1.877    0.004 /opt/conda/lib/python3.9/site-packages/ipywidgets/widgets/widget.py:490(open)
1019/258      0.002    0.000    1.511    0.006 {built-in method builtins.__import__}
893/478       0.027    0.000    1.457    0.003 /opt/conda/lib/python3.9/site-packages/ipywidgets/widgets/widget.py:565(get_state)
235792/80223  0.039    0.000    1.457    0.000 /opt/conda/lib/python3.9/site-packages/traitlets/traitlets.py:675(__get__)
38299/19873   0.109    0.000    1.441    0.000 /opt/conda/lib/python3.9/site-packages/traitlets/traitlets.py:643(get)
844420/483899 0.190    0.000    1.366    0.000 {built-in method builtins.getattr}
26160/11054   0.046    0.000    1.239    0.000 /opt/conda/lib/python3.9/site-packages/traitlets/traitlets.py:1740(trait_defaults)
4662/2952     0.006    0.000    1.095    0.000 <frozen importlib._bootstrap>:1033(_handle_fromlist)
23529/9668    0.009    0.000    1.025    0.000 /opt/conda/lib/python3.9/site-packages/traitlets/traitlets.py:605(default)
      415     0.001    0.000    1.005    0.002 /opt/conda/lib/python3.9/site-packages/ipywidgets/widgets/trait_types.py:167(make_dynamic_default)

Notice the huge number of calls to certiain traitlets methods...

Here's output with 5.9

  ncalls  tottime  percall  cumtime  percall filename:lineno(function)
 879/362    0.004    0.000    0.753    0.002 /opt/conda/lib/python3.9/site-packages/ipywidgets/widgets/widget.py:474(__init__)
  879/464    0.007    0.000    0.722    0.002 /opt/conda/lib/python3.9/site-packages/ipywidgets/widgets/widget.py:490(open)
    52/42    0.000    0.000    0.645    0.015 /opt/conda/lib/python3.9/importlib/__init__.py:109(import_module)
71407/39950    0.019    0.000    0.468    0.000 /opt/conda/lib/python3.9/site-packages/traitlets/traitlets.py:692(__get__)
  893/478    0.024    0.000    0.459    0.001 /opt/conda/lib/python3.9/site-packages/ipywidgets/widgets/widget.py:565(get_state)
38374/19948    0.035    0.000    0.457    0.000 /opt/conda/lib/python3.9/site-packages/traitlets/traitlets.py:654(get)
8186/3986    0.015    0.000    0.418    0.000 /opt/conda/lib/python3.9/site-packages/traitlets/traitlets.py:1860(trait_defaults

[unkcpz]

Thanks @danielhollas, fantastic enhancement!

How do you get this cProfile? Do I need to put some code in the notebook? I am looking forward to giving this a try on QE app.

[danielhollas]

I've essentially just copy pasted the code from the notebook to a python script and ran it through python -m cProfile script.py I only needed to delete the display() since they obviously don't work outside of the notebook. I also needed to remove the github error handler hook, which is apparently dependent on some ipykernel features. Other than that it just worked. I am sure there are better ways to do this but this was the easiest.

[yakutovicha]

Thanks a lot, @danielhollas! It's a great improvement.

The only suggestion I have is to add it to the AWB instead. Traits are more of app dependencies rather than a core requirement.

It is already a dependency of the home app. I see no problem in adding traitlets~5.7 to AWB as well.

[unkcpz]

The only suggestion I have is to add it to the AWB instead.

I'd prefer to add to both places. Adding it to AWB means the dependencies of the app need to be bumped to use the new traitlets, bumping it here will make it available even for the old version apps.

[yakutovicha]

I'd prefer to add to both places. Adding it to AWB means the dependencies of the app need to be bumped to use the new traitlets, bumping it here will make it available even for the old version apps.

The more I think about this change, the more I am getting convinced that the change should be done in the app(s), not here.

People will get a notification that AWB has a newer version, and they can update it as soon as the app is released. This also gives a sense of improvement.

[danielhollas]

@yakutovicha well, I've given it a lot of thought as well but arrived at the opposite conclusion. 😄

I am happy to declare a minimum dependency on traitlets in AWB, but imho we should not be bumping its version via AWB, for multiple reasons...

The only suggestion I have is to add it to the AWB instead. Traits are more of app dependencies rather than a core requirement.

Traits are in fact very much a core dependency of the Jupyter ecosystem, as can be seen by looking at which packages depend on it.

$ pip show traitlets
...
Location: /opt/conda/lib/python3.9/site-packages
Requires: 
Required-by: aiidalab, aiidalab-home, fileupload, ipykernel, ipympl, ipython, ipywidgets, jupyter-server, jupyter-telemetry, jupyter_client, jupyter_core, jupyterhub, matplotlib-inline, nbclassic, nbclient, nbconvert, nbformat, notebook, widget-bandsplot

Bumping traitlets in AWB would mean it would get installed via pip, which does not respect version constraints coming from other packages, and so could in principle easily break the jupyter installation. When we install it via conda here, we can be sure that the version is compatible with all the other packages in the jupyter image.

Moreover, since AWB is a core dependency of most AiiDAlab apps, we should be rather conservative with dependency constraints, otherwise we will always run into incompatibility issues (as we often already do).

There is an interesting blog post about managing version dependencies in libraries (as opposed to applications). https://hynek.me/articles/semver-will-not-save-you/
I must say I was very unhappy when I first read it, but I am slowly coming around on some of the points that it makes.

People will get a notification that AWB has a newer version, and they can update it as soon as the app is released. This also gives a sense of improvement.

Not that I think it is a very good argument, but note that the speedup here will not be that noticeable until the aiida.orm import speedup is merged in aiida-core, which will anyway require new image.

However, I take your point about users getting notified about updates. Previously in aiidalab-launch we would always download the newer image so it wouldn't be an issue for those who use it in local deployments. Now that we've switched to downloading it on demand, I think we should add a notification that a new image is available. I'll open an issue for that.

[danielhollas]

However, I take your point about users getting notified about updates. Previously in aiidalab-launch we would always download the newer image so it wouldn't be an issue for those who use it in local deployments. Now that we've switched to downloading it on demand, I think we should add a notification that a new image is available. I'll open an issue for that.

I've opened an issue for this aiidalab/aiidalab-launch#188

Note that updating traitlets also visibly speeds up home page loading (all apps in principle). I'll provide some numbers later.

[yakutovicha]

Let me try to motivate why we shouldn't change the Docker container.

We build the Docker container to provide a minimal working environment. We try to minimize the image settings as much as possible, allowing the apps to install almost any dependencies they want. Installing a specific version of traitlets will not prevent this from being overridden. Some versions of traitlets might already be installed in ~/.local and take priority w.r.t. the container ones.

Traits are in fact very much a core dependency of the Jupyter ecosystem, as can be seen by looking at which packages depend on it.

I'm afraid I have to disagree with that statement. We had a broad discussion on what we mean by core dependencies and we devised special procedures for handling them. Our dependency machinery won't attempt to update the core dependency. It will only allow apps to be installed if they can work with the given versions of core dependencies. Traitles can be overridden and are not protected, so they do not fall into this category.

Bumping traitlets in AWB would mean it would get installed via pip, which does not respect version constraints coming from other packages, and so could in principle easily break the jupyter installation. When we install it via conda here, we can be sure that the version is compatible with all the other packages in the jupyter image.

Yes, by design, AiiDAlab relies on pip when installing dependencies. Is it 100% safe? - No, and you've mentioned the reasons. But this is the compromise we came up with so far. If there are better alternatives - let's consider them.

Moreover, since AWB is a core dependency of most AiiDAlab apps, we should be rather conservative with dependency constraints, otherwise we will always run into incompatibility issues (as we often already do).

We worked hard to make it a non-core dependency. And, again, core dependencies have a specific meaning and are listed here.

There is an interesting blog post about managing version dependencies in libraries (as opposed to applications). https://hynek.me/articles/semver-will-not-save-you/
I must say I was very unhappy when I first read it, but I am slowly coming around on some of the points that it makes.

It is a thoughts provoking post, indeed. But it is unclear how it supports the need for current PR's change. However, it speaks volumes about how hard it is to keep a working system. The author proposes to use a specific version of dependencies and bump them/test them often. This can be achieved if communicating with AiiDA through REST API and keeping apps in separate environments. That is what we are eventually aiming for, but that requires a substantial amount of work and redesign.

Conclusion

Provided the circumstances we are currently in (pip-managed installation, multiple apps, conda environments), I find the change in the current PR is unnecessary. This won't be quick, this might not change the version of traitlets in some cases, and this is not where apps' dependencies are meant to be.

[unkcpz]

Hi @danielhollas, we had a short discussion on this in the last aiidalab meeting. I agree with @yakutovicha that this better goes to aiidalab-widget--base or as package dependencies explicitly.
I think @yakutovicha also working on the aiidalab-control widgets and it dependent on aiidalab-widgets-base with means the aiidalab-widgets-base will become the "core" dependencies and shipped with docker stack in the future so this one will be included automatically then.

[danielhollas]

Fair enough, although this was never meant to be a permanent solution, just a temporary quick win. 🤷

I think @yakutovicha also working on the aiidalab-control widgets and it dependent on aiidalab-widgets-base with means the aiidalab-widgets-base will become the "core" dependencies and shipped with docker stack in the future

This is fine, as long as AWB is published and installed via conda.

I'm afraid I have to disagree with that statement. We had a broad discussion on what we mean by core dependencies and we devised special procedures for handling them. Our dependency machinery won't attempt to update the core dependency. It will only allow apps to be installed if they can work with the given versions of core dependencies. Traitles can be overridden and are not protected, so they do not fall into this category.

I would like to flip this argument though. I think when we added the core dependencies, we were meant to come back to them and decide if we need to specify more of them as core. We definitely did not have a very robust discussion around what should be in the core, at least I don't remember. I would argue that traitlets and probably ipywidgets should be in this core category, because if an app decided to upgrade them it could easily break the whole environment.

[danielhollas]

I am re-opening this PR as a response to #494 where we found out that the current docker stack is incompatible with traitlets>=5.10, and the container fails to start. I am reiterating my belief that we should try as hard as possible to prevent apps from breaking the container. I will open a new issue with a proposed strategy about how to define and enforce core dependencies. In the interim we reverted AWB dependency back to traitlets>=5.4 to prevent users installing traitlets in ~/.local aiidalab/aiidalab-widgets-base#629. Installing these dependencies to ~/.local is a bad idea in general, because they will stay there even if user later upgrades their image, and they may thus overwrite the version in the base image.

This PR is just a stopgap solution that upgrades to a known good version of traitlets==5.9, which we upgrade here for performance reasons, as described at the top. This change will be soon reverted as part of an upgrade to Python 3.11 image in #455 which ships with traitlets 5.9 already.

[danielhollas]

@yakutovicha @unkcpz could you please give this your blessing (or otherwise), based on discussion in our last meeting? This is currently a blocker for releasing a new image with AiiDA 2.6. Thanks!

[yakutovicha]

Go ahead @danielhollas!