Test for ase fileio caching with VASP example

[JPDarby]

Code from Noam to check if ase calculators are executing multiple singlepoints.

Not tested because I don't have a vasp license.

[bernstei]

Thanks @JPDarby . I know you don't have VASP, but can you run the espresso tests locally and pass those? I think that with ASE 3.23 all you need is the new profile-style (~/.config/ase/config.ini) setup that says how to run the pwscf command (the PPs are taken care of by the tests).

If so, can you add an espresso example for the new test (which will presumably fail the assert) ?

[JPDarby]

@bernstei I've added an espresso example and it passes the assert fine!

I think all my problems have been coming from using Espresso from ase directly rather the version from wfl i.e.
I've been running

from ase.calculators.espresso import  EspressoProfile, Espresso
calculator = (Espresso, [], current_kwargs)
generic.calculate( inputs = configset,  outputs = outputspec,   calculator = calculator)

rather than

from wfl.calculators.espresso import Espresso
from ase.calculators.espresso import  EspressoProfile
calculator = Espresso(**current_kwargs)
generic.calculate( inputs = configset,  outputs = outputspec,   calculator = calculator)

the second one works fine (no duplicated singlepoint) because the wfl Espresso does set self.atoms

[bernstei]

the second one works fine (no duplicated singlepoint) because the wfl Espresso does set self.atoms

That's good to hear. Do we think it'd be worth it to check (whether error, or at least warn), in generic, that the user isn't passing a wrapped calculator without its wrapper?

It'd be nice to have this test regardless, though. I assume it won't pass in the CI until we merge the ASE PR, but I'll let it sit here until that happens.

[JPDarby]

Yes! I'm definitely biased but I think a warning would be really really useful. I arrived at this way of doing things from modifying the first example in the documentation so very plausible it could happen to other people in the future.

[JPDarby]

It'd be nice to have this test regardless, though. I assume it won't pass in the CI until we merge the ASE PR, but I'll let it sit here until that happens.

So this test should pass without the ASE PR because wfl.calculators.espresso.Espresso has it's own calculate method which sets self.atoms

[bernstei]

OK. I'm looking into why the CI is failing - something to do with finding pytorch, unrelated to this actual change.

Can you add a test to generic that checks if the type of the calculator matches any of the ASE types that underlie the wrapped calculators, and a test to make sure it worked? https://docs.pytest.org/en/7.1.x/how-to/capture-warnings.html#warns

[bernstei]

@JPDarby can you also apply this patch, so we can see why the pytorch CI search is failing?

diff --git a/.github/workflows/pytests.yml b/.github/workflows/pytests.yml
index 11550bb..c4679bc 100644
--- a/.github/workflows/pytests.yml
+++ b/.github/workflows/pytests.yml
@@ -108,12 +108,15 @@ jobs:
           # search for available torch version with +cpu support
           for torch_version_test in $( python3 -m pip install torch== 2>&1 | fgrep 'from versions' | 
                                        sed -e 's/.*from versions: //' -e 's/)//' -e 's/,[ ]*/\n/g' | tac ); do
+              echo "check torch_version_test $torch_version_test"
               set +e
               python3 -m pip install --dry-run torch==${torch_version_test}+cpu \
-                                     -f https://download.pytorch.org/whl/torch_stable.html > /dev/null 2>&1
+                                     -f https://download.pytorch.org/whl/torch_stable.html 2>&1
               search_stat=$?
+              echo "got search_stat $search_stat"
               set -e
               if [ $search_stat == 0 ]; then
+                  echo "got valid +cpu version, exiting"
                   torch_version=${torch_version_test}
                   break
               fi

[bernstei]

Error is apparently earlier than I thought. Can you add

echo "torch versions"
python3 -m pip install torch==
echo "torch versions to search"
python3 -m pip install torch== 2>&1 | fgrep 'from versions' | 
                               sed -e 's/.*from versions: //' -e 's/)//' -e 's/,[ ]*/\n/g' | tac
``

right after the `set +o pipefail` in the CI workflow `pytests.yml` that searches for torch? Before it does a loop over the output of that second command?

[bernstei]

Looks like a pip version error maybe? It used to respond with the list of available versions, now it just gives an error. Let me find out how to get that list, then I'll post a patch here for you to test.

[bernstei]

I found this https://stackoverflow.com/questions/4888027/how-to-list-all-available-package-versions-with-pip which mentions the package== approach breaking with pip 24. Working on another approach.

[bernstei]

OK, found a solution entirely separate from the pip package, since the suggested approach that works with pip 24 (pip index) is experimental. Try to replace the loop in that section with the following two lines (just watch the indentation)

         wget https://pypi.org/pypi/torch/json -O torch_versions
         for torch_version_test in $( python3 -c "import json; print(' '.join(json.load(open('torch_versions'))['releases'].keys()))" | sed 's/ /\n/g' | tac ); do

[JPDarby]

@bernstei That has done the trick for installing MACE but now the tests are failing to find pseudo potentials for quantum espresso. Any idea why this is happening? I'm a bit stumped because I haven't modified those tests

[bernstei]

I only see two failures. One is the change in the value of the reference value for the daisy-chained fit example, @gelzinyte and I dealt with, I thought, but I'll need to check the other PRs to figure out how - some python package version, I thought, but maybe that's still sitting in one of the other unmerged PRs?

The other, test_generic_DFT_autopara_defaults, is marked xfail because it supposedly needs to be updated to work with ASE 3.23. I'll look into that.

[bernstei]

The daisy-chain example is an rdkit version issue #324. Not sure how we ended up with it broken again. Any chance this PR is based on something other than the latest main branch? You'll need to do

git fetch origin
git merge origin/main

assuming origin is the name of the git remote that points to this github repo. If that leads to any updates, push your branch after doing this merge.

[bernstei]

Also, can you edit tests/calculators/test_calc_generic.py and replace, in def test_generic_DFT_autopara_defaults, replace the call to Espresso(calculator_exec= ...) with just Espresso(workdir=tmp_path) and remove the @pytest.mark.xfail that marks that test.

[bernstei]

Also, @JPDarby , can you merge from main, where we just merged some ASE fixes, to confirm that it's all compatible?

[bernstei]

@JPDarby just a reminder to merge from main so we can confirm that it's still working, and merge this one

[JPDarby]

@bernstei I have merged from main

[JPDarby]

daisy chain example is still failing, is the fix for that on main or a different branch?

[JPDarby]

This CI test used rdkit=2024.3.5 compared to 2024.3.3 in #324 and 2023.9.6 before. Looks like every change to rdkit changes the errors.

[JPDarby]

@bernstei fixing rdkit=2024.3.3 in the tests has done the job. Ok to merge?

If this isn't desirable long term maybe best to have a separate PR that removes the rdkit dependency?

[bernstei]

I'm going to merge, and create an issue for the rdkit version dependence.