Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] First draft of the API of WF generators for atomate v2.0. #1

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions atomate/abinit/firetasks/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
__author__ = 'David Waroquiers <[email protected]>'
Copy link

@fekad fekad Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's something we can use to simplify the importing of the relevant classes. I see a big advantage for the actual objects that users will use, e.g. in the atomate/abinit/__init__.py, we could specify the workflow generators so that the user can do:

from atomate.abinit import ScfWFGenerator

instead of

from atomate.abinit.workflows.core import ScfWFGenerator

Would you also do such definitions for "developer"-focused objects (a user of atomate is not likely to import a task) ? This is related to my comment about relative imports somehow. My point is that if we do that, when a developer reads such an import, it is not directly clear from where it is coming from. What do you think ?

11 changes: 11 additions & 0 deletions atomate/abinit/firetasks/common.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# coding: utf-8


"""
This module defines variables and functions for Abinit tasks.
"""


TMPDIR_NAME = "tmpdata"
OUTDIR_NAME = "outdata"
INDIR_NAME = "indata"
34 changes: 34 additions & 0 deletions atomate/abinit/firetasks/control_tasks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# coding: utf-8


"""
This module defines tasks that checks the proper execution of Abinit, and possibly takes appropriate measures to
correct errors or problems.
"""


from fireworks import explicit_serialize, FiretaskBase

from atomate.utils.utils import get_logger


__all__ = ("CheckTask", )


logger = get_logger(__name__)

__author__ = 'David Waroquiers'
__email__ = '[email protected]'


@explicit_serialize
class CheckTask(FiretaskBase):
"""
Check an Abinit Run and take appropriate corrections if errors occurred.
"""

required_params = []
optional_params = []

def run_task(self, fw_spec):
raise NotImplementedError
77 changes: 77 additions & 0 deletions atomate/abinit/firetasks/glue_tasks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
# coding: utf-8


"""
This module defines tasks that acts as a glue between other Abinit Firetasks to allow communication
between different Firetasks and Fireworks.
"""


from fireworks import explicit_serialize, FiretaskBase

from atomate.utils.utils import get_logger
from atomate.common.firetasks.glue_tasks import CopyFiles


__all__ = ("CopyAbinitOutputs",
"CreateAbinitFolders",
"ResolveDeps",)


logger = get_logger(__name__)

__author__ = 'David Waroquiers'
__email__ = '[email protected]'


@explicit_serialize
class CopyAbinitOutputs(CopyFiles):
"""
Copy files from one or multiple previous Abinit run directories to the current directory.

TODO: define what needs to be copied, what is required as inputs etc ... also think about a more general CopyFiles
TODO: is this task needed or do we put everything in a ResolveDeps task or the ResolveDeps task just defines what needs to be copied to a general CopyFiles task ?
"""

required_params = []
optional_params = []

def run_task(self, fw_spec):
raise NotImplementedError


@explicit_serialize
class CreateAbinitFolders(FiretaskBase):
"""
Firetask to create in, out and tmp folders for abinit.
TODO: is it worth to allow changing them ??
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a nice way to specify the default values of the optional_params list? I can only find fireworks where run_task has to go through and say e.g. self.get("in_dir", INDIR_NAME) which doesn't seem very scalable/reusable.

If it's clear what the defaults are then I don't see why you wouldn't allow people to change it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. This is currently not implemented in Fireworks as far as I know but it would be very beneficial. Strictly speaking, it's not part of this project but I propose we raise the point in fireworks itself. I'll contact Anubhav about it and maybe open an issue on the fireworks github.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to contribute something to Fireworks for this, would be good to be a bit hands-on so I can be more effective at reviewing here

Copy link

@gpetretto gpetretto Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this was a long standing point of discussion. Everything originates from the fact that the FireTask is basically a dict and dropping this entirely would be problematic for backward compatibility reasons. There is this PR to make the FireTasks become dataclasses in python which will be way better materialsproject/fireworks#393 . It is currently closed, I suppose since Alex did not have time to finalize it. We can propose to pick it up from where he left and complete it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to revisit that PR. I the last thing I need to do is convert some core Fireworks to the dataclass format and try running some atomate workflows. Having clear defaults and type hinting will be really useful.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI: I reopened the dataclass support PR in materialsproject/fireworks#418

TODO: see if this should be a subclass of a general CreateFolders Task ? (there is the CreateFolder task that creates just ONE folder and optionally moves to it)

Optional params:
in_dir(str): Directory for abinit input files (e.g. _DEN file from previous calculation).
out_dir(str): Directory for abinit output files (e.g. final _DEN file of the current calculation).
tmp_dir(str): Directory for temporary abinit files during calculation.
"""

