Refactor task killing & simplify task state deltas #6457

MetRonnie · 2024-11-04T17:54:14Z

In preparation for tackling the last piece of the cylc remove proposal:

When all flows are removed from active tasks, Cylc should first attempt to kill the job as it serves no further purpose and may cause resource contention issues if left running, however, failure of the kill command should be tolerated, though logged.

I extracted and de-deduped the kill mechanism into a scheduler method.

Also, it seems to me the data store manager's delta_task_state() method should include is_held, is_queued and is_runahead.

Check List

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
No dependency changes
Tests are included (or explain why tests are not needed).
Changelog entry not needed as refactor.
Docs not needed
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

MetRonnie · 2024-11-04T17:55:25Z

cylc/flow/scheduler.py

+
+        Returns number of tasks that could not be killed.
+        """
+        jobless = self.get_run_mode() == RunMode.SIMULATION


(Need to follow-up with skip mode #6039)

MetRonnie · 2024-11-04T20:28:26Z

tests/integration/tui/screenshots/test_task_states.filter-not-waiting-or-expired.html

@@ -8,13 +8,13 @@
 <span style="color:#000000;background:#e5e5e5">            </span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">̿● a                                                               </span>
 <span style="color:#000000;background:#e5e5e5">         </span><span style="color:#000000;background:#e5e5e5">-</span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">̿⊗ Y                                                                  </span>
 <span style="color:#000000;background:#e5e5e5">            </span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">̿⊗ b                                                               </span>
-<span style="color:#000000;background:#e5e5e5">      </span><span style="color:#000000;background:#e5e5e5">-</span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">̿⊘ 2                                                                     </span>
+<span style="color:#000000;background:#e5e5e5">      </span><span style="color:#000000;background:#e5e5e5">-</span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">̎⊘ 2                                                                     </span>


The reason for the change in icon is that previously, the held state was not updated in the data store by the test fixture:

cylc-flow/tests/integration/tui/test_app.py

Lines 34 to 44 in 478b0d0

def set_task_state(schd, task_states):

"""Force tasks into the desired states.

Task states should be of the format (cycle, task, state, is_held).

