aiidateam / aiida-pseudo

MIT License
5 stars 8 forks source link

CLI: add install `--from-download` for sssp and pseudo-dojo #135

Closed mbercx closed 1 year ago

mbercx commented 1 year ago

Fixes #92

Our workaround for installing an "established" family like a SsspFamily or PseudoDojoFamily required using the install family command, but this is explicitly forbidden since we do not want the user to be able to install an arbitrary set of pseudopotentials as one of these classes. This means there is currently no way to install the archive and metadata obtained with the --download-only option of the automated install commands as a proper SsspFamily or PseudoDojoFamily.

Here, we add the --from-download option to the automated install commands to allow the user to install the downloaded pseudopotentials and set the recommended cutoffs automatically for an SsspFamily or PseudoDojoFamily. The --download-only option now also produces a single .aiida_pseudo archive that contains:

The user can pass such an archive to the from-download option to install the corresponding pseudopotential family.

mbercx commented 1 year ago

@sphuber still have to update the documentation instructions. Here's a quick example of the usage:

(aiida-pseudo) mbercx@theospc46:~/envs/aiida-pseudo/data$ aiida-pseudo install pseudo-dojo -x LDA -p standard -f upf --download-only
Report: downloading selected pseudopotentials archive...  [OK]
Report: downloading selected pseudopotentials metadata archive...  [OK]
Success: Pseudopotential archive written to: PseudoDojo_0.4_LDA_SR_standard_upf.aiida_pseudo
(aiida-pseudo) mbercx@theospc46:~/envs/aiida-pseudo/data$ ls
PseudoDojo_0.4_LDA_SR_standard_upf.aiida_pseudo
(aiida-pseudo) mbercx@theospc46:~/envs/aiida-pseudo/data$ aiida-pseudo install pseudo-dojo --from-download PseudoDojo_0.4_LDA_SR_standard_upf.aiida_pseudo 
Report: unpacking archive and parsing pseudos...  [OK]
Report: unpacking metadata archive and parsing metadata.../home/mbercx/envs/aiida-pseudo/code/aiida-pseudo/src/aiida_pseudo/groups/family/pseudo_dojo.py:278: UserWarning: filename Ge.djrepo_old does not have a supported extension. Skipping...
  warnings.warn(f'filename {filename} does not have a supported extension. Skipping...')
 [OK]
Success: installed `PseudoDojo/0.4/LDA/SR/standard/upf` containing 70 pseudopotentials

If we still agree on the --download-only and --from-download approach, I'll update the docs as well.

sphuber commented 1 year ago

If we still agree on the --download-only and --from-download approach, I'll update the docs as well.

Remind me, did we consider any alternatives? Did we figure out any obvious problems of this approach? If not, it seems reasonable to me.

mbercx commented 1 year ago

I think your comment here presents some of the alternatives discussed: https://github.com/aiidateam/aiida-pseudo/pull/94#issuecomment-839145418. You can also read the two comments below regarding the choice to switch to --from-download.

My one idea was still to extend this .aiida_pseudo archive idea to installing other families, so it's easier for people to share pseudo families with specified cutoffs, but that's beyond the scope of this PR. If you think it's a good idea I'll open an issue for it.

sphuber commented 1 year ago

I think your comment here presents some of the alternatives discussed: #94 (comment). You can also read the two comments below regarding the choice to switch to --from-download.

I remember now. I was wondering if only accepting a tarball with everything was not too limited. But I think I was maybe getting ahead of myself. Maybe let's first merge this and see if people actually need it and whether they would require even more control.

Edit: conclusion, let's keep the current implementation and please add the docs.

My one idea was still to extend this .aiida_pseudo archive idea to installing other families, so it's easier for people to share pseudo families with specified cutoffs, but that's beyond the scope of this PR. If you think it's a good idea I'll open an issue for it.

I like the concept, feature request would be good to discuss the details.

mbercx commented 1 year ago

@sphuber docs should be updated! So this PR is ready for review.

mbercx commented 1 year ago

I totally missed this review, sorry @sphuber! I'll be picking up this PR soon since it's important for rapid QEapp cloud deployments.

sphuber commented 1 year ago

@mbercx I am very much interested in getting this feature in for a use-case we have. Do you have time soon to finalize this? If not, I could also take over if you are ok with that.

mbercx commented 1 year ago

@mbercx I am very much interested in getting this feature in for a use-case we have. Do you have time soon to finalize this? If not, I could also take over if you are ok with that.

I'm moving from EPFL to PSI the next two days, but I could have a look at it over the weekend? Would be good to have it merged for sure.

mbercx commented 1 year ago

@sphuber thanks for the review! I think I addressed all your comments. I've moved the download_only logic up, which does lead to some code duplication of:

  1. The download call.
  2. Creating the label.
  3. Checking if the configuration is valid.

I've also moved the check if the configuration is already installed before the download call in https://github.com/aiidateam/aiida-pseudo/pull/135/commits/3882e06d93884d34643959449b65b642c64f0eb4. This is to avoid pointlessly downloading the pseudos, so the user doesn't see the following:

❯ aiida-pseudo install sssp
Report: downloading patch versions information...  [OK]
Report: downloading selected pseudopotentials archive...  [OK]
Report: downloading selected pseudopotentials metadata...  [OK]
Report: SsspFamily<SSSP/1.3/PBE/efficiency> is already installed

But instead

❯ aiida-pseudo install sssp
Report: SsspFamily<SSSP/1.3/PBE/efficiency> is already installed
unkcpz commented 11 months ago

@mbercx @sphuber could you make a patch release with this? I need it for the AiiDAlab QE, Thanks.

sphuber commented 11 months ago

@unkcpz done: https://pypi.org/project/aiida-pseudo/1.3.0/

unkcpz commented 11 months ago

Thanks a lot!