required_params = []
optional_params = ["in_dir", "out_dir", "tmp_dir"]

def run_task(self, fw_spec):
raise NotImplementedError


@explicit_serialize
class ResolveDeps(FiretaskBase):
"""
Firetask resolve dependencies for abinit.

Should deal whether its a restart of a non converged calculation or a follow up calculation (e.g. nscf after scf).

TODO: define required and optional params
"""

required_params = []
optional_params = []

def run_task(self, fw_spec):
raise NotImplementedError
52 changes: 52 additions & 0 deletions atomate/abinit/firetasks/run_tasks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# coding: utf-8


"""
This module defines tasks to run Abinit or other related executables (e.g. anaddb, mrgddb, ...).
"""


from fireworks import explicit_serialize, FiretaskBase

from atomate.utils.utils import get_logger


__all__ = ("RunAbinit",
"RunAnaDdb",)


logger = get_logger(__name__)

__author__ = 'David Waroquiers'
__email__ = '[email protected]'



@explicit_serialize
class RunAbinit(FiretaskBase):
"""
Run Abinit.

TODO: define if/what are required/optional params
"""

required_params = []
optional_params = []

def run_task(self, fw_spec):
raise NotImplementedError


@explicit_serialize
class RunAnaDdb(FiretaskBase):
"""
Run anaddb.

TODO: define if/what are required/optional params
"""

required_params = []
optional_params = []

def run_task(self, fw_spec):
raise NotImplementedError
37 changes: 37 additions & 0 deletions atomate/abinit/firetasks/write_input_tasks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# coding: utf-8


"""
This module defines tasks to write inputs for Abinit or other related executables.
"""


from fireworks import explicit_serialize, FiretaskBase

from atomate.utils.utils import get_logger


__all__ = ("WriteInput",)


logger = get_logger(__name__)

__author__ = 'David Waroquiers'
__email__ = '[email protected]'



@explicit_serialize
class WriteInput(FiretaskBase):
"""
Write input for Abinit.

TODO: define if/what are required/optional params
"""

required_params = []
optional_params = []

def run_task(self, fw_spec):
raise NotImplementedError

1 change: 1 addition & 0 deletions atomate/abinit/fireworks/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
__author__ = 'David Waroquiers <[email protected]>'
44 changes: 44 additions & 0 deletions atomate/abinit/fireworks/core.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# coding: utf-8

"""
This module defines the function to generate Fireworks for abinit.
"""


