-
Notifications
You must be signed in to change notification settings - Fork 192
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
Redesign the Parser class #2397
Redesign the Parser class #2397
Conversation
.ci/test_daemon.py
Outdated
@@ -91,9 +91,9 @@ def validate_calculations(expected_results): | |||
valid = False | |||
|
|||
try: | |||
actual_dict = calc.out.output_parameters.get_dict() | |||
actual_dict = calc.out.parameters.get_dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, after discussion with some users, parameters
conveys a meaning of being an input. output_parameters
was alleviating partially this issue (even if ParameterData is still not necessarily the good name). This change however makes things even less clear (you risk to refer to these as the 'parameters' of the calculation, making it ambiguous). Maybe we can find a different name? E.g. results, for the default output? Or some other better name?
We should also think if we want to rename ParameterData e.g. to Dict and move it into 'base', but this is probably out of scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't like the superfluous output_
since it is already an output, but I agree, parameters
is also pretty bad. Clearly, I am all for changing the name. I would be all for changing ParameterData
into Dict
and subsequently calling the default output results
. This is one of the things I wanted to discuss with the other plugin developers, in coming up with a common interface. Shall I revert this change and we do everything in one go? But I guess, if we are going to change ParameterData
, we should do it now.
ebaeee9
to
37243b0
Compare
Now that job calculations are directly implemented as processes, by sub classing the `CalcJob` process, a lot of functionality has been removed from the `CalcJobNode` that did not belong there. However, quite a bit of attributes and methods remain there that are used by the `Parser` that parses results attached to the node, such as names for retrieved links. These should, just like the input and output nodes, be specified on the `Process` class. We redesign the `Parser` class to get this the information it needs from the `Process` class which can be gotten through the `CalcJobNode` as a proxy. Sub sequently, it can directly get information like default output node labels as well as exit codes from the process class. The base method that will be called by the engine to trigger parsing of a retrieved job calculation is the `parse` method. The engine will pass the nodes that the parser needs for parsing as keyword arguments. The engine knows which nodes to pass as they have been marked as such by the `CalcJob` process spec. The `Parser` class exposes a convenience method `parse_from_node` that can be called outside of the engine, passing in a `CalcJobNode`. This will trigger the `parse` method to be called again, but it will be wrapped in a `calcfunction` ensuring that the produced outputs will be connected to a `ProcessNode` in the provenance graph. Finally, what was the `CalculationResultManager` has been renamed to `CalcJobResultManager` and has been moved to `aiida.orm.utils`. The interface has also been adapted and it now no longer goes through the parser class to get the information it needs but rather through the `CalcJob` class that can be retrieved from the `CalcJobNode` for which it is constructed.
37243b0
to
df373df
Compare
Fixes #2390
Now that job calculations are directly implemented as processes, by sub
classing the
CalcJob
process, a lot of functionality has been removedfrom the
CalcJobNode
that did not belong there. However, quite a bitof attributes and methods remain there that are used by the
Parser
that parses results attached to the node, such as names for retrieved
links. These should, just like the input and output nodes, be specified
on the
Process
class.We redesign the
Parser
class to get this the information it needs fromthe
Process
class which can be gotten through theCalcJobNode
as aproxy. Sub sequently, it can directly get information like default
output node labels as well as exit codes from the process class. The
base method that will be called by the engine to trigger parsing of a
retrieved job calculation is the
parse
method. The engine will passthe nodes that the parser needs for parsing as keyword arguments. The
engine knows which nodes to pass as they have been marked as such by the
CalcJob
process spec.The
Parser
class exposes a convenience methodparse_from_node
thatcan be called outside of the engine, passing in a
CalcJobNode
. Thiswill trigger the
parse
method to be called again, but it will bewrapped in a
calcfunction
ensuring that the produced outputs will beconnected to a
ProcessNode
in the provenance graph.Finally, what was the
CalculationResultManager
has been renamed toCalcJobResultManager
and has been moved toaiida.orm.utils
. Theinterface has also been adapted and it now no longer goes through the
parser class to get the information it needs but rather through the
CalcJob
class that can be retrieved from theCalcJobNode
for whichit is constructed.