Feature/install from folder

[bosonie]

Fixes #79 Fixes #81

[bosonie]

I also fixed the CI problem every package had (pinned postgresql version).

[bosonie]

I'm also noticing now that

aiida-pseudo install family https://legacy-archive.materialscloud.org/file/2018.0001/v4/SSSP_1.0_PBE_efficiency.tar.gz label

that should be tested in tests/cli/test_install.py does actually fail in real life. Not because of a problem in the CLI command, but for failing of the underline family_type.create_from_folder. Is it expected?

[bosonie]

@sphuber @mbercx This is ready for review.

[sphuber]

I'm also noticing now that

aiida-pseudo install family https://legacy-archive.materialscloud.org/file/2018.0001/v4/SSSP_1.0_PBE_efficiency.tar.gz label

that should be tested in tests/cli/test_install.py does actually fail in real life. Not because of a problem in the CLI command, but for failing of the underline family_type.create_from_folder. Is it expected?

How is it failing exactly? And is it failing on the master branch or with your patch? Or both?

[bosonie]

I'm also noticing now that

aiida-pseudo install family https://legacy-archive.materialscloud.org/file/2018.0001/v4/SSSP_1.0_PBE_efficiency.tar.gz label

that should be tested in tests/cli/test_install.py does actually fail in real life. Not because of a problem in the CLI command, but for failing of the underline family_type.create_from_folder. Is it expected?

How is it failing exactly? And is it failing on the master branch or with your patch? Or both?

Info: unpacking archive and parsing pseudos...  [FAILED]
Critical: `<class 'aiida_pseudo.data.pseudo.pseudo.PseudoPotentialData'>` constructor did not define the element and could not parse a valid element symbol from the filename `{filename}` either. It should have the format `ELEMENT.EXTENSION`

Also in master

[sphuber]

Also in master

I see, well the problem should actually be solved by your PR. Try to run the following on your branch:

aiida-pseudo install family https://legacy-archive.materialscloud.org/file/2018.0001/v4/SSSP_1.0_PBE_efficiency.tar.gz label -P pseudo.upf

As stated in the docstring, the base family type PseudoPotentialFamily will use the basic PseudoPotentialData for the pseudopotentials. The PseudoPotentialData is a base class for "any" format, but it still needs an element. For now, the way I coded it, it only tries to parse this from the filename, because the file content could be anything and there is no way how to parse it for the element. So we impose a strict naming scheme for the filename {ELEMENT}.extension. If this is not the case, we cannot parse the element and so this error is raised. Apparently the SSSP_1.0_PBE_efficiency.tar.gz archive contains a file that doesn't follow this convention and so the element cannot be parsed, hence the error. I am a bit hesitant to start making the parsing more clever because there will always be edge cases that we won't catch and this CLI command was supposed to be just a last way out. One can always use the API to directly to create a family however they want.

[bosonie]

I see, well the problem should actually be solved by your PR. Try to run the following on your branch:

Indeed!

[bosonie]

I applied all the changes a part form the one I commented. I did not created our own custom CLI parameter type so far. @sphuber let me know if you wish me to look into it. Ideally I would love to merge this soon and have a release as well. I will use this features in aiida-siesta and I'm close to a release there as well.

[sphuber]

Ideally I would love to merge this soon and have a release as well. I will use this features in aiida-siesta and I'm close to a release there as well.

Just out of curiosity, since this PR just changes the CLI, why would you need this for aiida-siesta? And there are still a few unaddressed comments. I will comment on those specifically so you know which ones.

[bosonie]

Just out of curiosity, since this PR just changes the CLI, why would you need this for aiida-siesta? And there are still a few unaddressed comments. I will comment on those specifically so you know which ones.

Because in the package we deliver also examples and material for a tutorial. They require the creation of pseudo families and I do not want to make the topic more complicated than necessary.

[sphuber]

Because in the package we deliver also examples and material for a tutorial. They require the creation of pseudo families and I do not want to make the topic more complicated than necessary.

But you guys use psml right, and pseudos from Pseudo Dojo? So why can't you use aiida-pseudo install pseudo-dojo?

[bosonie]

But you guys use psml right, and pseudos from Pseudo Dojo? So why can't you use aiida-pseudo install pseudo-dojo?

No, psml is a new addition and will be the standard in the future. The current "stable" version of siesta still not support psml, only psf.

[sphuber]

@bosonie I have added a commit that implements our own version of PathOrUrl that makes the behavior of the command more consistent with respect to error handling. I have updated and simplified the implementation of the install family command accordingly and. If you are ok with the change, I can merge this. One more thing I realized while working on this, is that we probably should keep the option to change the family type. Creating an SsspFamily doesn't make sense, but I have added the CutoffsFamily and this will allow one to then add custom recommended cutoffs which may be useful. I will add this now

[sphuber]

Haha, guess @mbercx agrees with me on that last part. I am not sure if we should add the RecommendedCutoffMixin to the PseudoPotentialFamily base class. Especially since we already have the CutoffsFamily, which indeed I mainly added for testing purposes, but now with the fully fledged install family command, it starts to make sense for real use. Maybe we should rename it slightly though to CutoffsPseudoPotentialFamily to still make it plainly obvious that it is a pseudopotential family. I would also still check the type and only allow pseudo.family and pseudo.family.cutoffs. I don't think we want to allow creating pseudo-dojo or SSSP families through this command. Those respective commands already do that and if there are problems with internet, they even have the option to install directly from archive on disk.

[sphuber]

@bosonie and @mbercx added the implementation for the family type. Please have a look

[mbercx]

Haha, guess @mbercx agrees with me on that last part.

😁

I am not sure if we should add the RecommendedCutoffMixin to the PseudoPotentialFamily base class.

Sure, I do appreciate the cleanliness of a mixin class. I'm just wondering if there is a use case for a pseudo family without recommended cutoffs. There probably is, I'm just too in ❤️ with the get_builder_from_protocol method.

Especially since we already have the CutoffsFamily, which indeed I mainly added for testing purposes, but now with the fully fledged install family command, it starts to make sense for real use. Maybe we should rename it slightly though to CutoffsPseudoPotentialFamily to still make it plainly obvious that it is a pseudopotential family.

Fully agree 👍

I would also still check the type and only allow pseudo.family and pseudo.family.cutoffs. I don't think we want to allow creating pseudo-dojo or SSSP families through this command. Those respective commands already do that and if there are problems with internet, they even have the option to install directly from archive on disk.

Although I do agree, I did have a use case for installing a different version of the SSSP today, since there is a small issue with the header of the lanthanide SSSP files and I wanted to test the fixed versions. To circumvent the restriction on the label, I installed the fixed version as SSSP/1.0/efficiency 😬 . Anyways, this is a rare developer use case, and can be easily accomplished by installing a CutoffsPseudoPotentialFamily in the future.

[sphuber]

OK, we are starting to cram a bit much into a single PR. I am going to factor out the recent additions and then rebase this so it includes mostly the original idea of fixing install family.

[bosonie]

All good from my side! Unlike @mbercx, I was always familiar with families without recommended cutoffs. The good side of a collaborative effort! Thanks a million!

[sphuber]

Thanks a lot @bosonie !