data store: blank task state

[oliver-sanders]

A user has encountered a traceback in cylc tui which suggests that a task had a blank status:

  File "/.../cylc-8.3.6-1/lib/python3.9/site-packages/cylc/flow/tui/util.py", line 118, in get_task_icon
    ret.append(TASK_ICONS[status])
KeyError: ''

Tasks in the store should always have a status, even far future tasks should be waiting.

It's not clear how to reproduce this traceback (I haven't seen it before), possibly just some awkward edge case 🤦. We could protect Tui against this with a fallback to waiting, but it would be much better to find and cure the issue than cover it over.

[dwsutherland]

I'm not entirely sure how to reproduce it..
The state of a task is either picked up from:

• the given itask for active tasks
• batched for DB extraction

        # Active, but not in the data-store yet (new).
        if tp_id in self.n_window_nodes:
            self._process_internal_task_proxy(itask, tproxy)
            # Has run before, so get history.
            # Cannot batch as task is active (all jobs retrieved at once).
            if itask.submit_num > 0:
                flow_db = self.schd.workflow_db_mgr.pri_dao
                for row in flow_db.select_jobs_for_datastore(
                        {itask.identity}
                ):
                    self.insert_db_job(1, row)
        else:
            # Batch non-active node for load of DB history.
            self.db_load_task_proxies[itask.identity] = (
                itask,
                is_parent,
            )

The active itask should have a status:

    def spawn_task(
        self,
        name: str,
        point: 'PointBase',
        flow_nums: Set[int],
        flow_wait: bool = False,
    ) -> Optional[TaskProxy]:
        """Return a new task proxy for the given flow if possible.
        We need to hit the DB for:
        - submit number
        - task status
        - flow-wait
        - completed outputs (e.g. via "cylc set")
        If history records a final task status (for this flow):
        - if not flow wait, don't spawn (return None)
        - if flow wait, don't spawn (return None) but do spawn children
        - if outputs are incomplete, don't auto-rerun it (return None)
        Otherwise, spawn the task and load any completed outputs.
        """
        submit_num, prev_status, prev_flow_wait = (
            self._get_task_history(name, point, flow_nums)
        )
        # Create the task proxy with any completed outputs loaded.
        itask = self._get_task_proxy_db_outputs(
            point,
            self.config.get_taskdef(name),
            flow_nums,
            status=prev_status or TASK_STATUS_WAITING,
            submit_num=submit_num,
            flow_wait=flow_wait,
        )

The batch is processed before prune/add/update of any node:

    def update_data_structure(self):
        """Workflow batch updates in the data structure."""
        # Avoids changing window edge distance during edge/node creation
        if self.next_n_edge_distance is not None:
            self.n_edge_distance = self.next_n_edge_distance
            self.window_resize_rewalk()
            self.next_n_edge_distance = None
        # load database history for flagged nodes
        self.apply_task_proxy_db_history()
.
.
.

So unless the DB contains no entry for a node, the state should be set..

To combat this, we could simply set PbTaskProxy.state to TASK_STATUS_WAITING on creation, then let it be overwritten by the active or DB entry... A cheap workaround, but would hopefully cover any oddities.

Thoughts?

[oliver-sanders]

To combat this, we could simply set PbTaskProxy.state to TASK_STATUS_WAITING on creation

Lack of state should cause a brittle error, like this, so that we are made aware of the issue and can fix it. We should avoid using default values to cover up problems as this can lead to further issues (especially if the task isn't actually waiting).

Thoughts?

I have no idea, all I know is that the traceback is possible.

[dwsutherland]

We should avoid using default values to cover up problems as this can lead to further issues (especially if the task isn't actually waiting).

Well, if we agree that a task should be born in waiting state, we can probably justify setting a default so there's no possibility of it being left in limbo before the actual state is applied.

[dwsutherland]

See this comment for a description of how this error comes about, and potential solutions:
#6852 (comment)

[oliver-sanders]

The issue seems to be that tasks are added into the store before we have obtained their state from the DB.

So it's not a bug, it's more a consequence of us employing batching for DB lookups (to reduce the amount of queries hammering the DB for every task in the n>0 window)

Shouldn't the data store manager just wait for complete information before adding the task to the store?

Maybe the window computation should be delayed too?

Using a placeholder value is dodgy as it could be wrong. I think we would rather the update to take a little while longer than for it to contain incorrect information.

[dwsutherland]

Here are the options

1) We need to assign an intermediate state (which would be largely unnoticeable, and if not momentary), before batch processing.. I'm happy with using waiting state..

2) Another option is to adapt the system to handle stateless tasks i.e. using ( ❔ ) symbol like we do with workflow state when unknown.. Which would be more accurate.
(and the easiest way to do that is create an official unknown task state, otherwise we need to hunt down all the code, front end and back, that expects a state and change it)

A 3rd option is; we change the batching to be applied at the end of a increment_graph_window (instead of update_data_store of the main loop), that would work.. The only issue is, if you add in 1000 tasks (of independent sequences) you may have 1000 DB queries (of n lines, depending on window size) instead of one query with 1000 (*n) lines..

Shouldn't the data store manager just wait for complete information before adding the task to the store?

(
You would think a 4th option is to batch the creation n>0 data-store objects with the DB lookup, but that would complicate things in many ways.. The window creation expects to find already created nodes, so it doesn't recreate them (including edges of those nodes and their children/parents).. Also, we will miss deltas if the node(s) don't exist yet.. etc..
)

Maybe the window computation should be delayed too?

Catch22: How would you know which items you need to lookup from the DB without a window computation.

[dwsutherland]

Options 1, 2, 3 are on the table

[dwsutherland]

I warned about 3 .. but just batching one window walk and applying immediately might be enough

[dwsutherland]

Note: This inbetween state is almost never witnessed (otherwise TUI would crash all the time)

I will post all three options as draft