Open dthulke opened 3 months ago
Do you need/would like to have a separate function to resume a job?
No, this is why I suggest Option 1 and propose to remove the existing broken code that tries to do this.
I would prefer also option 1, I used this feature once in all the years (before the bug was even introduced), and then figured out I can solve it in a different way.
Agreed, Option 1 is the way to go here!
The separation into two functions is a left over from the beginning of Sisyphus. I thought we need two function, one for a clean start and one which can resume an interrupted job. In practice I realized that the resumeable function need to handle all possible cases, including a clean start, anyway, so there is really no need to have two functions here.
Great! Will prepare a PR.
When defining a Task you can set a
resume
function name which is called instead of the actual Task function when a task is resumed, e.g.:For this to work, the worker need to set the resume_job flag when calling
Task.run
. But this is currently hardcoded to False: https://github.com/rwth-i6/sisyphus/blob/a22e9236ef2a0dcb62fc322bd012f9d0f4e95063/sisyphus/worker.py#L244-L253The bug was introduced in commit c4f825b043bfd81c75d40f7fd9719e7ffb375f42 which removed the logic that detected whether a job is resumed.
To fix this I see two options:
resume: str
by aresumable: bool
(keeping and deprecatingresume
for compatibility).Any opinions? I'd vote for Option 1.