Add support for cancelled jobs. #23
Conversation
…. This entails refactoring the `@step` method to be a decorator or a decorator factory, depending on whether the first argument is `callable`.
…ather than direct references to `WorkflowStep` objects.
…et_input_value`." This reverts commit 4de9c10
…ndex of a given label.
…f index or label was specified; false otherwise.)
…red with `@step` decorator.
Co-authored-by: Néstor Pérez <[email protected]>
Co-authored-by: Néstor Pérez <[email protected]>
Co-authored-by: Néstor Pérez <[email protected]>
This reverts commit a1beeae.
…ich have been cancelled (or have any finalised status) are not run. This is essential for catching the new `CANCELLED` status, since the check for the remainder is necessarily done at the end of the step's execution.
|
@prryplatypus the tests are failing with an error that I'm not sure is related to my code.
Thoughts? |
|
Also, idk why there are so many old commits in this PR since I branched off of main after pulling upstream into it. |
ergate/job.py
Outdated
| def mark_aborted(self, message: str) -> None: | ||
| self.status = JobStatus.ABORTED | ||
|
|
||
| @property |
There was a problem hiding this comment.
I'd rather keep this as a method
There was a problem hiding this comment.
I've reverted it, so that's fine. But, why? It's merely a computed value from another property which doesn't have external calls, so wouldn't it serve well as a property?
ergate/job_runner.py
Outdated
| if job.should_be_requeued: | ||
| return | ||
|
|
There was a problem hiding this comment.
What's this for? This would be True for every job that's just coming through the queue, and would cause the worker to keep stopping 🤔
There was a problem hiding this comment.
Thanks for catching that. I forgot the not!
It's supposed to be if not job.should_be_requeued(), and the purpose of this is to catch jobs which have been set to CANCELLED in between steps, so that it does not process them anyway and then overwrite the CANCELLED status with RUNNING here: https://github.com/contrains/ergeats/blob/main/ergate/job_runner.py#L36
There was a problem hiding this comment.
This won't work if something gets cancelled in the state store (Netbox) if it doesn't get changed in the queue too, since this job object is obtained directly from the queue. The idea was to avoid an extra call to the state store every time a job was acquired.
There was a problem hiding this comment.
I see. Where would you recommend this check being made then?
| ruff format --check ergate | ||
| ruff format --diff ergate |
There was a problem hiding this comment.
Not yet, anyway. ;) I was leaving it in until this issue was resolved: #23 (comment)
Do you have any thoughts about that?
There was a problem hiding this comment.
I'm going to merge your PR with that since it does indeed appear unrelated. However I won't release until I get it fixed (possibly in a separate PR).
There was a problem hiding this comment.
If my code subsequently is shown to have any issues once that is resolved, I'll plan to make a new PR to address them.
contrains
force-pushed
the
add-cancelled-job-status
branch
from
8eb472c to
6e985c3
Compare
…ued()` invocation.
This pull request adds support for a cancelled job status. This allows the user to manually cancel a workflow.
Changes include:
JobStatus.CANCELLED.not Job.should_be_requeued)Job.should_be_requeueda class property.Jobmethods alphabetically.