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

Redesign the Parser class #2397

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jan 17, 2019

Fixes #2390

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.

@sphuber sphuber requested a review from giovannipizzi January 17, 2019 15:03
@coveralls
Copy link

coveralls commented Jan 17, 2019

Coverage Status

Coverage decreased (-1.4%) to 68.089% when pulling df373df on sphuber:fix_2390_parser_redesign into 2c961cb on aiidateam:provenance_redesign.

@@ -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()
Copy link
Member

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.

Copy link
Contributor Author

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.

@sphuber sphuber force-pushed the fix_2390_parser_redesign branch from ebaeee9 to 37243b0 Compare January 17, 2019 16:47
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.
@sphuber sphuber force-pushed the fix_2390_parser_redesign branch from 37243b0 to df373df Compare January 17, 2019 17:01
@sphuber sphuber merged commit 423523c into aiidateam:provenance_redesign Jan 18, 2019
@sphuber sphuber deleted the fix_2390_parser_redesign branch January 18, 2019 07:45
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

Successfully merging this pull request may close these issues.

3 participants