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

Bump traitlets for better performance #401

Merged
merged 3 commits into from
Sep 6, 2024
Merged

Bump traitlets for better performance #401

merged 3 commits into from
Sep 6, 2024

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Jul 13, 2023

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 danielhollas marked this pull request as ready for review July 13, 2023 22:25
@danielhollas
Copy link
Contributor Author

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

@danielhollas danielhollas changed the title Bump trailets for better performance Bump traitlets for better performance Jul 14, 2023
@unkcpz
Copy link
Member

unkcpz commented Jul 14, 2023

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.

@unkcpz unkcpz added this to the 2023.1014 milestone Jul 14, 2023
@danielhollas
Copy link
Contributor Author

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
Copy link
Member

yakutovicha commented Jul 14, 2023

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
Copy link
Member

unkcpz commented Jul 14, 2023

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
Copy link
Member

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
Copy link
Contributor Author

danielhollas commented Jul 20, 2023

@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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

unkcpz commented Sep 25, 2023

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
Copy link
Contributor Author

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 danielhollas deleted the bump-traitlets branch July 16, 2024 09:43
@danielhollas danielhollas restored the bump-traitlets branch August 29, 2024 11:05
@danielhollas danielhollas reopened this Aug 29, 2024
@danielhollas
Copy link
Contributor Author

danielhollas commented Aug 29, 2024

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
Copy link
Contributor Author

@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!

Copy link
Member

@yakutovicha yakutovicha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go ahead @danielhollas!

@danielhollas danielhollas merged commit 87a01a8 into main Sep 6, 2024
15 checks passed
@danielhollas danielhollas deleted the bump-traitlets branch September 6, 2024 12:03
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

Successfully merging this pull request may close these issues.

3 participants