aiidalab / aiidalab-widgets-base

Reusable widgets for AiiDAlab applications
MIT License
6 stars 17 forks source link

Bump traitlets to v5.9 #526

Closed danielhollas closed 8 months ago

danielhollas commented 9 months ago

New version of traitlets contains optimizations that significantly speed up loading of the app.

As discussed in https://github.com/danielhollas/aiidalab-widgets-base/pull/new/deps/bump-traitlets

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ac96dc6) 85.82% compared to head (9b2c30a) 85.82%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #526 +/- ## ======================================= Coverage 85.82% 85.82% ======================================= Files 27 27 Lines 4608 4608 ======================================= Hits 3955 3955 Misses 653 653 ``` | [Flag](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/526/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [python-3.10](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `85.82% <ø> (ø)` | | | [python-3.8](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `85.86% <ø> (ø)` | | | [python-3.9](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `85.86% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

danielhollas commented 9 months ago

Hmm, the notebook tests are failing during pip install, but only with Firefox which is very very strange. Need to investigate more, but unfortunately I don't see the error in the output, the installation seems to finish fine.

danielhollas commented 9 months ago

I saw chrome notebook test also failed. What is that free -m for?

Yeah, it's very annoying. :anguished: The original errors indicated that the pip process got killed by the OS OOM killer at the very end of the installation (there was no error message and the process return exit code 137, i.e. SIGKILL). I wanted to see how much memory is available inside the container. But now I see errors coming from the actual selenium tests, so perhaps there are real issues. Need to investigate more, but will probably not get to it today.

I hoped this would go smoother. :sob:

unkcpz commented 9 months ago

Maybe I can try to use the same way as we now test the notebook of QeApp, the image is build and the package is installed inside the container. It then makes the container test easy to reproduce locally. If there is no need to consider multi-arch and dockerhub uploaded, then the CI for the docker image build should be simple.

danielhollas commented 9 months ago

@unkcpz not sure what you mean, but we already do install AWB inside the container when running the notebook tests, see fixtures in the conftest.

I got ill over the weekend so will not be able to work on this today, and probably in the next few days. Feel free to continue. Imo the first thing is to just try installing AWB from this branch locally and verify that it works.(I should have done that myself). (and perhaps monitor pips use of memory during install).

unkcpz commented 9 months ago

Here is how qe app build the qe image with the things installed https://github.com/aiidalab/aiidalab-qe/blob/fc36106f315b2c46ec50995b048b4aa9bf5e5cd3/.github/workflows/docker-build-test-upload.yml#L48-L62

then there is not need to install the app through conftest.py. See https://github.com/aiidalab/aiidalab-qe/blob/fc36106f315b2c46ec50995b048b4aa9bf5e5cd3/tests_integration/conftest.py#L70-L93

unkcpz commented 8 months ago

Hehe, seems to work now. Time is the master of solving problems.

unkcpz commented 8 months ago

Emm... Now it is the unit test that stuck. I ran locally and it stuck at tests/test_databases.py::test_cod_query_widget, but it happened to the master branch as well, thus nothing related to the changes in this PR.

EDIT, cod query test timeout since the https://www.crystallography.net/ is down for the moment.

danielhollas commented 8 months ago

Okay, let's try this out. @unkcpz could you please release a new beta version? It would be great if you could also test this together with aiida-core 2.4