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

Process: refactor out input handling specific to CalcJob #5539

Merged
merged 2 commits into from
May 25, 2022
Merged
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
23 changes: 23 additions & 0 deletions aiida/engine/processes/calcjobs/calcjob.py
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,29 @@ def prepare_for_submission(self, folder: Folder) -> CalcInfo:
"""
raise NotImplementedError()

def _setup_metadata(self, metadata: dict) -> None:
"""Store the metadata on the ProcessNode."""
computer = metadata.pop('computer', None)
if computer is not None:
self.node.computer = computer

options = metadata.pop('options', {})
for option_name, option_value in options.items():
self.node.set_option(option_name, option_value)

super()._setup_metadata(metadata)

def _setup_inputs(self) -> None:
"""Create the links between the input nodes and the ProcessNode that represents this process."""
super()._setup_inputs()

# If a computer has not yet been set, which should have been done in ``_setup_metadata`` if it was specified
# in the ``metadata`` inputs, set the computer associated with the ``code`` input. Note that not all ``code``s
# will have an associated computer, but in that case the ``computer`` property should return ``None`` and
# nothing would change anyway.
if not self.node.computer:
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking is there any possibility that _setup_inputs is called before _setup_metadata? then the computer is not set yet and will cause a problem?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, nevermind. It is sure about the order in _setup_db_record

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right though, if we were to change that in Process._setup_db_record it would fail.

self.node.computer = self.inputs.code.computer # type: ignore[union-attr]

def _perform_dry_run(self):
"""Perform a dry run.

Expand Down
20 changes: 6 additions & 14 deletions aiida/engine/processes/process.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import asyncio
import collections
from collections.abc import Mapping
import copy
import enum
import inspect
import logging
Expand Down Expand Up @@ -684,27 +685,22 @@ def _setup_db_record(self) -> None:
elif isinstance(self.node, orm.WorkflowNode):
self.node.base.links.add_incoming(parent_calc, LinkType.CALL_WORK, self.metadata.call_link_label)

self._setup_metadata()
self._setup_metadata(copy.copy(dict(self.inputs.metadata)))
Copy link
Member

Choose a reason for hiding this comment

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

I do not quite get the idea of using copy here. Could you give me an example of a subclass that has a custom key, what issue will happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CalcJob, for example, defines the computer and options keys. It needs to be able to pop those, otherwise this line

raise RuntimeError(f'unsupported metadata key: {name}')

will raise in the Process._setup_metadata step.

self._setup_inputs()

def _setup_metadata(self) -> None:
def _setup_metadata(self, metadata: dict) -> None:
"""Store the metadata on the ProcessNode."""
version_info = self.runner.plugin_version_provider.get_version_info(self.__class__)
self.node.base.attributes.set_many(version_info)

for name, metadata in self.metadata.items():
for name, value in metadata.items():
if name in ['store_provenance', 'dry_run', 'call_link_label']:
continue

if name == 'label':
self.node.label = metadata
self.node.label = value
elif name == 'description':
self.node.description = metadata
elif name == 'computer':
self.node.computer = metadata
elif name == 'options':
for option_name, option_value in metadata.items():
self.node.set_option(option_name, option_value)
self.node.description = value
else:
raise RuntimeError(f'unsupported metadata key: {name}')

Expand All @@ -716,10 +712,6 @@ def _setup_inputs(self) -> None:
if node is None:
continue

# Special exception: set computer if node is a remote Code and our node does not yet have a computer set
if isinstance(node, orm.InstalledCode) and not self.node.computer:
self.node.computer = node.computer

# Need this special case for tests that use ProcessNodes as classes
if isinstance(self.node, orm.CalculationNode):
self.node.base.links.add_incoming(node, LinkType.INPUT_CALC, name)
Expand Down
3 changes: 2 additions & 1 deletion tests/engine/processes/calcjobs/test_calc_job.py
Original file line number Diff line number Diff line change
Expand Up @@ -874,11 +874,12 @@ class TestImport:
"""Test the functionality to import existing calculations completed outside of AiiDA."""

@pytest.fixture(autouse=True)
def init_profile(self, aiida_profile_clean, aiida_localhost): # pylint: disable=unused-argument
def init_profile(self, aiida_profile_clean, aiida_localhost, aiida_local_code_factory): # pylint: disable=unused-argument
"""Initialize the profile."""
# pylint: disable=attribute-defined-outside-init
self.computer = aiida_localhost
self.inputs = {
'code': aiida_local_code_factory('core.arithmetic.add', '/bin/bash', computer=aiida_localhost),
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity. I think the code is mandatory input for calcjob. This is not in the original code, but why did that not fail the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is in fact not required in the port. For normal executions it is sort of required because the validator will make sure it is defined. However, there is an exception for imported jobs. See these lines

remote_folder = inputs.get('remote_folder', None)
if remote_folder is not None:
# The `remote_folder` input has been specified and so this concerns an import run, which means that neither
# a `Code` nor a `Computer` are required. However, they are allowed to be specified but will not be explicitly
# checked for consistency.
return None

If a remote_folder is defined, it is an import job and so the code is not required.

'x': orm.Int(1),
'y': orm.Int(2),
'metadata': {
Expand Down