Process: refactor out input handling specific to CalcJob
#5539
Conversation
The `CalcJob` subclass of `Process` adds inputs that are specific to its implementation, namely `code`, `metadata.computer` and `metadata.options. Nevertheless, these were being handled in the `Process` class making assumptions about the exact type of the `code` input. The code is refactored to move the `CalcJob` specific logic to its implementation. The signature of `_setup_metadata` had to be changed to pass in a copy of the metadata inputs as the subclass needs to pop its custom keys such that `Process` won't raise on keys it doesn't recognize. The `_setup_inputs` implementation on `CalcJob` is simplified as it now just needs to set the `Computer` of the `Code` if one hasn't already been set in the `_setup_metadata` which is called before it.
| """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), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
aiida-core/aiida/engine/processes/calcjobs/calcjob.py
Lines 57 to 63 in 9d903ca
If a remote_folder is defined, it is an import job and so the code is not required.
| @@ -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))) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The CalcJob, for example, defines the computer and options keys. It needs to be able to pop those, otherwise this line
aiida-core/aiida/engine/processes/process.py
Line 709 in 9d903ca
will raise in the
Process._setup_metadata step.
| # 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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Okay, nevermind. It is sure about the order in _setup_db_record
There was a problem hiding this comment.
Yes, you are right though, if we were to change that in Process._setup_db_record it would fail.
Fixes #5537
The
CalcJobsubclass ofProcessadds inputs that are specific to itsimplementation, namely
code,metadata.computerandmetadata.options.Nevertheless, these were being handled in the
Processclass makingassumptions about the exact type of the
codeinput.The code is refactored to move the
CalcJobspecific logic to itsimplementation. The signature of
_setup_metadatahad to be changed topass in a copy of the metadata inputs as the subclass needs to pop its
custom keys such that
Processwon't raise on keys it doesn't recognize.The
_setup_inputsimplementation onCalcJobis simplified as it nowjust needs to set the
Computerof theCodeif one hasn't alreadybeen set in the
_setup_metadatawhich is called before it.