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

KeyError: 'CONTROL' #970

Open
alinelena opened this issue Oct 2, 2023 · 7 comments
Open

KeyError: 'CONTROL' #970

alinelena opened this issue Oct 2, 2023 · 7 comments

Comments

@alinelena
Copy link

I am trying a test installation of aiida with qe espresson (7.0,7.1 soon 7.2) and I am getting the error above, full trace is below. (I did not have issues in the past)

the script I test with is also below.

calculation itself runs correctly

# -*- coding: utf-8 -*-
###############################################################################
"""Run simple DFT calculation."""

import os
import sys
import click

from aiida.common import NotExistent
from aiida.engine import run
from aiida.orm import Dict, KpointsData, StructureData, load_code, load_group
from ase.build import bulk


StructureData = DataFactory('structure')  # pylint: disable=invalid-name


def example_dft(qe_code):
    """Run simple DFT calculation."""

    thisdir = os.path.dirname(os.path.realpath(__file__))
    # Structure.
    structure = StructureData(ase=bulk('Si', 'fcc', 5.43))
    pseudo_family = load_group('SSSP/1.3/PBE/efficiency')
    pseudos = pseudo_family.get_pseudos(structure=structure)
    cutoff_wfc, cutoff_rho = pseudo_family.get_recommended_cutoffs(
    structure=structure,
    unit='Ry'
)
    parameters = Dict({
        'control': {
            'calculation': 'scf',
             'verbosity': 'high'
        },
        'system': {
            'ecutwfc': cutoff_wfc,  # wave function cutoff in Ry
            'ecutrho': cutoff_rho,  # density cutoff in Ry
        },
    })
    kpoints = KpointsData()
    kpoints.set_kpoints_mesh([4,4,4])

    # Construct process builder.
    builder = qe_code.get_builder()
    builder.structure = structure
    builder.code = qe_code
    builder.pseudos = pseudos
    builder.kpoints = kpoints
    builder.parameters = parameters
    builder.metadata.options.resources = {
        "num_machines": 1
    }
    builder.metadata.options.max_wallclock_seconds = 1 * 3 * 60

    print("Submitted calculation...")
    results, node = run.get_node(builder)

    print(f"node status: {node.exit_status()}")

@click.command('cli')
@click.argument('codelabel')
def cli(codelabel):
    """Click interface."""
    try:
        code = Code.get_from_string(codelabel)
    except NotExistent:
        print(f"The code '{codelabel}' does not exist.")
        sys.exit(1)
    example_dft(code)


if __name__ == '__main__':
    cli()  # pylint: disable=no-value-for-parameter
