Update Calculators to work with ASE v3.23 Profiles

gelzinyte commented 5 months ago

In the the v3.23 release ASE has changed how the file-based calculators are interfaced. In general, that means there's less to do in wfl as users can provide an ase configuration file or a CalculatorProfile as a keyword argument. To stay backwards compatible, wfl can check if CalculatorProfile can be imported (or should it instead check if ASE's version is 3.22 or 3.23?) and default to the old behaviour.

Overview:

[x] Aims: #313
[x] Orca: ASE-3.22 was lightly broken, so I would only update to support the Profile update.
[x] Castep: It doesn't look like much has changed. @stenczelt would you be able to run the castep unit tests locally with the latest ASE release?
[x] Espresso: Similar update to Aims #325
[x] Mopac: No "Profile" changes, to be re-tested locally. #323
[x] Vasp: No "Profile" changes, @noambernstein could you test it with ASE v3.23 locally? #317

bernstei commented 5 months ago

I recently tested VASP, but not sure if it was pre or post 3.23. Let me make sure.

bernstei commented 5 months ago

@gelzinyte do we need calculator tests with various ways of specifying the executable (argument vs. env var vs. profile)?

bernstei commented 5 months ago

espresso is currently very messy. I'd love to find a way to simplify. Maybe we should make a more conscious decision of what to support.

bernstei commented 5 months ago

There is a small bug in vasp. command works, but calculator_exec does not. I have a fix. Should I add it to the existing aims PR?

gelzinyte commented 5 months ago

@gelzinyte do we need calculator tests with various ways of specifying the executable (argument vs. env var vs. profile)?

That would help with future ASE updates 🙃

There is a small bug in vasp. command works, but calculator_exec does not. I have a fix. Should I add it to the existing aims PR?

Let's keep different calculators' updates in separate PRs?

I'll move non-Aims discussion from #313 here:

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)?

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?

I think running jobs in separate directories introduces extra keywords (keep_files, rundir_prefix, workdir, scrachdir) without needing to mess with any of the ASE Calculators' keywords, no? I've never worked with VASP, is that different?

I would be up for leaving the keywords alone, to be worked out by the user. Should we remove any changes to the calculator kwargs? That would break some of wfl's current interface (e.g. calculator_exec wouldn't be used anymore). In ASE 3.22, as far as I can tell, most of messing with ase's kwargs was because ASE wasn't internally consistent, so wfl was allowing for "calculator_exec" and then were parsing that into the correct format (e.g. for orca command = claculator_exec + PREFIX.inp > PREFIX.out, but user can give command themselves). I'll update the checklist above with what the consequences of removing calculator_exec (or any other such updates) would be

bernstei commented 5 months ago

I think that we have few enough users that breaking the interface is OK. Maybe we can check for neither "command" nor "profile" in kwargs, and give a warning (or even an error, but that would break if ASE ever switches again) that we no longer modify kwargs and you need to figure out what works with your ASE version. We can even summarize the simplest way to get it to work in the warning message.

bernstei commented 5 months ago

@gelzinyte do you want to use the Aims PR as a test for this approach, or should we just drop that and do all of them in a single PR, simplifying by dropping the kwargs modification code.

gelzinyte commented 5 months ago

I think that we have few enough users that breaking the interface is OK. Maybe we can check for neither "command" nor "profile" in kwargs, and give a warning (or even an error, but that would break if ASE ever switches again) that we no longer modify kwargs and you need to figure out what works with your ASE version. We can even summarize the simplest way to get it to work in the warning message.

Sounds good. Note that neither command nor profile are necessary, because it seems that ase prefers a configuration file anyway and raises an error if it's not present. So I think even not raising a warning is ok, I'd simply add a note in version changes and update docs.

@gelzinyte do you want to use the Aims PR as a test for this approach, or should we just drop that and do all of them in a single PR, simplifying by dropping the kwargs modification code.

I'd rather not mix different calculator updates. I'll do Aims.

bernstei commented 5 months ago

I'd rather not mix different calculator updates. I'll do Aims.

you really want to do [edited: 6] (?) PRs? OK by me, but I'll make you set them all up :)

bernstei commented 5 months ago

Looks like the fact that we're using a random, latest ASE version is now breaking unrelated tests (e.g. #316). It'd be nice to fix this so we can specifically test with 3.23.0

jungsdao commented 5 months ago

I have modified espresso.py to make it compatible with new ASE version. It's not much of modification, just deleting a few lines from existing file. But I don't think it will work with previous version. I'll be happy to make PR or contribute in some way if this will help reducing overall workloads.

bernstei commented 4 months ago