"""

for cycle, task, state, is_held in task_states:

itask = schd.pool.get_task(cycle, task)

if not itask:

itask = schd.pool.spawn_task(task, cycle, {1})

itask.state_reset(state, is_held=is_held)

schd.data_store_mgr.delta_task_state(itask)

MetRonnie · 2024-11-05T11:42:29Z

cylc/flow/task_events_mgr.py

@@ -1322,7 +1322,6 @@ def _process_message_failed(self, itask, event_time, message, forced):
            no_retries = True
            if itask.state_reset(TASK_STATUS_FAILED, forced=forced):
                self.setup_event_handlers(itask, self.EVENT_FAILED, message)
-                self.data_store_mgr.delta_task_state(itask)


Duplicate of L1328 just below

oliver-sanders · 2024-11-05T13:18:53Z

cylc/flow/data_store_mgr.py

            tp_id, PbTaskProxy(id=tp_id))
        tp_delta.stamp = f'{tp_id}@{update_time}'
        tp_delta.state = itask.state.status
+        tp_delta.is_held = itask.state.is_held


Currently:

attr updater

status delta_task_state

is_held delta_task_held

is_queued delta_task_queued

is_runahead delta_task_runahead

I'm not sure why we would move is_held in with delta_task_state? Unless we wanted to move all the attrs into delta_task_state?

To me it looks like it makes sense to move all into delta_task_state? These are all handled by 1 method on TaskProxy, so it would reduce chances of forgetting to update them all in the data store. And it would be more efficient in the data store to do all at once

We could do it all in one, but I think it would be less (not more) efficient for the data store to do this all at once as we would be pushing out all four attributes every time any one of them changes.

If that's a concern, could do

if tproxy.is_held != itask.state.is_held: tp_delta.is_held = itask.state.is_held

etc.

Or drop the commit as I've separated it out into its own commit now.

Sounds good. I think, either we leave as is, or we combine all the related attrs and compare to previous value as you suggested.

oliver-sanders · 2024-11-05T13:20:23Z

cylc/flow/scheduler.py

+                    status=(TASK_STATUS_FAILED if jobless else None),
+                    is_held=True,
+                )
+                self.data_store_mgr.delta_task_state(itask)


Suggested change

self.data_store_mgr.delta_task_state(itask)

self.data_store_mgr.delta_task_state(itask)

self.data_store_mgr.delta_task_held(itask)

Not needed if we include itask.is_held in delta_task_state()

dwsutherland

Approval conditional on your option of doing this:

if tproxy.is_held != itask.state.is_held:
    tp_delta.is_held = itask.state.is_held

And you'd want to do the same with queue and runahead ..etc
Also check/do nothing if nothing has changed (unless that's impossible upstream)..

It's only if a field is set that it will be sent as a delta, so if we can avoid sending a field, that's ideal.

MetRonnie · 2024-11-06T13:10:46Z

@dwsutherland I noticed that if you inspect tp_delta.is_held it already as a value (True/False) at time of creation of the PbTaskProxy object. However, unless you proactively set a value on it, it does not get included in the delta received at the UI. What sorcery is this? 😅

MetRonnie added small code refactor Large code refactors labels Nov 4, 2024

MetRonnie added this to the 8.4.0 milestone Nov 4, 2024

MetRonnie self-assigned this Nov 4, 2024

MetRonnie commented Nov 4, 2024

View reviewed changes

MetRonnie marked this pull request as draft November 5, 2024 11:12

MetRonnie force-pushed the cylc-kill branch from 00b2a97 to d842c9a Compare November 5, 2024 11:31

MetRonnie marked this pull request as ready for review November 5, 2024 11:41

MetRonnie commented Nov 5, 2024

View reviewed changes

MetRonnie requested a review from oliver-sanders November 5, 2024 11:42

oliver-sanders reviewed Nov 5, 2024

View reviewed changes

Refactor task killing

fb9eb2b

MetRonnie force-pushed the cylc-kill branch from d842c9a to 392dbfe Compare November 5, 2024 14:25

MetRonnie requested a review from dwsutherland November 5, 2024 14:26

MetRonnie changed the title ~~Refactor task killing~~ Refactor task killing & simplify task state deltas Nov 5, 2024

dwsutherland approved these changes Nov 5, 2024

View reviewed changes

Simplify task state deltas

d326432

MetRonnie force-pushed the cylc-kill branch from 392dbfe to d326432 Compare November 6, 2024 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor task killing & simplify task state deltas #6457

Refactor task killing & simplify task state deltas #6457

MetRonnie commented Nov 4, 2024 •

edited

Loading

MetRonnie Nov 4, 2024

MetRonnie Nov 4, 2024

MetRonnie Nov 5, 2024

oliver-sanders Nov 5, 2024 •

edited

Loading

MetRonnie Nov 5, 2024 •

edited

Loading

oliver-sanders Nov 5, 2024

MetRonnie Nov 5, 2024

oliver-sanders Nov 5, 2024

oliver-sanders Nov 5, 2024

MetRonnie Nov 5, 2024

dwsutherland left a comment

MetRonnie commented Nov 6, 2024

	def set_task_state(schd, task_states):
	"""Force tasks into the desired states.

	Task states should be of the format (cycle, task, state, is_held).
	"""
	for cycle, task, state, is_held in task_states:
	itask = schd.pool.get_task(cycle, task)
	if not itask:
	itask = schd.pool.spawn_task(task, cycle, {1})
	itask.state_reset(state, is_held=is_held)
	schd.data_store_mgr.delta_task_state(itask)

attr	updater
status	delta_task_state
is_held	delta_task_held
is_queued	delta_task_queued
is_runahead	delta_task_runahead

	self.data_store_mgr.delta_task_state(itask)
	self.data_store_mgr.delta_task_state(itask)
	self.data_store_mgr.delta_task_held(itask)

Refactor task killing & simplify task state deltas #6457

Are you sure you want to change the base?

Refactor task killing & simplify task state deltas #6457

Conversation

MetRonnie commented Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

MetRonnie Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwsutherland left a comment

Choose a reason for hiding this comment

MetRonnie commented Nov 6, 2024

MetRonnie commented Nov 4, 2024 •

edited

Loading

oliver-sanders Nov 5, 2024 •

edited

Loading

MetRonnie Nov 5, 2024 •

edited

Loading