search

siesta-project / aiida_siesta_plugin

Source code for the AiiDA-Siesta package (plugin and workflows). See wiki
Other
6 stars 11 forks source link

Subclass from "Restart" abstract Workchain #27

Closed albgar closed 4 years ago

albgar commented 5 years ago

Sebastiaan Huber has created a "BaseRestartWorkChain" for QuantumEspresso (see aiida-quantumespresso/aiida_quantumespresso/common/workchain/base/restart.py).

This class has the basic constructs to (re)run a calculation depending on various error conditions, by implementing a framework of error handling.

Our current SiestaBaseWorkchain reuses a lot of that code internally. Would it be better/more clear to make it inherit from a "BaseRestartWorkChain"? This would be so if we need this abstract restart functionality in other workflows.

The VASP plugin is also taking this route.

vdikan commented 5 years ago

In the end, that's exactly what I did: while replicating the style of aiida_quantumespresso/workflows/pw/base.py I gradually took the chunks from aiida_quantumespresso/common/workchain/base/restart.py and its imports.

Instead, I could indeed reuse these modules from aiida_quantumespresso:

All these are abstract enough to allow us to build our own analogs of aiida_quantumespresso/workflows/pw/base.py. Makes the plugin dependent on aiida_quantumespresso though, but that does not seem a big deal.

bosonie commented 5 years ago

I don't think is a good idea to make our plugin dependent on aiida_quantumespresso. Every plugin should stand alone. I would rather copy the entire file and maybe write something about the authorship in the file AUTHORS.txt in case we need to really reuse aiida_quantumespresso code. However I don't think this is what @albgar meant. I think he meant that in vasp and quantumespresso plagins we have in a separate file (restart.py) the basic constructs to analyze errors and restart calculations, while we have them directly in the SiestaBaseWorkchain. Correct? I think for us to have a separate file dealing with generic restart instructions makes less sense because we just have the siesta executable migrated to 1.0 at the moment (while qe and vasp has many! pw,ph,q2r,matdyn ..), but in the future could be handy...

sphuber commented 4 years ago

Just stumbled on this and was going to tell that the BaseRestartWorkChain has now been integrated in aiida-core but I see you already migrated the code to use it, so I think his can be closed no?