Submitted calculation...
Report: [244|PwCalculation|on_except]: Traceback (most recent call last):
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/plumpy/process_states.py", line 228, in execute
    result = self.run_fn(*self.args, **self.kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida/engine/processes/calcjobs/calcjob.py", line 678, in parse
    exit_code_retrieved = self.parse_retrieved_output(retrieved_temporary_folder)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida/engine/processes/calcjobs/calcjob.py", line 789, in parse_retrieved_output
    exit_code = parser.parse(**parse_kwargs)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida_quantumespresso/parsers/pw.py", line 71, in parse
    structure = self.build_output_structure(parsed_structure)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida_quantumespresso/parsers/pw.py", line 431, in build_output_structure
    type_calc = self.node.inputs.parameters.get_dict()['CONTROL']['calculation']
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
KeyError: 'CONTROL'

Traceback (most recent call last):
  File "/home/drFaustroll/micromamba/envs/aiida/bin/verdi", line 10, in <module>
    sys.exit(verdi())
             ^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida/cmdline/utils/decorators.py", line 73, in wrapper
    return wrapped(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida/cmdline/commands/cmd_run.py", line 115, in run
    exec(compile(handle.read(), str(filepath), 'exec', dont_inherit=True), globals_dict)  # pylint: disable=exec-used
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "test_qe_block.py", line 73, in <module>
    cli()  # pylint: disable=no-value-for-parameter
    ^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "test_qe_block.py", line 69, in cli
    example_dft(code)
  File "test_qe_block.py", line 56, in example_dft
    results, node = run.get_node(builder)
                    ^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida/engine/launch.py", line 60, in run_get_node
    return runner.run_get_node(process, *args, **inputs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida/engine/runners.py", line 277, in run_get_node
    result, node = self._run(process, *args, **inputs)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida/engine/runners.py", line 249, in _run
    process_inited.execute()
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/plumpy/processes.py", line 86, in func_wrapper
    return func(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/plumpy/processes.py", line 1197, in execute
    return self.future().result()
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/plumpy/process_states.py", line 228, in execute
    result = self.run_fn(*self.args, **self.kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida/engine/processes/calcjobs/calcjob.py", line 678, in parse
    exit_code_retrieved = self.parse_retrieved_output(retrieved_temporary_folder)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida/engine/processes/calcjobs/calcjob.py", line 789, in parse_retrieved_output
    exit_code = parser.parse(**parse_kwargs)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida_quantumespresso/parsers/pw.py", line 71, in parse
    structure = self.build_output_structure(parsed_structure)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/drFaustroll/micromamba/envs/aiida/lib/python3.11/site-packages/aiida_quantumespresso/parsers/pw.py", line 431, in build_output_structure
    type_calc = self.node.inputs.parameters.get_dict()['CONTROL']['calculation']
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
KeyError: 'CONTROL'

@sphuber
Copy link
Contributor

sphuber commented Oct 2, 2023

Thanks for the report. Could you please try to run the exact same script, but change the top-level keys in the parameters dictionary to all-caps, i.e.:

    parameters = Dict({
        'CONTROL': {
            'calculation': 'scf',
             'verbosity': 'high'
        },
        'SYSTEM': {
            'ecutwfc': cutoff_wfc,  # wave function cutoff in Ry
            'ecutrho': cutoff_rho,  # density cutoff in Ry
        },
    })

Notice that control and system are now capitalized. I think it should then work. In the meantime, I will work on a fix.

@alinelena
Copy link
Author

hi Sebastiaan,
indeed that is the issue. odd that one went back to f77 mindset, previous version were case insensitive.

@sphuber
Copy link
Contributor

sphuber commented Oct 3, 2023

I am a bit surprised that you mention that this used to work for you. I did some testing and this behavior goes back to at least v3.3.0. The problem is that the PwCalculation class automatically converts the top-level keys in the parameters input to all-caps, and so in your case doesn't complain. However, the PwParser does not do this and checks for the capitalized keys, and so if it is not there, will raise a KeyError.

I noted this problematic behavior already 5 years ago: #121
It is somewhat surprising that a user hasn't encountered this before yet, or at least, hasn't reported it. @mbercx maybe for v5.0 we can finally bite the bullet and enforce a single consistent casing naming rule and stick with it?

@mbercx
Copy link
Member

mbercx commented Nov 19, 2023

@sphuber I agree that we need to make sure the casing is consistent. Else anywhere you check an input parameter or setting (not just in PwParser, but also anywhere in a work chain, also in other packages) you have to first convert the dictionary into one where you know the casing. I also thought about trying some kind of case-insensitive dict, but that is probably a bad idea. It's much easier to just enforce the casing than having to deal with this everywhere in the code base (e.g. also inputs/overrides from the protocols - the more I think of it the more convinced I am ^^). And I think we can write a pretty clear error message to users so they can fix it before running a calculation with some validation.

One idea I was still toying with was to "fix" the casing using a serializer:

import warnings
from aiida.engine import WorkChain

from aiida_quantumespresso.calculations import _uppercase_dict as uppercase_dict

class MyWorkChain(WorkChain):
    @classmethod
    def define(cls, spec):
        super().define(spec)
        
        # Define the inputs
        spec.input(
            'input_parameters',
            valid_type=orm.Dict,
            required=True,
            serializer=cls.serialize_input_parameters,
        )
        
        # Define the outline of the work chain
        spec.outline(
            cls.report_input,

        )

    def report_input(self):
        self.report(f"Input parameters: {self.inputs.input_parameters.get_dict()}")

    @staticmethod
    def serialize_input_parameters(value):
        """Validate the input parameters."""
        input_parameters = value.get_dict()

        has_lowercase = any(any(char.islower() for char in key) for key in input_parameters.keys())

        if has_lowercase:
            warnings.warn("At least one top-level key has a lowercase character. Setting to uppercase.")

        return orm.Dict(dict=uppercase_dict(input_parameters, 'parameters'))

But I quickly reliased that serializers only act upon non-Data types, indeed:

https://github.com/aiidateam/aiida-core/blob/d3788adea220107bce3582d246bcc9674b5e1571/aiida/engine/processes/ports.py#L103-L123

Is there a feature for updating an input port that I'm now aware of? Would it make sense to allow this for the current serializer input?

My thinking is that we can enforce the casing on the fly, warning the user that we made these changes. At least @unkcpz seems to also think this is the way to go. ^^

EDIT: Also: I don't think we have to wait for v5.0 to enforce this, since running with lowercase will most likely not work anywhere?

@sphuber
Copy link
Contributor

sphuber commented Nov 19, 2023

But I quickly reliased that serializers only act upon non-Data types, indeed:
https://github.com/aiidateam/aiida-core/blob/d3788adea220107bce3582d246bcc9674b5e1571/aiida/engine/processes/ports.py#L103-L123
Is there a feature for updating an input port that I'm now aware of?

No, the serializers would be it.

Would it make sense to allow this for the current serializer input?

Perhaps. The request has been made before to make it act on non-Data inputs as well: aiidateam/aiida-core#5342
My response there kind of applies here as well. The serializer can still be used, users simply have to pass plain dicts. This is actually even easier for the user as well. It is just that currently they are used to having to pass explicit Dict nodes. I think ideally all ports that accept Dict nodes automatically convert normal dicts such that users become used to this. Then you can add the logic you describe in the serializer.

One concrete downside of allowing Data nodes to also be modified by serializers is that this may lead to unexpected behavior (and somewhat a loss of "explicit" provenance). If you pass in a (stored) node, the serializer could now decide to change it and actually return another node. This modification would not be captured. Not a problem per se, but could be if the user doesn't expect it. Maybe as a "rule" it would be better therefore to say that serializers will never modify existing nodes. But I can see arguments made for both sides

@mbercx
Copy link
Member

mbercx commented Nov 20, 2023

Thanks @sphuber, you make some valid points. I agree that adapting the serializer would muddy the concept of what a serializer is supposed to do and potentially open a Pandora's box of unexpected behaviour. I'd like to think I'd handle this power responsibly (warn the user, perhaps raise an error if the node is stored etc) but who knows what those other naughty plugin developers will do. ^^ In the end I don't think it's worth adapting the serializer for this use case.

Instead let's:

Side note: Part of me likes the idea of having serializers and users just passing in regular Python types (or e.g. a pymatgen Structure). Seems more user-friendly. Part of me also worries that users might get less used to the concept of nodes as inputs and outputs and then be confused again after obtaining the outputs, but that's life. ^^

@sphuber
Copy link
Contributor

sphuber commented Nov 20, 2023

Add serializers where appropriate.

The idea has been floated to add the to_aiida_type serializer` as the default serializer for input ports. This might be interesting to look at instead of starting to manually add it to all ports in our process specs.

Seems more user-friendly. Part of me also worries that users might get less used to the concept of nodes as inputs and outputs and then be confused again after obtaining the outputs, but that's life. ^^

This was the very reason that we originally decided against this. Also for not adding this automatic serialization to process functions. However, as of v2.3 we actually added the to_aiida_type as the default serializer for process function arguments. Maybe its best to actually go all the way and apply this to all processes.

Add validators to enforce the established casing conventions.

And what are those conventions? 😄 Upper- or lowercase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants