Make the job submission script configurable through CalcJob options
#3948
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3948 +/- ##
===========================================
- Coverage 78.31% 78.31% -0.01%
===========================================
Files 461 461
Lines 34080 34079 -1
===========================================
- Hits 26690 26688 -2
- Misses 7390 7391 +1
Continue to review full report at Codecov.
|
sphuber
added
the
priority/critical-blocking
There was a problem hiding this comment.
thanks a lot @sphuber !
The only question that comes to my mind is whether script_filename is clear to everyone.
Some parts of the API use submit_script, see e.g. https://aiida.readthedocs.io/projects/aiida-core/en/latest/apidoc/aiida.schedulers.plugins.html#aiida.schedulers.plugins.direct.DirectScheduler._get_submit_script_header
submit_script_filename would be a bit longer, of course... I guess it's a matter of taste.
The name of the job submission script was hard coded to `_aiidasubmit.sh` which not only makes it less flexible but it also required the value to be passed in a long series of calls from `CalcJob.presubmit` all the way through the transport tasks to the `submit_calculation` method in the `aiida.engine.daemon.execmanager` module. This value is now the input `metadata.options.submit_script_filename` with the default being the same. This allows to remove the value from the return value of `CalcJob.presubmit` and the call-chain through all the transport tasks, because `submit_calculation` can now simply retrieve the correct value directly from the `CalcJobNode`. This change also fixes a bug in the `task_upload_job` that returned the wrong value if the idempotency check was hit to see if the method had already been executed. This can happen if the daemon is stopped before the next transport task is reached but after the upload has already been executed. This check prevents it from being executed again by simply returning. However, it was supposed to return the `calc_info` and `script_filename` normally returned by the `presubmit` call, but it would no longer have these variables and so returned `True`. The caller `Waiting.execute` would expect a tuple and except.
sphuber
force-pushed
the
fix/3947/idempotency-task-upload
branch
from
a959de0 to
be69479
Compare
|
Thanks @ltalirz . I actually agree with the more specific name |
Fixes #3947
The name of the job submission script was hard coded to
_aiidasubmit.shwhich not only makes it less flexible but it also required the value to
be passed in a long series of calls from
CalcJob.presubmitall the waythrough the transport tasks to the
submit_calculationmethod in theaiida.engine.daemon.execmanagermodule.This value is now configurable through
metadata.options.script_filenamewith the default being the same. This allows to remove the value from
the return value of
CalcJob.presubmitand the call-chain through allthe transport tasks, because
submit_calculationcan now simplyretrieve the correct value directly from the
CalcJobNode.This change also fixes a bug in the
task_upload_jobthat returned thewrong value if the idempotency check was hit to see if the method had
already been executed. This can happen if the daemon is stopped before
the next transport task is reached but after the upload has already been
executed. This check prevents it from being executed again by simply
returning. However, it was supposed to return the
calc_infoandscript_filenamenormally returned by thepresubmitcall, but itwould no longer have these variables and so returned
True. The callerWaiting.executewould expect a tuple and except.