Install pseudos from downloaded files #524

unkcpz · 2023-10-21T16:17:53Z

fixes #490

The relevant changes needed are:

pinging the aiida-pseudo version.
Refactoring the methods in setup_pseudos.py to support flexible parameter passing.

Note that from the test all 8 pseudos seems to only save ~10s but in the place where network condition are bad, this is very helpful.

unkcpz · 2023-10-21T16:18:31Z

Slightly blocked by #523 where the modules are restructured.

codecov · 2023-10-21T16:26:33Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (8a550c8) 79.07% compared to head (515bdb3) 80.72%.
Report is 6 commits behind head on main.

❗ Current head 515bdb3 differs from pull request most recent head 9de1dea. Consider uploading reports for the commit 9de1dea to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #524      +/-   ##
==========================================
+ Coverage   79.07%   80.72%   +1.65%     
==========================================
  Files          47       49       +2     
  Lines        3211     3414     +203     
==========================================
+ Hits         2539     2756     +217     
+ Misses        672      658      -14

Flag	Coverage Δ
python-3.10	`80.72% <97.68%> (+1.65%)`	⬆️
python-3.8	`?`
python-3.9	`80.76% <97.68%> (+1.65%)`	⬆️

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

Files	Coverage Δ
src/aiidalab_qe/app/configuration/__init__.py	`98.79% <100.00%> (+0.04%)`	⬆️
src/aiidalab_qe/app/configuration/advanced.py	`98.19% <100.00%> (+0.03%)`	⬆️
src/aiidalab_qe/app/configuration/pseudos.py	`88.98% <100.00%> (-0.45%)`	⬇️
src/aiidalab_qe/app/main.py	`98.57% <100.00%> (+2.27%)`	⬆️
src/aiidalab_qe/app/structure/__init__.py	`98.63% <100.00%> (+0.03%)`	⬆️
src/aiidalab_qe/app/submission/__init__.py	`81.06% <100.00%> (+1.73%)`	⬆️
src/aiidalab_qe/common/panel.py	`86.95% <100.00%> (ø)`
src/aiidalab_qe/plugins/bands/result.py	`94.73% <100.00%> (ø)`
...idalab_qe/plugins/electronic_structure/__init__.py	`100.00% <100.00%> (ø)`
src/aiidalab_qe/plugins/pdos/result.py	`92.95% <100.00%> (ø)`
... and 13 more

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

unkcpz · 2023-10-23T21:59:23Z

Blocked by aiidateam/aiida-pseudo#165

src/aiidalab_qe/common/setup_pseudos.py

src/aiidalab_qe/workflows/__init__.py

unkcpz · 2023-10-25T13:01:10Z

@superstar54, this is ready for review.
The version of pseudo libraries now should only need to be updated in two places for all.
This change also accelerates the first time of start the container ~24s, will be more for server with bad network condition.

superstar54

Great job on working on this feature. It will improve the user experience. I added a few change requests.

docker/Dockerfile

docker/before-notebook.d/71_install-qeapp.sh

src/aiidalab_qe/__main__.py

src/aiidalab_qe/common/setup_pseudos.py

superstar54 · 2023-10-27T20:03:18Z

src/aiidalab_qe/app/configuration/pseudos.py

        if library == "PseudoDojo":
