aiidateam / aiida-pseudo

MIT License
5 stars 8 forks source link

Decouple the maintenance of pseudo libraries version from the release #146

Closed unkcpz closed 1 year ago

unkcpz commented 1 year ago

When a new version of pseudo libraries is released, the package needs to be updated to able to download the new pseudo version. This lead to the issue that having to update aiida-core version in order to install new library version. It seems the pseudos libraries version is bookkeeping in https://github.com/aiidateam/aiida-pseudo/blob/master/src/aiida_pseudo/groups/family/sssp.py for SSSP and https://github.com/aiidateam/aiida-pseudo/blob/master/src/aiida_pseudo/groups/family/pseudo_dojo.py for pseudo-dojo. Probably deserve to move the list and maintain it separately so the old version can download new pseudo libraries.

Here is the commit of changes to update SSSP to v1.2 https://github.com/aiidateam/aiida-pseudo/pull/144/files If all information can be fetched from like https://aiida-pseudo.github.io/ it will be useful.

sphuber commented 1 year ago

If all information can be fetched from like https://aiida-pseudo.github.io/ it will be useful.

I agree that this would make it easier to maintain when new versions are released, however, one problem that I see is that now the properly functioning of the package becomes dependent on this online resource functioning properly. If, for whatever reason, the website that stores the list of valid families goes down, the package no longer works.

Moreover, all functionality that need to get a list of available families, for example the CLI commands to install families, will now have to perform an HTTP request just to get the list. So for example, take the option in aiida-pseudo install sssp that denotes the valid versions:https://github.com/aiidateam/aiida-pseudo/blob/1410e6879a38792b5a33515efffa8c2670d7dc40/src/aiida_pseudo/cli/install.py#L185 This will now have to make an HTTP request. It should do so each time the command line is invoked, even just to provide the help string. Of course we can cache the request, but then you have to deal with cache invalidation.

So with all that in mind, for now I would just be tempted to leave it as is. Instead, we should think of upping the version number for aiida-pseudo to v1.0. I think it is being used in production and so can be considered stable enough. This will allow packages downstream to declare aiida-pseudo~=1.0 and they will automatically use a new minor version when we release it with support for a new family. I think the problem currently is that packages are pinning the minor version and we are bumping it for a new family.

What do you think @unkcpz @mbercx ?

mbercx commented 1 year ago

I agree that we should release aiida-pseudo as v1.0, was already planning to bring this up after the last release since aiida-quantumespresso also needed a release to use the SSSP v1.2 without pip complaining. ^^

sphuber commented 1 year ago

Ok, then I will go ahead and release v1.0 with the current state and then we can update aiida-quantumespresso to depend on that so we no longer have to update when a minor version is bumped.

mbercx commented 1 year ago

Maybe we can also remove the workaround discussed in https://github.com/aiidateam/aiida-pseudo/issues/76 now and close that issue.

unkcpz commented 1 year ago

Thanks both! If I understand correctly, releasing aiida-pseudo as v1.0 can solve the installing problem for packages that already support AiiDA 2.x but not further bring compatibility of new pseudos libraries to the packages not migrate to aiida-core 2.x. Is that possible to release 0.7.1 to have SSSP 1.2 supported? The issue comes from I need to add support to qe-input-generator which using aiida-pseudo to install the SSSP library. I can either migrate the tool to AiiDA 2.x, which causes more effort or just install the SSSP1.2 using aiida-pseudo if this support can be added.

unkcpz commented 1 year ago

I agree that this would make it easier to maintain when new versions are released, however, one problem that I see is that now the properly functioning of the package becomes dependent on this online resource functioning properly. If, for whatever reason, the website that stores the list of valid families goes down, the package no longer works.

Yes, this adds one more uncertainty that can break the work flow. But aiida-pseudo anyway needs to connect to the internet to fetch the library, the GitHub server in general more stable than the materials cloud one :p So I would not too much worry about this.

sphuber commented 1 year ago

Yes, this adds one more uncertainty that can break the work flow. But aiida-pseudo anyway needs to connect to the internet to fetch the library, the GitHub server in general more stable than the materials cloud one :p So I would not too much worry about this.

I agree, but the stability of the Github server was not my only point. The main point is that this information is needed by the package quite often, for very simple things, which now will have to defer to an HTTP request.

sphuber commented 1 year ago

Thanks both! If I understand correctly, releasing aiida-pseudo as v1.0 can solve the installing problem for packages that already support AiiDA 2.x but not further bring compatibility of new pseudos libraries to the packages not migrate to aiida-core 2.x. Is that possible to release 0.7.1 to have SSSP 1.2 supported? The issue comes from I need to add support to qe-input-generator which using aiida-pseudo to install the SSSP library. I can either migrate the tool to AiiDA 2.x, which causes more effort or just install the SSSP1.2 using aiida-pseudo if this support can be added.

I see, I wasn't aware of this problem. I can definitely have a look if I can release a version that is compatible with aiida-core==1.6.x. You say that v0.7 was still compatible with v1.x but this commit https://github.com/aiidateam/aiida-pseudo/commit/a59c5510709fc2ed96c3e5f454cab2d30c246cc9 seems to suggest that the requirement for v2.0 was added in v0.7. So shouldn't it be v0.6.x?

unkcpz commented 1 year ago

You say that v0.7 was still compatible with v1.x but this commit https://github.com/aiidateam/aiida-pseudo/commit/a59c5510709fc2ed96c3e5f454cab2d30c246cc9 seems to suggest that the requirement for v2.0 was added in v0.7. So shouldn't it be v0.6.x?

Yes, I mean 0.6.x. The 0.7 already start to only support 2.0

unkcpz commented 1 year ago

@sphuber thanks! I test and all four commands in https://github.com/materialscloud-org/tools-qe-input-generator/blob/1f20c804032db820389757f1699e4bd98737b0c7/compute/scripts/aiida_setup.sh#L84-L87 works fine and they download the SSSP v1.2 of PBE/PBEsol libraries.

sphuber commented 1 year ago

Released v0.6.4: https://pypi.org/project/aiida-pseudo/0.6.4/

sphuber commented 1 year ago

With v1.0.0 and v0.6.4 released, I will consider this issue addressed for the time being.

unkcpz commented 1 year ago

Sure! It solves my issue. Thanks!