Skip to content

Add User Cancel Event to Journal, Server cancel replaced with Abort#1089

Open
Dym03 wants to merge 1 commit intoIt4innovations:mainfrom
Dym03:cancel_with_reason
Open

Add User Cancel Event to Journal, Server cancel replaced with Abort#1089
Dym03 wants to merge 1 commit intoIt4innovations:mainfrom
Dym03:cancel_with_reason

Conversation

@Dym03
Copy link
Copy Markdown
Contributor

@Dym03 Dym03 commented Apr 13, 2026

This PR finishes work on #1001 with the addition of JobCancel event into the Journal. Also this PR introduces difference between cancellation of Task made by user and hq, as was suggested by @spirali so canceled tasks are canceled by user and aborted tasks are canceled by the hq.

I would like to hear your opinion if the colors suggested are okay with you

create_dataset(&aborted, "Aborted", Color::LightCyan) and
DashboardTaskState::Aborted => Color::LightCyan
Example below:
image

Documentation:

I will update the documentation once I get agreement that this state is wanted

Sorry for not dividing it into a smaller chunk, like only the addition of the JobCancel, but while I was at it, I just took a shot at doing it at once :P.

@Dym03 Dym03 force-pushed the cancel_with_reason branch 3 times, most recently from f6a1136 to 32a3882 Compare April 13, 2026 20:57
@Dym03
Copy link
Copy Markdown
Contributor Author

Dym03 commented Apr 13, 2026

Updated test so it matches the new aborted state.

@spirali
Copy link
Copy Markdown
Collaborator

spirali commented Apr 14, 2026

LGTM.
Random thoughs:

  • Maybe color should be more similar to "Failed" as it something happen from runtime not from a user command.
  • We can think about (not in this PR) if not to use Abort also after reaching crash counter (it now sets a Failed state, but technically it is not that the fail of the task itself), but it would be nice to distinguish between "abort-after-dep" and "abort-after-crash".

@Dym03
Copy link
Copy Markdown
Contributor Author

Dym03 commented Apr 14, 2026

Okey, will look at the colorway. And update the documentation. Also there are some minor bugs of Abort, that will be fixed.
About the crash counter, that as you said could be changed in another PR.

@Dym03 Dym03 marked this pull request as draft April 14, 2026 12:28
@Dym03 Dym03 force-pushed the cancel_with_reason branch 7 times, most recently from f34dcb0 to 4e8013f Compare April 14, 2026 18:57
@Dym03 Dym03 marked this pull request as ready for review April 14, 2026 19:08
@Dym03 Dym03 force-pushed the cancel_with_reason branch from 4e8013f to 71018af Compare April 14, 2026 19:34
@Dym03
Copy link
Copy Markdown
Contributor Author

Dym03 commented Apr 14, 2026

Actually finding all the places that this would affect took me a while. But god bless tests, that forced me to do it. Hopefully I have managed to discover all the places.

Example of the colorway in dashboard:
image

@spirali spirali self-requested a review April 15, 2026 13:20
Copy link
Copy Markdown
Collaborator

@spirali spirali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the final colors.

I have just put one change request to the documentation; otherwise it is ready to merge.

Comment thread docs/jobs/jobs.md
| | | |
v v v v
Finished Failed Canceled Aborted
```
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change just to:

Waiting--------------------\
   | ^                     |
   | |                     |
   v |                     |
Running--------------------|
   | |                     |
   | \--------\            |
   |          |            |
   v          v            v
Finished    Failed   Canceled/Aborted

@Dym03 Dym03 force-pushed the cancel_with_reason branch from 71018af to 328c72d Compare April 15, 2026 13:59
@Dym03
Copy link
Copy Markdown
Contributor Author

Dym03 commented Apr 15, 2026

Documentation updated, I think with this merged the #1001 can be closed. And maybe new Issue can be opened to capture the thought about the distuguish of the Abort due to Dependency/ Abort due to crash of worker | due to max fails. I have found 2 more things that needs an update, so I will update them later today.

@Dym03 Dym03 marked this pull request as draft April 15, 2026 14:48
@Dym03 Dym03 force-pushed the cancel_with_reason branch from 328c72d to 4ee7a47 Compare April 15, 2026 18:35
@Dym03 Dym03 marked this pull request as ready for review April 15, 2026 18:38
@Dym03
Copy link
Copy Markdown
Contributor Author

Dym03 commented Apr 15, 2026

Updated documentation about the addition of the Aborted state also in the Job, to keep the original idea of propagating the task state to the Job state. But it is also mentioned in the documentation that this shouldn't happen as aborted state is closely connected to the failure of the task, which would imply that this Job State shouldn't be reached, in the current state.
But I think that it should be added, as if there ever in the future there could be abortion of the task without the need of a failure of a task, I think this should be ready to go :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants