aiidaplugins / aiida-ase

AiiDA plugin for ASE
MIT License
5 stars 9 forks source link

Migrate `AseCalculation` and `AseParser` to `aiida-core==1.0.0` #4

Closed sphuber closed 3 years ago

lxknll commented 3 years ago

Dear sphuber, Dear aiida-core team,

I would like to inquire about the status of the migration of aiida-ase to aiida-core>=1.0.0 . My goal is to run GPAW calculations within an aiida workchain. However, in its current state, aiida-ase does not properly load anymore with the latest aiida-core version.

Perhaps I am missing an aiida feature which replaced this plugin altogether? If so, I apologize in advance!

Otherwise, I am also happy to contribute to this issue. The necessary changes seem to be limited to renaming ParameterData to Dict and the adjustment of aiida.orm.calculation.job etc.

Thank you very much! Best Alex

lxknll commented 3 years ago

Dear Sebastiaan @sphuber I realize that this issue has been stale for quite some time now, so I hope I do not seem too impatient commenting again already. I would like to know whether the issue is still relevant. Maybe there has been a new development in aiida since Oct 2019 which entirely replaced aiida-ase?

Thanks for your reply, I appreciate it! Best Alex

sphuber commented 3 years ago

Hi Alex. Sorry for the delayed response. I have never used the aiida-ase plugin myself, I just moved it out of the aiida-core source code a few years back when plugins were still hard coded into the source code (there was no actual plugin system yet). I am a but surprised this was still open because I was under the impression that some people were using ASE with AiiDA. I am trying to find out who know in our private communication channels to see who that is (don't actually remember any names). They may have already performed the necessary work but just didn't commit it back to this public plugin. If that is not the case, we can indeed migrate this. The migration from AiiDA v0.x to v1.x was not trivial for plugins and required quite some work. Given that this plugin "only" has a single calculation and parser plugin, the amount of work should be reasonable. I can have a look later to see if I can give a quick shot, but I could use your help in testing since there are no unit tests whatsoever and I don't have any experience with running ASE. I will get back to you

lxknll commented 3 years ago

Hi Sebastiaan, thank you very much for the reply! It would be awesome if this plugin worked with AiiDA v1.x . Of course I am happy to run tests, let me know in which way I can contribute.

sphuber commented 3 years ago

@lxknll as you can see, I have merged a PR that migrates the plugin and does a bunch of different stuff that should make it work with Python 3 and AiiDA v1.0 and above. Could you please check out the develop branch and install that. I have also opened a PR #8 that updates the examples. You can maybe try to run that:

#!/usr/bin/env runaiida
# -*- coding: utf-8 -*-
"""Example script of how to perform a GPAW calculation on crystalline silicon using AiiDA."""
from aiida import common
from aiida import engine
from aiida import orm
from aiida import plugins
from ase.build import bulk

Dict = plugins.DataFactory('dict')
StructureData = plugins.DataFactory('structure')
KpointsData = plugins.DataFactory('array.kpoints')
AseCalculation = plugins.CalculationFactory('ase.ase')

# Change the following value to the ``Code`` that you have configured
code = 'bash@localhost'

def main():
    kpoints = KpointsData()
    kpoints.set_kpoints_mesh([2, 2, 2])

    parameters = {
        'calculator': {
            'name': 'gpaw',
            'args': {
                'mode': {
                    '@function': 'PW',
                    'args': {
                        'ecut': 300
                    }
                }
            }
        },
    }

    inputs = {
        'code': orm.load_code(code),
        'structure': StructureData(ase=bulk('Si')),
        'kpoints': kpoints,
        'parameters': Dict(dict=parameters),
        'metadata': {
            'label': 'Si with GPAW',
            'description': 'Test calculation of GPAW using `aiida-ase`.',
            'options': {
                'resources': {
                    'num_machines': 1,
                    'num_mpiprocs_per_machine': 1
                },
                'max_wallclock_seconds': 30 * 60,  # 30 minutes
            }
        }
    }

    process_name = AseCalculation.__name__
    print(f'Running an {process_name}...')
    _, node = engine.run_get_node(AseCalculation, **inputs)
    outputs = node.get_outgoing(link_type=(common.LinkType.CREATE, common.LinkType.RETURN)).all()

    if node.is_finished and node.exit_message:
        state = f'{node.process_state.value} [{node.exit_status}] `{node.exit_message}`'
    elif node.is_finished:
        state = f'{node.process_state.value} [{node.exit_status}]'
    else:
        state = node.process_state.value

    print(f'{process_name}<{node.pk}> terminated with state: {state}')

    if not outputs:
        print(f'{process_name}<{node.pk}> registered no outputs')
        return

    print(f"\n{'Output link':25s} Node pk and type")
    print(f"{'-' * 60}")

    for triple in sorted(outputs, key=lambda triple: triple.link_label):
        print(f'{triple.link_label:25s} {triple.node.__class__.__name__}<{triple.node.pk}> ')

if __name__ == '__main__':
    main()

Just make sure to update the code variable to an actual Code that you setup on your machine. Let me know how that goes and if you experience issues, feel free to open an issue.

lxknll commented 3 years ago

Hi @sphuber thank you so much for putting in the work! I will give it a try over the weekend and let you know how it goes.

lxknll commented 3 years ago

Hi @sphuber! I have now tested the new version of aiida-ase with your script and several custom GPAW calculations on local machines and remote clusters. Everything seems to be working perfectly! :muscle:

I have one suggestion: perhaps you could update the example script you provided to me (and which is very similar to the one in the official aiida-ase documentation) to make use of the builder workflow encouraged by aiida-core?

#!/usr/bin/env runaiida                                                                                                                     
# -*- coding: utf-8 -*-                                                                                                                     
"""Example script of how to perform a GPAW calculation on crystalline silicon using AiiDA."""
from aiida.engine import submit, run_get_node
from aiida.orm import load_code
from aiida.plugins import DataFactory, CalculationFactory

# ASE imports for structure building.                                                                                                       
from ase.build import bulk

Dict = DataFactory('dict')
StructureData = DataFactory('structure')
KpointsData = DataFactory('array.kpoints')
AseCalculation = CalculationFactory('ase.ase')

# Change the following value to the ``Code`` that you have configured                                                                       
codename = 'codename'

def submit_gpaw():
    kpoints = KpointsData()
    kpoints.set_kpoints_mesh([2, 2, 2])

    parameters = {
        'calculator': {
            'name': 'gpaw',
            'args': {
                'mode': {
                    '@function': 'PW',
                    'args': {
                        'ecut': 300
                    }
                }
            }
        }
    }

    metadata = {
        'label': 'Si with GPAW',
        'description': 'Test calculation of GPAW using `aiida-ase`.',
        'options': {
            'resources': {
                'num_machines': 1,
                'num_mpiprocs_per_machine': 1
            },
            'max_wallclock_seconds': 30 * 60,  # 30 minutes                                                                                 
        }
    }

    code = load_code(codename)
    builder = code.get_builder()
    builder.structure = StructureData(ase=bulk('Si'))
    builder.kpoints = kpoints
    builder.parameters = Dict(dict=parameters)
    builder.metadata = metadata

    run_get_node(builder)

if __name__ == '__main__':
    submit_gpaw()

I feel like this is a little more concise. Also, it might make it easier for beginners when the documentations of aiida-core and aiida-ase make use of the same features.

In any case, thank you so much for your effort and striving to improve aiida. I really appreciate it. Let me know if there is anything else I can help you with :)

Best wishes Alex

sphuber commented 3 years ago

Thanks for testing Alex. Glad it is working now and it is useful. A few comments about your suggestion to use the builder instead. The dictionary and builder methods are completely equivalent and aiida-core doesn't recommend one over the other. The dictionary method is the simplest and most basic way of specifying inputs for a process. The builder concept was added later as an additional feature that has useful features when used interactively, for example, tab-completion, automatic validation when setting an input and automated documentation. This is described in the documentation. These features are not really useful when used non-interactively, but we wanted to make sure that what works in a shell also works in scripts, so you can still use the builder if you like. Ultimately, both methods are perfectly equivalent and fine to use, but I think for example scripts the dictionary use is simpler for new users since it doesn't introduce a new concept that they might not be familiar with yet.

Another minor thing: I would also not recommend to get the builder from the Code. The reason is that this required the default plugin to be set on the Code, but since this is optional, this approach does not always work. That is why I prefer to be explicit and get it from the process class. This is what Code.get_builder does anyway, it is just syntactic sugar.

Finally, I noticed you removed the code that I added after the submission of the process. Did you do this on purpose because you find the additional code confusing? I can see why because there are quite some lines, but I did this since this will give some interesting information while the process is running and after it has finished. In your example, there will be no output of how to inspect the process or its results. Since these examples might be used primarily by people with very little experience with AiiDA, I thought it might be useful to include some verbose output of what happened.

lxknll commented 3 years ago

Hi Matthew! Thank you for your explanations :) I did not want to insinuate that I know the aiida documentation better than you do, of course not, and I am sorry if I did! As a new user, I just felt it might be more homogeneous if both the aiida-core and the aiida-ase documentation used the builder method in their introductory examples. At least for me, the aiida documentation introduces the builder first, so one gets the feeling that this is the preferred way to do things. Nevertheless, I fully agree that it is best to circumvent unnecessary syntactic sugar and every user will be able to understand a python dictionary.

Concerning your final question: I did not find the additional code confusing, actually the extra information is quite interesting. I only removed it for my own application and forgot to add it back before submitting my comment here :)