Aims profile update

[gelzinyte]

Mainly clean up the AimsProfile construction to work with the latest ASE release.

[bernstei]

Thanks. Should we make sure this is done for all the other DFT calculators that support profile? Even better if we could somehow abstract out a common function to deal with this.

Or we could go all in on the new ASE approach, only support profile. Unless we hate it enough to keep the workaround :)

[edited] I see you followed up. Can you look over the other DFT calculators and see if it makes sense to do some refactoring, or at least making them all consistent? This might involve checking that all the underlying ASE calculators (that we care about) use profiles now.

[gelzinyte]

I would rather have as few modifications on top of ASE as possible so that wfl is easier to pick up and behaves more as expected. Also, setting up the profile yourself isn't that big of a deal, once ASE documentation is updated, if it's pickleable (will check).

Enforcing a profile wouldn't be backwards compatible, though and I think people will take a while to update their scripts to ASE3.23, since a lot changed. Should we
a) tag a latest ase-3.22-compatible version or
b) keep both interfaces with something like if CalculatorProfile is not None: assert "profile" is in kwargs?

Let me look through how other calculators behave.

[bernstei]

I would rather have as few modifications on top of ASE as possible

I agree in principle. In that case, can we avoid doing anything with the keywords, and assume the user is passing the right ones (consistent with whatever ASE versions they have installed)? Or does that conflict with, e.g. the fact that we want each job to run in a separate subdirectory (e.g. for VASP)?

[bernstei]

I would rather have as few modifications on top of ASE as possible so that wfl is easier to pick up and behaves more as expected. Also, setting up the profile yourself isn't that big of a deal, once ASE documentation is updated, if it's pickleable (will check).

What do we think about doing nothing to the keywords, then, and leaving it up to people to pass the keywords that make sense for their ASE version? Is that feasible, or does the fact that we want to run each job in a separate subdirectory make that impossible?

[gelzinyte]

I've moved the general non-aims discussion to #314.

For aims, we can drop messing with the execution-related kwargs. Then the user should give

• 3.22: "aims_command" (e.g. srun -n N path/to/aims_bin.x )
• 3.23 profile (which gets loaded automatically from ase config file or the user can give a profile=AimsProfile(aims_command=srun -n N path/to/aims_bin.x ) among the kwargs)

_setup_calc_params is untouched.

To Do

• Check AimsProfile is pickleable
• Update docs examples
• Update unit tests

[bernstei]

@gelzinyte I know you preferred separate PRs, but I'm worried that since the testing is now happening on the latest ASE, various unrelated tests will fail, and it'll be a bit tricky to figure out whether the failures that are left in any given PR are or are not related to that PR, or to the other calculator wrappers that have't been updated yet.

Can you tell for this PR which tests are meant to be working now, and which ones we can expect to continue to fail until we update the other calculators? Also, do we want to keep two versions of the tests, ones that use command and ones that use a profile, depending on what ASE version it detects?

Or should we first get the CI testing to happen with the older ASE, and then explicitly change it to the new one and update all the tests?

[gelzinyte]

I mainly suggested separate PRs because I thought it'd take me longer to test the calculators I have never used and would need to install/compile. Separate PRs would also make it easier for others to contribute meanwhile.

Regardless of the number of PRs, maybe I can first fix Aims, set CI tests to use pip-installed ase-3.23 and mark the not yet updated calculators' tests as x-failing for now. And go from there with either adding more calculators to this PR or merging to main.

Having looked a bit more into the update, I would only keep the local ase configuration (profile)-based testing. They're only ever done locally, so the ase configuration file makes that convenient and it's for ase to make sure that both the configuration file and profile are supported. As for ase version-specific tests, I'm not sure if it's worth the effort to try and also support the 3.22 version.. (so no testing for command)

[bernstei]

Looks like the VASP calculator doesn't have a profile yet. Should I make a corresponding PR for VASP that we do at the same time, just to make sure that we don't introduce anything that completely breaks the older-style calculators, as long as those are still around?

