quacc icon indicating copy to clipboard operation
quacc copied to clipboard

Add support for new ASE profile format

Open Andrew-S-Rosen opened this issue 1 year ago • 3 comments

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.

Andrew-S-Rosen avatar Apr 29 '24 19:04 Andrew-S-Rosen

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.

codecov[bot] avatar Apr 29 '24 19:04 codecov[bot]

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?

tomdemeyere avatar Apr 29 '24 23:04 tomdemeyere

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. 👍

Andrew-S-Rosen avatar Apr 29 '24 23:04 Andrew-S-Rosen

@tomdemeyere: With the ASE changes merged into master, I can now properly revisit this PR.

There are really only two points to discuss:

  1. For ONETEP, we can decide whether to: a) have a single ONETEP_CMD setting that is passed as-is to command in OnetepProfile (e.g. "mpirun -np 4 onetep.arch"), or b) have a ONETEP_CMD that is just the binary (e.g. "onetep.arch") and a separate ONETEP_PARALLEL_CMD that 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.
  2. For Espresso, it's a bit tricky because we can't just use a single ESPRESSO_CMD passed to command in EspressoProfile since 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 avatar May 31 '24 03:05 Andrew-S-Rosen

@Andrew-S-Rosen

  1. Former is nicer I agree
  2. 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 avatar May 31 '24 08:05 tomdemeyere

@tomdemeyere: Thanks for your input! This should be taken care of now.

  1. For ONETEP, I went with the single ONETEP_CMD: str option, which is essentially identical to the ASE command in the profile.
  2. For Espresso, I went with an ESPRESSO_PARALLEL_CMD: str | tuple[str, str] approach for maximum flexibility. With the tuple[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 single str, in which case it will be transformed internally to str, ".

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.

Andrew-S-Rosen avatar May 31 '24 18:05 Andrew-S-Rosen