-
Notifications
You must be signed in to change notification settings - Fork 8
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
Feature/install from folder #80
Conversation
I also fixed the CI problem every package had (pinned postgresql version). |
I'm also noticing now that
that should be tested in |
How is it failing exactly? And is it failing on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bosonie . I have given it a first pass. I am wondering if we should reuse the PathOrUrl
from aiida-core
. It checks whether the remote URL can be fetched but than discards the result. Maybe it would be best to create our own custom type that does everything in one go as well as validation. But I can look at this myself later.
Also in master |
I see, well the problem should actually be solved by your PR. Try to run the following on your branch:
As stated in the docstring, the base family type |
Indeed! |
I applied all the changes a part form the one I commented. |
Just out of curiosity, since this PR just changes the CLI, why would you need this for |
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 |
No, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bosonie! I tried it out on a folder with pseudos I actually needed for a project, so you saved me the trouble of putting them in an archive! ;)
I just had one comment regarding removing the FAMILY_TYPE
input. Although I do understand that if someone wants to install an SsspFamily
or PseudoDojoFamily
, they can use the corresponding command, I actually just now noticed that the base PseudoPotentialFamily
doesn't have the RecommendedCutoffMixin
as a parent class. So any pseudos installed with the command currently cannot use this functionality. There is the CutoffsFamily
class, which @sphuber added mainly for testing purposes, but I'm thinking that perhaps we should make it possible for users to install this class type with the install family
command. Or, is there any reason not to give the base PseudoPotentialFamily
class the methods for adding and using recommended cutoffs?
@bosonie I have added a commit that implements our own version of |
Haha, guess @mbercx agrees with me on that last part. I am not sure if we should add the |
😁
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
Fully agree 👍
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks @sphuber! Just a small comment that we should perhaps help the user to the right command if they want to install sssp
or pseudo_dojo
families.
Also: Maybe we should immediately adapt the docstring of CutoffsFamily
now that it is a proper class with use cases beyond testing? :)
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 |
0831dcb
to
a41d738
Compare
All good from my side! Unlike @mbercx, I was always familiar with families without recommended cutoffs. The good side of a collaborative effort! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
family.description = description | ||
echo.echo_success(f'installed `{label}` containing {family.count()} pseudo potentials') | ||
echo.echo_success(f'installed `{label}` containing {family.count()} pseudopotentials') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learn from the best
Uptil now, the `install family` command only accepted (compressed) archives of a directory containing the pseudopotential files. Often, however, a user may already have an unpacked directory on local disk and it would be ridiculous to force them to archive it first before being able to use it with the CLI. The parameter type of the `archive` argument is changed to use the `PathOrUrl` type of `aiida-core`. This type will treat the value as a URL and fetch its content if the value is not a valid filepath on the local file system. This type is not ideal since its error handling is different from the rest of the `aiida-pseudo` CLI so its a bit inhomogeneous, but this will be improved later. Additionally, the `--pseudo-type` option was added. Historically, the command only exposed the `--family-type` option since in the beginning of `aiida-pseudo` each family type defined its own hardcoded pseudo type so it was implied. Since then, families can define their pseudo type per instance and so now the `--pseudo-type` option has become necessary to let the user specify which pseudopotential data class should be used.
Instead of using the `PathOrUrl` parameter type from `aiida-core` we add our own implementation that uses the `requests` library instead of `urllib`, but most importantly, it attempts to retrieve the URL in the `attempt` context manager that will catch any exceptions and properly display the error and exit the command, just like other important parts of the command that can fail. In addition, the `PseudoPotentialFamilyParamType` is updated to take an `exclude` list at construction time to signify a list of values that are considered to be invalid. We use this in the `install family` command to blacklist the `pseudo.family.sssp` and `pseudo.family.pseudo_dojo` since they have their own dedicated install commands that should be used instead. Finally, the `-F/--archive-format` option is changed to use `-f` for the shorthand instead, such that it allows `-F` for the `--family-type` which makes more sense there given that we have `-P/--pseudo-type`. The option was only in use by `install family` which was a broken command currently anyway, so the breaking change shouldn't affect any users.
a41d738
to
c53d623
Compare
Instead of using the `PathOrUrl` parameter type from `aiida-core` we add our own implementation that uses the `requests` library instead of `urllib`, but most importantly, it attempts to retrieve the URL in the `attempt` context manager that will catch any exceptions and properly display the error and exit the command, just like other important parts of the command that can fail. In addition, the `PseudoPotentialFamilyParamType` is updated to take an `exclude` list at construction time to signify a list of values that are considered to be invalid. We use this in the `install family` command to blacklist the `pseudo.family.sssp` and `pseudo.family.pseudo_dojo` since they have their own dedicated install commands that should be used instead. Finally, the `-F/--archive-format` option is changed to use `-f` for the shorthand instead, such that it allows `-F` for the `--family-type` which makes more sense there given that we have `-P/--pseudo-type`. The option was only in use by `install family` which was a broken command currently anyway, so the breaking change shouldn't affect any users.
c53d623
to
cadec98
Compare
Thanks a lot @bosonie ! |
Uptil now, the `install family` command only accepted (compressed) archives of a directory containing the pseudopotential files. Often, however, a user may already have an unpacked directory on local disk and it would be ridiculous to force them to archive it first before being able to use it with the CLI. The parameter type of the `archive` argument is changed to use the `PathOrUrl` type of `aiida-core`. This type will treat the value as a URL and fetch its content if the value is not a valid filepath on the local file system. This type is not ideal since its error handling is different from the rest of the `aiida-pseudo` CLI so its a bit inhomogeneous, but this will be improved later. Additionally, the `--pseudo-type` option was added. Historically, the command only exposed the `--family-type` option since in the beginning of `aiida-pseudo` each family type defined its own hardcoded pseudo type so it was implied. Since then, families can define their pseudo type per instance and so now the `--pseudo-type` option has become necessary to let the user specify which pseudopotential data class should be used.
Fixes #79
Fixes #81