Skip to content
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

Wien2k implementation #314

Merged
merged 8 commits into from
Aug 29, 2023
Merged

Wien2k implementation #314

merged 8 commits into from
Aug 29, 2023

Conversation

sphuber
Copy link
Collaborator

@sphuber sphuber commented Jul 14, 2023

No description provided.

rubel75 and others added 2 commits July 14, 2023 09:36
Also refactored the formatting of the `-red` option string to a
one-liner
@sphuber
Copy link
Collaborator Author

sphuber commented Jul 14, 2023

The tests fail because the required wien2k plugin is not installed. @rubel75 @bosonie where is the aiida-wien2k plugin located?

@sphuber
Copy link
Collaborator Author

sphuber commented Jul 14, 2023

supercedes #313

@bosonie
Copy link
Collaborator

bosonie commented Jul 14, 2023

The aiida-wien2k is now owned by aiida-team, @mbercx and I are in charge to take care of the release. I will coordinate with him.
Regarding this PR, for me it is the same to work here or merge this and open a new one. As you wish.

@sphuber
Copy link
Collaborator Author

sphuber commented Jul 24, 2023

Let's just add a commit on to this branch once the plugin is done and released. This way we can be sure the tests pass and don't break the master branch.

@mbercx mbercx force-pushed the wien2k_implementation branch 5 times, most recently from f881530 to fd1ba46 Compare August 18, 2023 01:00
@mbercx
Copy link
Member

mbercx commented Aug 18, 2023

Tests for 3.10 (see this action run) seem to be failing because of

yaml/pyyaml#724

I tried installing pyyaml==5.4.1 in a 3.10 environment on Ubuntu and am running into the same issue. (Funny enough, it works fine on mac). So we may have to update our dependencies for aiida-core 1.6.X.

After removing 3.10 from the test matrix though, the tests immediately fail because of abipy, see:

abinit/abipy#263

Let's see if I can get our tests to run...
Python 3.8 is not supported by `abipy`.

Fixing the `abipy` branch to one where I am fixing issues.
@mbercx
Copy link
Member

mbercx commented Aug 21, 2023

Alright, at least the Python 3.9 tests pass now! For Python 3.10, I think we need to update the pyyaml dependency of aiida-core v1.6.X. Python 3.8 is not supported by the latest version of abipy, so we may need to restrict to Python 3.9 and above.

@sphuber
Copy link
Collaborator Author

sphuber commented Aug 21, 2023

Alright, at least the Python 3.9 tests pass now! For Python 3.10, I think we need to update the pyyaml dependency of aiida-core v1.6.X. Python 3.8 is not supported by the latest version of abipy, so we may need to restrict to Python 3.9 and above.

That would essentially mean that we only support 3.9 because aiida-core==1.6.9 officially only supports 3.9: https://github.com/aiidateam/aiida-core/blob/v1.6.9/setup.json

I think this is quite limited and not ideal for a major release of this package. It would be great if we can check whether aiida-core v1.6.9 can add support for Python 3.10 and maybe 3.11 and also check whether abipy can add support for Python 3.7 or at least Python 3.8. Are they actually using functionality that is only introduced in Python 3.9? If so what, there are a number of modules that have backports.

@mbercx
Copy link
Member

mbercx commented Aug 21, 2023

It would be great if we can check whether aiida-core v1.6.9 can add support for Python 3.10 and maybe 3.11

Yes, that's also was I was thinking. I hope it will only require use to update the pyyaml dependency. Do you have time to look into this?

abipy can add support for Python 3.7 or at least Python 3.8.

I'll look into that. Python 3.7 is already past EOL, so I think we can skip that (aiida-abinit also doesn't support it), but Python 3.8 may be feasible for abipy. I hope it will only require some from __future__ import annotations additions.

@mbercx
Copy link
Member

mbercx commented Aug 21, 2023

Seems I only had to add one from __future__ import annotations to make our tests pass. Will open a PR into abipy to reintroduce support for Python 3.8.

@sphuber
Copy link
Collaborator Author

sphuber commented Aug 21, 2023

Yes, that's also was I was thinking. I hope it will only require use to update the pyyaml dependency. Do you have time to look into this?

If only it were so simple. The same dependency has also been pinned by plumpy which in turn relies on kiwipy. So these may also require new support releases. But I can try to have a look.

@sphuber
Copy link
Collaborator Author

sphuber commented Aug 22, 2023

@mbercx I solved the kiwipy and plumpy issues in this branch but as I expected, the real problems only start then. The installation runs for Python 3.7, 3.8 and 3.9, but the transport tests fail. This can most likely be fixed, however, for Python 3.10 and 3.11 the installation doesn't even succeed. This is again due to compilation errors when building pymatgen. These are very tricky to solve if even possible and I don't have the time nor interest currently to work on this, just to bring Python 3.10 and 3.11 support to an old version of AiiDA that has been out of support for a while. Think we will have to live with support for just Python 3.8 and 3.9 for this release of ACWF. We can then update to AiiDA v2.0 a.s.a.p and support more modern Python versions

@mbercx
Copy link
Member

mbercx commented Aug 22, 2023

@sphuber of course, I fully understand. Thanks for having a look anyways!

@bosonie
Copy link
Collaborator

bosonie commented Aug 29, 2023

@mbercx I fixed the small conflict about the abipy version in pyproject.toml.
Do you agree then that we merge this? We can keep testing and if some other issue comes up we can do another PR.
At least we have a working implementation.

@mbercx
Copy link
Member

mbercx commented Aug 29, 2023

@bosonie sure, we can go ahead and (Squash merge) this PR. I can confirm it runs WIEN2k just fine, just having trouble getting a working binary.

@mbercx mbercx self-requested a review August 29, 2023 21:47
@mbercx mbercx merged commit 3e4014d into master Aug 29, 2023
6 checks passed
@mbercx mbercx deleted the wien2k_implementation branch August 29, 2023 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants