Implement CalcJob process class #2389
Merged
sphuber merged 4 commits intoJan 17, 2019
Merged
Conversation
0bf4e16 to
fa3652c
Compare
fa3652c to
1d4d305
Compare
5c35c74 to
62638b4
Compare
This commit can be summarized in three steps: * Reimplementation of a job calculation as `Process` called `CalcJob` * Changing job calculation to be purely informational * Remove the old job calculation mechanics and business logic The old way of creating a job calculation, was to subclass the `JobCalculation` class and override the `_use_methods` class method to define the input nodes and the `_prepare_for_submission` to setup the input files for the calculation. The problem was that these methods were implemented on the `Node` class, thus mixing the responsabilities of running and introspecting the results of a completed calculation. Here we define the `CalcJob` class, a subclass of `Process`. This class replaces the old `JobCalculation` and allows a user to defined the inputs and outputs through the `ProcessSpec`, just as they would do for a `WorkChain`. Except, instead of defining an `outline`, one should implement the `prepare_for_submission`, which fulfills the exact same function as before, only it is now a public method of the `CalcJob` process class. Finally, the role of the job calculation state, stored as an attribute with the key `state` on the `CalcJobNode` has changed significantly. The original job calculations had a calculation state that controlled the logic during its lifetime. This was already superceded a long time ago by the process wrapper that now fully governs the progression of the calculation. Despite the calculation state no longer being authoritative during the calculation's lifetime, it was still present. Here we finally fully remove and only leave a stripped down version. The remaining state is stored as an attribute and is a sub state while the `CalcJob` process is in an active state and serves as a more granual state that can be queried for. This is useful, because the process status, which also keeps similar information is human readable and doesn't allow for easy querying.
62638b4 to
72083b6
Compare
giovannipizzi
requested changes
Jan 17, 2019
Member
giovannipizzi
left a comment
There was a problem hiding this comment.
Just a couple minor things
Member
There was a problem hiding this comment.
A few notes here
- you should escape the underscore (should be
\_) - maybe better to do something like
(attribute.key='environment_variables' OR attribute.key like 'environment\_variables.%')
Similar for the others where applicable (where it's a single variable and not a dict/list, you don't need to do like instead, just =)
Member
There was a problem hiding this comment.
Don't we want to still do some validation depending on the (process_)state? Or it's somewhere else?
The `replace_*link_from` methods where only implemented because they were needed by the old job calculation created. Since that system is now removed, this functionality is no longer needed. Likewise, the methods to remove links, either from the cache or the database were not used nor tested and so they are removed.
When a `Node` instance is sealed, it should be impossible to add links to or from it. To enforce this, the `validate_incoming` and `validate_outgoing` methods are overridden in the `Sealable` mixin that will raise `ModificationNotAllowed` if the node is sealed.
In the pattern operands for the `LIKE` operator, the underscore is a special character that matches any character. If a literal underscore should be matched, it should be escaped by a backslash.
d0b6c67 to
70f5ff6
Compare
giovannipizzi
approved these changes
Jan 17, 2019
Member
|
Wonderful! |
This was referenced Jan 17, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2377, fixed #2280, fixes #2219 and fixed #2381
This commit can be summarized in three steps:
ProcesscalledCalcJobThe old way of creating a job calculation, was to subclass the
JobCalculationclass and override the_use_methodsclass method todefine the input nodes and the
_prepare_for_submissionto setup theinput files for the calculation. The problem was that these methods were
implemented on the
Nodeclass, thus mixing the responsabilities ofrunning and introspecting the results of a completed calculation.
Here we define the
CalcJobclass, a subclass ofProcess. This classreplaces the old
JobCalculationand allows a user to defined theinputs and outputs through the
ProcessSpec, just as they would do fora
WorkChain. Except, instead of defining anoutline, one shouldimplement the
prepare_for_submission, which fulfills the exact samefunction as before, only it is now a public method of the
CalcJobprocess class.
Finally, the role of the job calculation state, stored as an attribute
with the key
stateon theCalcJobNodehas changed significantly.The original job calculations had a calculation state that controlled
the logic during its lifetime. This was already superceded a long time
ago by the process wrapper that now fully governs the progression of the
calculation. Despite the calculation state no longer being authoritative
during the calculation's lifetime, it was still present. Here we finally
fully remove and only leave a stripped down version. The remaining state
is stored as an attribute and is a sub state while the
CalcJobprocessis in an active state and serves as a more granual state that can be
queried for. This is useful, because the process status, which also
keeps similar information is human readable and doesn't allow for easy
querying.