-
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
Rename node class for JobCalculation
#2201
Rename node class for JobCalculation
#2201
Conversation
The node class changed from `JobCalculation` to `CalcJobNode`. Since the `JobCalculation` class is heavily used by users, as it is the class that is sub classed to define a job calculation, we cannot quite get rid of it. Instead we moved all the logic from the old `Calculation` and `JobCalculation` classes to their new equivalents `ProcessNode` and `CalcJobNode`, and put an alias to `CalcJobNode` for `JobCalculation`. This last class can now still be imported from `aiida.orm.calculation` and `aiida.orm.calculation.job`. Note that the paths that import directly from the `implementation.general` sub module have been removed, because the job calculation classes are now fully concrete. Furthermore, the old entry points for the base calculation classes `inline`, `job` `work` and `function` have been removed as they have been replaced with the entry points in the `aiida.node` group for the new node classes.
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.
A few minor comments
@@ -45,98 +45,6 @@ def get_duplicate_node_uuids(self): | |||
""" | |||
return self.raw(query) | |||
|
|||
# This is an example of a query that could be overriden by a better implementation, | |||
# for performance reasons: | |||
def query_jobcalculations_by_computer_user_state( |
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.
Are these just renamed or completely removed? These were here because they could be made faster by custom implementations of the query (how does now verdi calculation list
get the list of non-finished calculations, and is this fast?
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 removed them because they were not being used, and they are outdated. verdi calculation list
uses the JobCalculation._list_calculations
classmethod, which is also not ideal, but that has been the situation for a long time. Currently, it is the process state that determines "activeness" of a process and hence calculation, the old calc_state
is merely an additional attribute.
calculation_type = d['calculation']['type'] | ||
calculation_class = get_plugin_type_from_type_string(calculation_type) | ||
module, class_name = calculation_class.rsplit('.', 1) | ||
|
||
# For the base class 'calculation.job.JobCalculation' the module at this point equals 'calculation.job' | ||
# For the base class 'calculation.job.CalcJobNode' the module at this point equals 'calculation.job' |
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.
This comment is outdated? (calculation.job.CalcJobNode
-> node.process.calculation.calcjob.CalcJobNode
?)
# Find a new subfolder. | ||
# I do not user tempfile.mkdtemp, because it puts random characters | ||
## Find a new subfolder. | ||
## I do not user tempfile.mkdtemp, because it puts random characters |
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.
Why two # characters? Either also for the lines below or don't add the second #
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.
Mistake. In first commit of this branch, I copied over content of JobCalculation
and already cleaned it a bit such as these double comment characters. But then in following commits I changed JobCalculation
a bit and to make sure that I included everything in this PR, I simply pasted the JobCalculation
code once more in CalcJobNode
, which undid my removal of the two characters. Long story short, I will remove them again :)
'simpleplugins.arithmetic.add = aiida.orm.calculation.job.simpleplugins.arithmetic.add:ArithmeticAddCalculation', | ||
'simpleplugins.templatereplacer = aiida.orm.calculation.job.simpleplugins.templatereplacer:TemplatereplacerCalculation', | ||
'arithmetic.add = aiida.calculations.plugins.arithmetic.add:ArithmeticAddCalculation', | ||
'templatereplacer = aiida.calculations.plugins.templatereplacer:TemplatereplacerCalculation', |
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.
Should we add entries for arithmetic and templatereplacer in the plugin registry, to reserve the names?
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.
Yes, if you agree with the renaming, we should do this. I did not like the simpleplugins
prefix, but if we could find a good namespace to put these internal ones in that'd be ok too. If we think it is not a problem to reserve these namespace, then we should register them on the registry
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.
Safe
# For the base class 'calculation.job.CalcJobNode' the module at this point equals 'calculation.job' | ||
# For this case we should simply set the type to the base module calculation.job. Otherwise we need | ||
# to strip the prefix to get the proper sub module | ||
# For the base class 'mode.process.calculation.calcjob.CalcJobNode' the module at this point equals |
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.
There's a typo here but we can fix in the next PR
Fixes #2199
The node class changed from
JobCalculation
toCalcJobNode
.Since the
JobCalculation
class is heavily used by users, as it is theclass that is sub classed to define a job calculation, we cannot quite get
rid of it. Instead we moved all the logic from the old
Calculation
andJobCalculation
classes to their new equivalentsProcessNode
andCalcJobNode
,and put an alias to
CalcJobNode
forJobCalculation
.This last class can now still be imported from
aiida.orm.calculation
andaiida.orm.calculation.job
. Note that the paths that import directly from theimplementation.general
sub module have been removed, because the job calculationclasses are now fully concrete.
Furthermore, the old entry points for the base calculation classes
inline
,job
work
andfunction
have been removed as they have been replaced with the entrypoints in the
aiida.node
group for the new node classes.