Move getting completed job accounting to retrieve transport task
#3639
Conversation
This file contains 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
sphuber
force-pushed
the
fix_3133_detailed_calcjob_info
branch
2 times, most recently
from
73709c1
to
b81f5a6
Compare
aiida/schedulers/scheduler.py
Outdated
| def get_detailed_jobinfo(self, jobid): | ||
| """ | ||
| Return a string with the output of the detailed_jobinfo command. | ||
|
|
||
| .. deprecated:: use the `get_detailed_job_info` instead |
There was a problem hiding this comment.
Can we declare when we drop this (2.0) and issue a warning in the code?
|
Actually, a second comment - here we're writing job_info with an underscore since it's a new key anyway. What to do we do with the old one? Am I allowed to just replace it with an underscore when I change from string to dict? |
sphuber
force-pushed
the
fix_3133_detailed_calcjob_info
branch
from
b81f5a6
to
8d2cfe6
Compare
I think that if you change the attribute key, we should add a migration. I am not sure if it is worth the effort but feel free to add it if you prefer the consistency in the attribute keys |
The task of retrieving the detailed accounting info from the scheduler for completed jobs was erroneously placed within the scheduler status update cycle of the `JobsList`. This would have requested the detailed job info each update cycle where it not for a conditional that checked that the scheduler status was `DONE`. However, within the `JobsList` loop, completed jobs would not even appear in the result and hence the conditional would never be hit. This caused the detailed accounting never to be retrieved. A logically better place for this functionality is in the `retrieve` transport task after the `UPDATE` task, i.e. when the scheduler reports that the job has terminated. The job accounting will query the scheduler for the full accounting which, when implemented for the given scheduler plugin, will be stored as an attribute on the calculation job node. The `Scheduler.get_detailed_jobinfo` method is deprecated in favour of the `Scheduler.get_detailed_job_info`. The former was returning a string with the return value, stdout and stderr formatted within it, which is not very flexible. The replacing method returns a dictionary.
This is useful for future implementation of a method that can parse the string output into a dictionary.
sphuber
force-pushed
the
fix_3133_detailed_calcjob_info
branch
from
8d2cfe6
to
661b57c
Compare
giovannipizzi
approved these changes
Dec 10, 2019
This was referenced Dec 12, 2019
sphuber
pushed a commit
to giovannipizzi/aiida-core
that referenced
this pull request
Dec 13, 2019
For historical reasons, this field was stored as a JSON-serialized string in the `last_jobinfo` attribute of a CalcJob. However, this is cumbersome and makes querying very hard. We now replace this with a dictionary, thanks to new commands to get directly a dictionary (with serialized fields, so that the dictionary is JSON-serializable). These (and existing) methods of the JobInfo class are now also tested. Finally, the attribute key has been renamed from `last_jobinfo` to `last_job_info`, for consistency with the key `detailed_job_info` introduced in aiidateam#3639. By changing the type of the content, the field is anyway not directly usable as before in scripts, so changing the name is not an additional issue. This should not give a real backward- incompatibility problem, since this field was there mostly for debugging reasons.
sphuber
pushed a commit
that referenced
this pull request
Dec 13, 2019
For historical reasons, this field was stored as a JSON-serialized string in the `last_jobinfo` attribute of a CalcJob. However, this is cumbersome and makes querying very hard. We now replace this with a dictionary, thanks to new commands to get directly a dictionary (with serialized fields, so that the dictionary is JSON-serializable). These (and existing) methods of the JobInfo class are now also tested. Finally, the attribute key has been renamed from `last_jobinfo` to `last_job_info`, for consistency with the key `detailed_job_info` introduced in #3639. By changing the type of the content, the field is anyway not directly usable as before in scripts, so changing the name is not an additional issue. This should not give a real backward- incompatibility problem, since this field was there mostly for debugging reasons.
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 #3133
The task of retrieving the detailed accounting info from the scheduler
for completed jobs was erroneously placed within the scheduler status
update cycle of the
JobsList. This would have requested the detailedjob info each update cycle where it not for a conditional that checked
that the scheduler status was
DONE. However, within theJobsListloop, completed jobs would not even appear in the result and hence the
conditional would never be hit. This caused the detailed accounting
never to be retrieved.
A logically better place for this functionality is in the
retrievetransport task after the
UPDATEtask, i.e. when the scheduler reportsthat the job has terminated. The job accounting will query the scheduler
for the full accounting which, when implemented for the given scheduler
plugin, will be stored as an attribute on the calculation job node.