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 to v5.9 #526

Merged
merged 28 commits into from
Nov 9, 2023

Conversation

danielhollas
Copy link
Contributor

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

New version of traitlets contains optimizations
that significantly speed up loading of the app.
@danielhollas danielhollas self-assigned this Oct 13, 2023
@danielhollas danielhollas changed the title Bump traitlets to 5.9 Bump traitlets to v5.9 Oct 13, 2023
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #526   +/-   ##
=======================================
  Coverage   85.82%   85.82%           
=======================================
  Files          27       27           
  Lines        4608     4608           
=======================================
  Hits         3955     3955           
  Misses        653      653           
Flag Coverage Δ
python-3.10 85.82% <ø> (ø)
python-3.8 85.86% <ø> (ø)
python-3.9 85.86% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielhollas
Copy link
Contributor Author

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 danielhollas removed the request for review from unkcpz October 13, 2023 11:11
@danielhollas danielhollas marked this pull request as draft October 13, 2023 11:11
@danielhollas danielhollas removed the request for review from yakutovicha October 13, 2023 11:12
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

but only with Firefox which is very very strange.

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

@danielhollas
Copy link
Contributor Author

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

Yeah, it's very annoying. 😧 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. 😭

@unkcpz
Copy link
Member

unkcpz commented Oct 16, 2023

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

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

unkcpz commented Oct 16, 2023

@unkcpz
Copy link
Member

unkcpz commented Nov 8, 2023

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

@@ -43,7 +43,7 @@ jobs:
matrix:
tag: [latest]
browser: [Chrome, Firefox]
python-version: ['3.10']
python-version: ['3.9']
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
python-version: ['3.9']
python-version: ['3.10']

Copy link
Member

Choose a reason for hiding this comment

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

let me try to change this back to 3.10 see if it still fine.

@unkcpz unkcpz added this to the v2.1.0 milestone Nov 8, 2023
@unkcpz unkcpz marked this pull request as ready for review November 8, 2023 21:24
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Notebook tests all pass 🥂 ! Just one minor request, see if it is correct.

tests_notebooks/docker-compose.yml Outdated Show resolved Hide resolved
@unkcpz
Copy link
Member

unkcpz commented Nov 8, 2023

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.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

All tests passed. (finally 😄 )

@danielhollas danielhollas merged commit 665375a into aiidalab:master Nov 9, 2023
7 checks passed
@danielhollas danielhollas deleted the deps/bump-traitlets branch November 9, 2023 14:10
@danielhollas
Copy link
Contributor Author

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

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.

2 participants