I updated wfl.calculators.vasp to simplify in the way we decided (#317), but since Vasp can use a different executable for gamma-point only, there's still some of the kwargs mangling code left. When I run the tests locally, it fails some quantum espresso test (in test_remote_run.py), but I'm hoping that once those are xfailed as part of the Aims PR #313 and merged into this branch, all the tests will pass.

bernstei commented 3 months ago

I updated the espresso checklist item

stenczelt commented 3 months ago

@bernstei / @gelzinyte bad news:

running the castep tests gets into some

pytest tests/calculators/test_castep.py

Processed: [####################] 100%

tests/calculators/test_castep.py:15 (test_castep_calculation)
self = -----------------Atoms--------------------
Atoms(symbols='Al2', pbc=True, cell=[4.05, 4.05, 4.05])
-----------------Pa...ue
          _set_atoms : False
       _track_output : False
          _try_reuse : False
           _pedantic : False

atoms = Atoms(symbols='Al2', pbc=True, cell=[4.05, 4.05, 4.05], calculator=Castep(...))
properties = ['energy']
system_changes = ['positions', 'numbers', 'cell', 'pbc', 'initial_charges', 'initial_magmoms']

    def calculate(self, atoms=None, properties=_default_properties, system_changes=all_changes):
        """Do the calculation. Handles the working directories in addition to regular
        ASE calculation operations (writing input, executing, reading_results)
        Reimplements & extends GenericFileIOCalculator.calculate() for the development version of ASE
        or FileIOCalculator.calculate() for the v3.22.1"""

        if atoms is not None:
            self.atoms = atoms.copy()

        # this may modify self.parameters, <...>_ _ _ _ _ _ _ _ 
../../venv/lib/python3.11/site-packages/ase/atoms.py:755: in get_potential_energy
    energy = self._calc.get_potential_energy(self)
../../venv/lib/python3.11/site-packages/ase/calculators/abc.py:24: in get_potential_energy
    return self.get_property(name, atoms)
../../venv/lib/python3.11/site-packages/ase/calculators/calculator.py:538: in get_property
    self.calculate(atoms, [name], system_changes)
../../wfl/calculators/castep.py:99: in calculate
    result = self.get_property(property)
../../venv/lib/python3.11/site-packages/ase/calculators/calculator.py:538: in get_property
    self.calculate(atoms, [name], system_changes)
../../wfl/calculators/castep.py:99: in calculate
    result = self.get_property(property)
../../venv/lib/python3.11/site-packages/ase/calculators/calculator.py:538: in get_property
    self.calculate(atoms, [name], system_changes)
E   RecursionError: maximum recursion depth exceeded in comparison
!!! Recursion detected (same locals & position)

Though when running with a debugger and stepping line by line, I am getting the recursion error here:

 File "/Users/tks32/research/tmp-wfl-tests/workflow/venv/lib/python3.11/site-packages/ase/calculators/calculator.py", line 538, in get_property
    self.calculate(atoms, [name], system_changes)
  File "/Users/tks32/research/tmp-wfl-tests/workflow/wfl/calculators/castep.py", line 99, in calculate
    result = self.get_property(property)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tks32/research/tmp-wfl-tests/workflow/venv/lib/python3.11/site-packages/ase/calculators/calculator.py", line 538, in get_property
    self.calculate(atoms, [name], system_changes)
  File "/Users/tks32/research/tmp-wfl-tests/workflow/wfl/calculators/castep.py", line 76, in calculate
    self.atoms = atoms.copy()
                 ^^^^^^^^^^^^
  File "/Users/tks32/research/tmp-wfl-tests/workflow/venv/lib/python3.11/site-packages/ase/atoms.py", line 931, in copy
    atoms = self.__class__(cell=self.cell, pbc=self.pbc, info=self.info,
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tks32/research/tmp-wfl-tests/workflow/venv/lib/python3.11/site-packages/ase/atoms.py", line 215, in __init__
    self.set_cell(cell)
  File "/Users/tks32/research/tmp-wfl-tests/workflow/venv/lib/python3.11/site-packages/ase/atoms.py", line 391, in set_cell
    cell = Cell.new(cell)
           ^^^^^^^^^^^^^^
  File "/Users/tks32/research/tmp-wfl-tests/workflow/venv/lib/python3.11/site-packages/ase/cell.py", line 86, in new
    cellobj = cls(cell)
              ^^^^^^^^^
  File "/Users/tks32/research/tmp-wfl-tests/workflow/venv/lib/python3.11/site-packages/ase/cell.py", line 33, in __init__
    assert array.shape == (3, 3)
           ^^^^^^^^^^^^^^^^^^^^^
RecursionError: maximum recursion depth exceeded in comparison

Which looks more like an upstream problem, not being able to copy an Atoms object for some reason.

gelzinyte commented 3 months ago

Thanks for checking! I've meanwhile set up castep myself (it turns out I don't need a personal license) and fixed this recursion (there's a functional allow_calculation=False in ase.3.23). I'll open a full PR soon

libAtoms / workflow

Update Calculators to work with ASE v3.23 Profiles #314