-            pseudo_family_string = f"PseudoDojo/0.4/{functional}/SR/{accuracy}/upf"
+            pseudo_family_string = (
+                f"PseudoDojo/{PSEUDODOJO_VERSION}/{functional}/SR/{accuracy}/upf"


Becareful here. The setup_pseudos in the common folder is something we want to move to the aiidalab-widgets-base, so it's better not to import the PSEUDODOJO_VERSION from the setup_pseudos. The pseudo version is a local configuration parameter belonging to the app itself.

I also thought about this when I moved version and family data class to common/setup_pseudos.py. First of all it has to be moved outside from app/ since if it is inside, it will import app/ which need the profile and can not be called from Dockerfile which doesn't have a profile setup. Second, it fit quite well in the common/, if we move pseudos stuff to let's say aiidalab-widget-base, this PSEUDODOJO_VERSION can be imported from the AWB or if QeApp needs to define its own version, just define it as we did in qeapp.yaml.

Since you already move it outside the app folder, please define the version in the qeapp.yaml.

I am not sure it is doable, the SSSP_VERSION is defined in common/setup_pseudos.py which is not in the app module in order to avoid loading profile when calling aiidalab-qe install. The qeapp.yaml is in the app module instead.

in order to avoid loading profile when calling aiidalab-qe install

You mean aiidalab-qe install_pseudos. I checked this function, it needs load the profile at the beginning.

The problem here is: we want to make the setup_pseudos.py as a common function, but it doesn't allow users to customize the pseudos.
I would suggest passing the pseudo info into the install method in the setup_pseudos.py.

src/aiidalab_qe/common/setup_pseudos.py

tests/test_codes.py

tests/test_pseudo.py

superstar54

A few small changes are needed.

src/aiidalab_qe/__main__.py

docker/Dockerfile

superstar54 · 2023-11-07T09:09:16Z

src/aiidalab_qe/app/configuration/pseudos.py

        if library == "PseudoDojo":
-            pseudo_family_string = f"PseudoDojo/0.4/{functional}/SR/{accuracy}/upf"
+            pseudo_family_string = (
+                f"PseudoDojo/{PSEUDODOJO_VERSION}/{functional}/SR/{accuracy}/upf"


Since you already move it outside the app folder, please define the version in the qeapp.yaml.

unkcpz · 2023-11-07T10:20:48Z

docker/Dockerfile

+     # The command wil first install the dependencies from list by parsing setup config files,
+     # (for `aiidalab/aiidalab<23.03.2` the `setup.py` should be in the root folder of the app https://github.com/aiidalab/aiidalab/pull/382).


I hope this is clear.

What will happen if I git clone the repo and install it using pip?

As a developer, you can install it with pip, but from appstore it calls aiidalab install. Here in Dockerfile I use it to test the appstore installation behavior is correct. You can have a look at #532 why I changed it to this to spot the error which not raised before.

superstar54 · 2023-11-08T08:18:18Z

docker/Dockerfile

+     # The command wil first install the dependencies from list by parsing setup config files,
+     # (for `aiidalab/aiidalab<23.03.2` the `setup.py` should be in the root folder of the app https://github.com/aiidalab/aiidalab/pull/382).


What will happen if I git clone the repo and install it using pip?

superstar54 · 2023-11-08T08:31:13Z

src/aiidalab_qe/app/configuration/pseudos.py

        if library == "PseudoDojo":
-            pseudo_family_string = f"PseudoDojo/0.4/{functional}/SR/{accuracy}/upf"
+            pseudo_family_string = (
+                f"PseudoDojo/{PSEUDODOJO_VERSION}/{functional}/SR/{accuracy}/upf"


in order to avoid loading profile when calling aiidalab-qe install

You mean aiidalab-qe install_pseudos. I checked this function, it needs load the profile at the beginning.

The problem here is: we want to make the setup_pseudos.py as a common function, but it doesn't allow users to customize the pseudos.
I would suggest passing the pseudo info into the install method in the setup_pseudos.py.

tests/conftest.py

unkcpz · 2023-11-08T14:03:01Z

The issue is opened in #550 for future enhancement on the generic pseudo setup tool.

superstar54

LTGM.

unkcpz · 2023-11-08T21:58:42Z

@superstar54 thanks a lot for the review!

fixes #490 In this commit, we pinging the aiida-pseudo version to use the --download-only option to download the pseudo archive to the container for quick installation when the container first time start. Note that from the test all 8 pseudos seems to only save ~10s but in the place where the network condition is bad, this is very helpful. Further more Refactoring the methods in setup_pseudos.py to support flexible parameter passing.

unkcpz marked this pull request as draft October 21, 2023 16:17

unkcpz force-pushed the fix/490/pre-download-pseudos branch from 023346e to 9272b4e Compare October 23, 2023 20:07

unkcpz added the pr/blocked label Oct 23, 2023

unkcpz changed the title ~~Pining the lowest version of aiida-pseudo~~ Download pseudos and install later Oct 23, 2023

unkcpz changed the title ~~Download pseudos and install later~~ Install pseudos from downloaded files Oct 23, 2023

unkcpz force-pushed the fix/490/pre-download-pseudos branch from f5a4719 to e728098 Compare October 23, 2023 22:39

unkcpz marked this pull request as ready for review October 24, 2023 16:12

unkcpz requested a review from superstar54 October 24, 2023 16:12

unkcpz force-pushed the fix/490/pre-download-pseudos branch from e3b69f8 to e1c5737 Compare October 25, 2023 10:15

unkcpz commented Oct 25, 2023

View reviewed changes

src/aiidalab_qe/common/setup_pseudos.py Outdated Show resolved Hide resolved

src/aiidalab_qe/common/setup_pseudos.py Outdated Show resolved Hide resolved

src/aiidalab_qe/workflows/__init__.py Outdated Show resolved Hide resolved

unkcpz removed the pr/blocked label Oct 25, 2023

unkcpz requested a review from mikibonacci October 26, 2023 09:18

superstar54 requested changes Oct 27, 2023

View reviewed changes

unkcpz force-pushed the fix/490/pre-download-pseudos branch from 11fbaf1 to cd0e48b Compare November 2, 2023 14:51

unkcpz requested a review from superstar54 November 2, 2023 14:53

unkcpz force-pushed the main branch from 8a550c8 to e0f0e7e Compare November 2, 2023 15:41

unkcpz force-pushed the fix/490/pre-download-pseudos branch from 0768847 to a466b51 Compare November 2, 2023 16:57

superstar54 requested changes Nov 7, 2023

View reviewed changes

unkcpz force-pushed the fix/490/pre-download-pseudos branch from 86e07ed to d9cccf6 Compare November 7, 2023 10:19

unkcpz requested a review from superstar54 November 7, 2023 10:20

unkcpz commented Nov 7, 2023

View reviewed changes

unkcpz force-pushed the fix/490/pre-download-pseudos branch from d9cccf6 to 0ea967d Compare November 7, 2023 10:22

superstar54 requested changes Nov 8, 2023

View reviewed changes

unkcpz commented Nov 8, 2023

View reviewed changes

tests/conftest.py Show resolved Hide resolved

unkcpz force-pushed the fix/490/pre-download-pseudos branch 2 times, most recently from 110c0ae to 16b9606 Compare November 8, 2023 10:53

unkcpz added 18 commits November 8, 2023 10:53

Tidy up the pseudo setup

2a0de9c

Add cwd and install from downloaded file

a33f6ea

Use patched aiida-pseudo

ba34235

Add CLI test for pseudo install and setup

95b03cc

fix test

1dd99ac

Fix a pseudo test by set sssp fixture to function scope

deeaf32

fix unit test

67538f0

fix the normal install call of pseudos

9a79faf

Add test to measure the time saved

5941523

More sssp fixture used

30cee68

pining aiida-pseudos to 1.4.0 to use download-only

945d440

Update src/aiidalab_qe/common/setup_pseudos.py

a90dced

Update src/aiidalab_qe/workflows/__init__.py

e5adc4f

Update src/aiidalab_qe/common/setup_pseudos.py

c4ae796

new

9ba741b

fix pseudo not installed blocker

dede466

f-x

c0006bd

qeapp.yaml use dict for default pseudo set

3728c94

unkcpz force-pushed the fix/490/pre-download-pseudos branch from 16b9606 to 3728c94 Compare November 8, 2023 10:54

unkcpz requested a review from superstar54 November 8, 2023 10:54

Merge branch 'main' into fix/490/pre-download-pseudos

f6edbdf

superstar54 approved these changes Nov 8, 2023

View reviewed changes

unkcpz added 2 commits November 8, 2023 15:41

Merge branch 'main' into fix/490/pre-download-pseudos

515bdb3

Merge branch 'main' into fix/490/pre-download-pseudos

9de1dea

unkcpz merged commit 772f1fd into main Nov 8, 2023
11 of 12 checks passed

unkcpz deleted the fix/490/pre-download-pseudos branch November 8, 2023 21:58

unkcpz mentioned this pull request Nov 8, 2023

Put all pseudo default library version setting to one place. #391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install pseudos from downloaded files #524

Install pseudos from downloaded files #524

unkcpz commented Oct 21, 2023 •

edited

Loading

unkcpz commented Oct 21, 2023

codecov bot commented Oct 21, 2023 •

edited

Loading

unkcpz commented Oct 23, 2023

unkcpz commented Oct 25, 2023

superstar54 left a comment

superstar54 Oct 27, 2023

unkcpz Oct 28, 2023

superstar54 Nov 7, 2023

unkcpz Nov 7, 2023

superstar54 Nov 8, 2023

superstar54 left a comment

superstar54 Nov 7, 2023

unkcpz Nov 7, 2023 •

edited

Loading

superstar54 Nov 8, 2023

unkcpz Nov 8, 2023

superstar54 Nov 8, 2023

superstar54 Nov 8, 2023

unkcpz commented Nov 8, 2023

superstar54 left a comment

unkcpz commented Nov 8, 2023

		# The command wil first install the dependencies from list by parsing setup config files,
		# (for `aiidalab/aiidalab<23.03.2` the `setup.py` should be in the root folder of the app https://github.com/aiidalab/aiidalab/pull/382).

Install pseudos from downloaded files #524

Install pseudos from downloaded files #524

Conversation

unkcpz commented Oct 21, 2023 • edited Loading

unkcpz commented Oct 21, 2023

codecov bot commented Oct 21, 2023 • edited Loading

Codecov Report

unkcpz commented Oct 23, 2023

unkcpz commented Oct 25, 2023

superstar54 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superstar54 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented Nov 8, 2023

superstar54 left a comment

Choose a reason for hiding this comment

unkcpz commented Nov 8, 2023

unkcpz commented Oct 21, 2023 •

edited

Loading

codecov bot commented Oct 21, 2023 •

edited

Loading

unkcpz Nov 7, 2023 •

edited

Loading