Add support for new ASE profile format
Summary of Changes
This PR seeks to streamline the handling of parallelization information across codes in quacc, namely to update the behavior for the Espresso and ONETEP recipes that are based on GenericFileIOCalculator. This PR closes #1532 and is reliant on the merge of https://gitlab.com/ase/ase/-/merge_requests/3343.
In short, handling of parallel_info is avoided. Instead, users provide quacc settings that will automatically apply in the instantiated Profile object's command keyword argument.
For ONETEP, that means the ONETEP_PARALLEL_CMD changes from a dict type to a str type.
For Espresso, that means parallel_info is removed from each recipe and in its place there is an ESPRESSO_PARALLEL_CMD setting that takes a str.
Then behavior is the same as every other recipe.
Checklist
- [X] I have read the "Guidelines" section of the contributing guide. Don't lie! 😉
- [X] My PR is on a custom branch and is not named
main. - [X] I have added relevant, comprehensive unit tests.
Notes
- Your PR will likely not be merged without proper and thorough tests.
- If you are an external contributor, you will see a comment from @buildbot-princeton. This is solely for the maintainers.
- When your code is ready for review, ping one of the active maintainers.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 99.04%. Comparing base (
29de805) to head (4be1feb). Report is 1 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #2061 +/- ##
=======================================
Coverage 99.04% 99.04%
=======================================
Files 81 81
Lines 3357 3364 +7
=======================================
+ Hits 3325 3332 +7
Misses 32 32
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I had a quick look and I am not sure if this allows for command suffix as well? In espresso something like
"mpirun -v -np 10 pw.x -nk 2 -nt 8 -ndiag 36 -in pw.in" is possible, but not supported currently, will the new mechanism in https://gitlab.com/ase/ase/-/merge_requests/3343/diffs#1107270596d43f253c632b6ffb2ce9c5e75460cb take care of that?
I had a quick look and I am not sure if this allows for command suffix as well? In espresso something like
"mpirun -v -np 10 pw.x -nk 2 -nt 8 -ndiag 36 -in pw.in" is possible, but not supported currently, will the new mechanism in https://gitlab.com/ase/ase/-/merge_requests/3343/diffs#1107270596d43f253c632b6ffb2ce9c5e75460cb take care of that?
This should be possible in ASE. It was agreed upon among the maintainers. Whether it is functional yet or is different. The MR still needs testing.
In terms of this PR, it's also still a WIP but I'll return to this when the ASE MR is merged. 👍
@tomdemeyere: With the ASE changes merged into master, I can now properly revisit this PR.
There are really only two points to discuss:
- For ONETEP, we can decide whether to: a) have a single
ONETEP_CMDsetting that is passed as-is tocommandinOnetepProfile(e.g. "mpirun -np 4 onetep.arch"), or b) have aONETEP_CMDthat is just the binary (e.g. "onetep.arch") and a separateONETEP_PARALLEL_CMDthat is the parallelization string that comes before the binary (e.g. "mpirun -np 4"). I am inclined to do the former since it is inherently more flexible. - For Espresso, it's a bit tricky because we can't just use a single
ESPRESSO_CMDpassed tocommandinEspressoProfilesince there is some automated logic for determining the binary. If we want to add commands before and after the binary per your request (which is indeed supported by ASE), we'll have to come up with a clean way to do that via the settings. It's mostly just a matter of preference.
@Andrew-S-Rosen
- Former is nicer I agree
- I am not sure I will let you decide on that one. Being able to do something like ESPRESSO_PARALLEL_CMD = "blablabla {binary} blablabla" would be nice?
It would also be nice to make sure that this is compatible to build the command automatically with Parsl as per https://github.com/Quantum-Accelerators/quacc/issues/2076 but that complicates things a little bit more.
@tomdemeyere: Thanks for your input! This should be taken care of now.
- For ONETEP, I went with the single
ONETEP_CMD: stroption, which is essentially identical to the ASEcommandin the profile. - For Espresso, I went with an
ESPRESSO_PARALLEL_CMD: str | tuple[str, str]approach for maximum flexibility. With thetuple[str, str], the 0th index is the string that comes before the binary, and the 1st index is the string that comes afterwards. For convenience, the user can also supply a singlestr, in which case it will be transformed internally tostr, ".
I'll merge this in for now to unbreak the tests since parallel_info is gone in ASE, but feel free to propose changes as you see fit.