from fireworks import Firework
from atomate.abinit.firetasks.glue_tasks import CreateAbinitFolders
from atomate.abinit.firetasks.glue_tasks import ResolveDeps
from atomate.abinit.firetasks.glue_tasks import CopyAbinitOutputs
from atomate.abinit.firetasks.write_input_tasks import WriteInput
from atomate.abinit.firetasks.run_tasks import RunAbinit
from atomate.abinit.firetasks.control_tasks import CheckTask
Comment on lines +9 to +14
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a little bit nicer if you use relative import path (https://realpython.com/absolute-vs-relative-python-imports/) like:

from ..firetasks.glue_tasks import CreateAbinitFolders
from ..firetasks.glue_tasks import ResolveDeps
from ..firetasks.glue_tasks import CopyAbinitOutputs
from ..firetasks.write_input_tasks import WriteInput
from ..firetasks.run_tasks import RunAbinit
from ..firetasks.control_tasks import CheckTask

sometimes the code might look cleaner when you import the module itself. Of course, you need to use prefixes but that is actually a good thing. You can immediately see from where the eg WriteInput (write_input_tasks.WriteInput) coming...

from ..firetasks import glue_tasks
from ..firetasks import write_input_tasks
from ..firetasks import run_tasks
from ..firetasks import control_tasks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a similar vein, it can be nice get into the practice of defining an __all__ for each submodule so that other imports in that submodule do not get mixed with the public API.

For the case of theatomate.abinit.fireworks.core submodule this would just be

__all__ = ("scf_fw", )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm usually not a fan of relative imports (its not always easy from where something is coming from) except when there are a lot of subsubsubpackages. I would say that we are somewhere in between here. I propose we see along the way what to do ? For the others, do you have a strong opinion on that ?

For the __all__, I fully agree.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I also prefer the absolute path for the imports. It was just an idea ... :)
You can still group them together but if you prefer this way that is completely fine as well...

Copy link
Collaborator Author

@davidwaroquiers davidwaroquiers Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on this. I propose to open an issue just to keep it in mind and in any case if we want to change, that's easy. For the __all__, I will add it to the different modules.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we just leave formatting up to black and isort --profile=black. This will automatically handle grouping etc.

from atomate.common.firetasks.glue_tasks import PassCalcLocs


__all__ = ("scf_fw", )


def scf_fw(structure):
"""Generate a Firework for a Self-Consistent Field (SCF) calculation.

Args:
structure: Pymatgen Structure object.
TODO: Add arguments needed here

Returns:
Firework: Firework object with all Firetasks needed for an SCF calculation.
"""

spec = {'structure': structure}
#TODO: only a very general structure is done here :) goal is to foster discussion on the overall architecture
tasks = []
tasks.append(CreateAbinitFolders())
tasks.append(ResolveDeps()) # Does it link/move/copy the files directly or is it just to define which files have to be copied where and then the next task does the work ?
tasks.append(CopyAbinitOutputs()) # Or a general CopyFiles task, what about moving or symbolic links ?
tasks.append(WriteInput())
tasks.append(RunAbinit())
tasks.append(CheckTask())
tasks.append(PassCalcLocs())

raise NotImplementedError
# return Firework(tasks=tasks, spec=spec)
fekad marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions atomate/abinit/workflows/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
__author__ = 'David Waroquiers <[email protected]>'
52 changes: 52 additions & 0 deletions atomate/abinit/workflows/core.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# coding: utf-8

"""
This module defines Workflow generators for abinit.
"""


from atomate.common.workflows.base import BaseStructureWFGenerator
from atomate.abinit.fireworks.core import scf_fw
from typing import Any
from typing import Dict
from typing import List
from typing import Optional
from typing import Union

from pymatgen.core.structure import Structure
from fireworks import Workflow


__all__ = ("ScfWFGenerator", )


class ScfWFGenerator(BaseStructureWFGenerator):
"""Workflow generator for Self-Consistent Field (SCF) calculations."""

def __init__(self):
"""Constructor for the SCF Workflow generator."""

raise NotImplementedError

def get_wf(self, structure: Optional[Structure] = None,
name: str = 'Structure Workflow',
prev_calc_locs: Optional[Union[List[str], str, bool]] = None,
metadata: Optional[Union[Dict["str", Any], bool]] = None,
name_tag: Optional[str] = None):

"""
Generate and return a SCF Workflow.

Args:
structure: Pymatgen Structure object to be used as an input for the Workflow.
name: Name of this workflow.
prev_calc_locs:
metadata:
name_tag:

Returns:
Fireworks' Workflow object
"""

fw = scf_fw(structure=structure)
return Workflow([fw])
1 change: 1 addition & 0 deletions atomate/common/workflows/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
__author__ = 'David Waroquiers <[email protected]>'
Loading