Also, we could consider doing the same for Espresso, just to have a second data point for the ones that do use Profile.

[gelzinyte]

Looks like the VASP calculator doesn't have a profile yet. Should I make a corresponding PR for VASP that we do at the same time, just to make sure that we don't introduce anything that completely breaks the older-style calculators, as long as those are still around?

What would go in the PR, you mentioned some small changes? I haven't touched anything VASP-related and the tests on the github CI pass. Do your local VASP tests pass for this branch? If your worry is about more extensive testing with other calculators before this branch is merged, add them here?

Also, we could consider doing the same for Espresso, just to have a second data point for the ones that do use Profile.

If I were to update other calculators, I would rather come back to them at some later time. So a separate PR for QE would not hold up this branch? Or we could ask @jungsdao to contribute and test his changes directly here?

[bernstei]

What would go in the PR, you mentioned some small changes?

Just removing all the special handling of kwargs, basically, and then making sure that the tests are updated. But I guess as long as this PR passes by itself, there isn't really a need for the Vasp one to be tested at the same time.

[bernstei]

Just removing all the special handling of kwargs, basically, and then making sure that the tests are updated. But I guess as long as this PR passes by itself, there isn't really a need for the Vasp one to be tested at the same time.

I knew I had a reason to be worried. VASP has separate executables for gamma-point only calculations, and now we're dealing with that, I don't know how to generalize the current mechanism, which is to accept command_gamma and calculator_exec_gamma arguments, save them someplace special, and modify the underlying ASE calculator when they are needed. I'll try to simplify the current approach, but I don't really know how it's going to work if/when Vasp starts using a profile.

[gelzinyte]

I think it's fine to leave VASP (CASTEP, ..) similar to how it's been so far until it's clear how it's updated in ASE?

[bernstei]

I think it's fine to leave VASP (CASTEP, ..) similar to how it's been so far until it's clear how it's updated in ASE?

I think we should clean up all the command_exec stuff, but I think that'll be quite easy. Let's move this to the PRs for the individual calculators, like I created for Vasp #317

[bernstei]

@gelzinyte please let me know explicitly when you think it's ready for another (final?) review.

[gelzinyte]

This should be it!

[bernstei]

Given that this PR now specifies a particular ASE version in the CI testing, what do you think about changing the dependencies and/or docs (in particular the top level README.md) to indicate what we are now doing w.r.t. this dependencies and its version?

In fact, what should we do about the ASE version in the installation file? >= 3.23.0, but say that it's only tested against 3.23.0 itself? I don't want to always force people to downgrade their ASE.

[gelzinyte]

Yes, at least a note in the README will be good.

Actually, why not to test against the latest pip-installable ase version? (That's why I initially didn't specify a version.) The latest wfl version should work with ase v3.22 with the correct keywords (even if we don't test this), so I think we can leave 'ase>=3.22.1' in the requirements.

We should tag a new version of wfl (0.4.0?) once the other calculators are updated. We could then enforce ase 3.23, because there then would be an option to pip install wfl==0.3.

[bernstei]

Yes, at least a note in the README will be good.

Can you add that as part of this PR?

Actually, why not to test against the latest pip-installable ase version? (That's why I initially didn't specify a version.) The latest wfl version should work with ase v3.22 with the correct keywords (even if we don't test this), so I think we can leave 'ase>=3.22.1' in the requirements.

I'm ambivalent about this. Maybe we should discuss on the slack?

[edited] I'm leaning toward testing on the latest (which would mean undoing the change I asked for in the CI ase install, sorry), and requiring 3.22.1 as an install prerequisite.

We should tag a new version of wfl (0.4.0?) once the other calculators are updated. We could then enforce ase 3.23, because there then would be an option to pip install wfl==0.3.

Yes, that's a good idea.

[bernstei]

@gelzinyte why don't you remove the ase version from the CI, but leave it with pip, not github. And we'll leave the pyproject dependency as is.

[gelzinyte]

Done. Have also updated readme and home page of documentation.