Fix data-store reloaded flag

[dwsutherland]

Partially addresses cylc/cylc-uiserver#485

The reloaded field wasn't being set/sent, this change fixes it:

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).
• Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
• Covered by existing tests.
• CHANGES.md entry included if this is a change that can affect users
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

[oliver-sanders]

Cheers, this should be enough information for the UI to maintain its store. Couple of questions:

• Can there be added/updated/pruned information in a reloaded delta?
  • If so, should this be ignored, or applied after the store is wiped?
• Can we send the reloaded delta when workflows restart?
  • Or maybe just make the first delta sent on scheduler startup a reloaded delta to initiate a blank store.
  • This would be enough information for the UI to handle restart scenarios (note restart infers reload).

[dwsutherland]

• Can there be added/updated/pruned information in a reloaded delta?

  • If so, should this be ignored, or applied after the store is wiped?

There will be added information (pretty certain). I can't rule out the others (although it might not be possible), but pretty hard to rule out what gets included main loop batch.. However, the publishing in this case might happen outside the main loop batching.
The reload wipes the data-store/delta-store and rebuilds, so any updated/pruned should be applied after the reloaded wipe and added applied.

• Can we send the reloaded delta when workflows restart?

  • Or maybe just make the first delta sent on scheduler startup a reloaded delta to initiate a blank store.
  • This would be enough information for the UI to handle restart scenarios (note restart infers reload).

Good idea, then I can avoid setting fields at the UIS.. I'll add it to this PR.

[dwsutherland]

Ok I've extended the reloaded flag to play/restart/initial-burst.
All of these types will included the added data with the flag set to true, there shouldn't be any other updated/pruned in the same push.

[oliver-sanders]

Tested in combination with cylc/cylc-ui#1479, works for both reloads and restarts.

I applied this diff to the cylc-ui branch to ensure things were working as intended:

diff --git a/src/services/treeCallback.js b/src/services/treeCallback.js
index 276501bb..562b1910 100644
--- a/src/services/treeCallback.js
+++ b/src/services/treeCallback.js
@@ -38,11 +38,16 @@ class CylcTreeCallback extends DeltasCallback {
     // do this, we can end up with nodes in the store which aren't meant to be
     // there and won't get pruned.
     if (deltas.updated?.workflow?.reloaded) {
+      console.log('updated', deltas.updated.workflow)
       store.commit('workflows/REMOVE_CHILDREN', (deltas.updated.workflow.id))
+      return
     }
     if (deltas.added?.workflow?.reloaded) {
+      console.log('added', deltas.added.workflow)
       store.commit('workflows/REMOVE_CHILDREN', (deltas.added.workflow.id))
+      return
     }
+    console.log('REGULAR DELTA')
   }
 
   onAdded (added, store, errors) {
diff --git a/src/store/workflows.module.js b/src/store/workflows.module.js
index 3dff0a8a..5c49542a 100644
--- a/src/store/workflows.module.js
+++ b/src/store/workflows.module.js
@@ -535,7 +535,7 @@ const mutations = {
   },
   // remove all children of a node
   REMOVE_CHILDREN (state, id) {
-    // console.log('@ REMOVE CHILDREN')
+    console.log('@ REMOVE CHILDREN')
     const workflow = getIndex(state, id)
     if (workflow) {
       removeTree(state, workflow, false)

It looks like the UI is receiving more reloaded deltas than expected.

We would expect two reloaded deltas, one per subscription (because we have one subscription for gscan and one for each workflow to make subscription management easier). But, I'm sometimes seeing 6 deltas, so 3 per subscription which doesn't make sense.

This appears to be harmless as the first non reloaded delta doesn't arrive until after all of the reloaded deltas, so we don't appear to have the issue of these being mixed in with reloaded deltas causing added elements to be removed in between.

[dwsutherland]

I usually just use firefox dev tools (ctrl+shift+e) and track the deltas comming through the subscription.. I'll have a look this avo if I can nail down a different PR I'm working on.

[oliver-sanders]

I usually just use firefox dev tools (ctrl+shift+e) and track the deltas comming through the subscription.

I did do this, but the results are very noisy so harder to convey. It does look like there's some duplicate reloaded deltas coming through, but the reason for this isn't necessarily related.

[MetRonnie]

LGTM, not sure how to reproduce the question mark bug

[oliver-sanders]

I've had a go at testing a regular subscription (no deltas) in GraphiQL:

subscription {
  workflows(ids: ["~osanders/sequential"]) {
    id
    reloaded
  }
}

I used a regular workflow with all of the tasks held to avoid task updates coming through.

Here are the results, there seem to be some erroneous updates, errors marked in bold:

• start subscription
  • reloaded: true (the workflow is stopped, it shouldn't show as reloaded)
• cylc play sequential --pause
  • reloaded: true
• cylc reload sequential
  • reloaded: true
• cylc play sequential
  • reloaded: false
• cylc reload sequential
  • reloaded: false
• cylc reload sequential
  • reloaded: false
• cylc reload sequential
  • reloaded: false
• cylc stop sequential
  • reloaded: false
  • reloaded: false
• cylc play sequential
  • reloaded: true
• cylc reload sequential
  • reloaded: false
• cylc reload sequential
  • reloaded: false
• cylc reload sequential
  • reloaded: false

Raw web socket responses

[                                                                                                                       
  ["start subscription"],                                                                                               
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": true}]}}},  

  ["cylc", "play", "sequential", "--pause"],                                                                            
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": true}]}}},  

  ["cylc", "reload", "sequential"],                                                                                     
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": true}]}}},  

  ["cylc", "play", "sequential"],                                                                                       
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": false}]}}}, 

  ["cylc", "reload", "sequential"],
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": false}]}}},

  ["cylc", "reload", "sequential"],
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": false}]}}},

  ["cylc", "reload", "sequential"],
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": false}]}}},

  ["cylc", "stop", "sequential"],
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": false}]}}},
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": false}]}}},

  ["cylc", "play", "sequential"],
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": true}]}}}, 

  ["cylc", "reload", "sequential"],
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": false}]}}},

  ["cylc", "reload", "sequential"],
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": false}]}},

  ["cylc", "reload", "sequential"],
  {"id": "8", "type": "data", "payload": {"data": {"workflows": [{"id": "~osanders/sequential", "reloaded": false}]}}}
]                          

@MetRonnie, could you check you get the same result as me?

[MetRonnie]

I get much the same thing

 * start subscription
   * reloaded: **true** (the workflow is stopped, it shouldn't show as reloaded)
 * cylc play sequential --pause
   * reloaded: true
 * cylc reload sequential
-  * reloaded: true
+  * reloaded: **false**
 * cylc play sequential
   * reloaded: false
 * cylc reload sequential
   * reloaded: **false**
 * cylc reload sequential
   * reloaded: **false**
 * cylc reload sequential
   * reloaded: **false**
 * cylc stop sequential
   * reloaded: false
 * cylc play sequential
   * reloaded: true
 * cylc reload sequential
   * reloaded: **false**
 * cylc reload sequential
   * reloaded: **false**
 * cylc reload sequential
   * reloaded: **false**

[dwsutherland]

Will have to take a look in the morning.. If I can reproduce, I can fix it.

[dwsutherland]

Everything is working as expected! 😆

That subscription (subscriptions { workflows ... ) isn't ideal for two reasons:

• Everytime the data-store changes the subscription sends a snapshot of the data-store (or delta-store if specified), and the data-store may have turned reloaded flag true then false between a delta coming in and the dump being collected and sent.
• Because it's snap-shots of the entire data-store, there's an option ignoreInterval which defaults to 2.5 seconds (to ignore deltas that have been sent too close to the previous).

i.e. if you used a subscription with:

subscription {
  workflows (ids: ["~sutherlander/linear/run1"], ignoreInterval: 0, deltaType: "added")
  	{
      id
      reloaded
    }
}

it will send a dump of the last added store every time the data-store changes (in any way), and it will always have reloaded: true (which means there are no false sent with added workflows)...
Our early UI prototypes used this kind of subscription to get going, because you didn't have to manage a UI store.. (which could still be useful in some cases)

However going through that above sequence of commands with:

subscription {
  deltas(workflows: ["~sutherlander/linear/run1"], stripNull: true) {
    id
    added {
      workflow {
        status
        statusMsg
        reloaded
      }
    }
   updated {
      workflow {
        status
        statusMsg
        reloaded
      }
    }
  }
}

Will always send reloaded: true where expected.. You can see these in the network traffic for the subscription (in the browser), amongst all the updated deltas.

We may want to try ignore sending deltas if the subscription resolves to empty to some level (i.e. only IDs) or the same content as the previous sent (?) .. But that has not been implemented, so we have to fish through all the deltas to find the corresponded added delta.

• start subscription
  • reloaded: true (the workflow is stopped, it shouldn't show as reloaded)

This is intentional, when a new subscription is made, an initial-burst of data is sent with the data-store sent as an added delta (essentially), and the client told it's a reload in order to remove any old/irrelevant data it may be holding:

                for w_id in w_ids:
                    if w_id in self.data_store_mgr.data:
                        if sub_id not in delta_queues[w_id]:
                            delta_queues[w_id][sub_id] = deltas_queue
                            # On new yield workflow data-store as added delta
                            if args.get('initial_burst'):
                                delta_store = create_delta_store(
                                    workflow_id=w_id)
                                delta_store[DELTA_ADDED] = (
                                    self.data_store_mgr.data[w_id])
                                delta_store[DELTA_ADDED][
                                    WORKFLOW
                                ].reloaded = True
                                deltas_queue.put(
                                    (w_id, 'initial_burst', delta_store))
                    elif w_id in self.delta_store[sub_id]:
                        del self.delta_store[sub_id][w_id]

(if this isn't desirable we can change it)

[oliver-sanders]

Ok, it makes sense to me that the reloaded field might not show correctly in plain subscriptions as a result of multiple deltas getting merged (although I'm a little confused in what it was being merged with) thanks for explaining.

I've put up the cylc-ui end of this, it wipes the store when it receives a reloaded delta (whether it's added or updated as the UI is agnostic on this), the applies any other data that was delivered with the delta.

Testing this branch in combination with cylc/cylc-ui#1479 should demonstrate the fix to cylc/cylc-ui#1480

[dwsutherland]

(although I'm a little confused in what it was being merged with)

Yeah, by default it's a dump of the data-store (so the only merging is the deltas from the scheduler into the UIS data-store). The arrival of a delta from the scheduler triggers this dump to be sent to the UI/subscriber.

[oliver-sanders]

Reloaded deltas coming through as expected.

Suggest keeping this open whilst cylc/cylc-ui#1479 is reviewed as the two are a pair and any issues discovered in cylc/cylc-ui#1479 may require changes here.

[hjoliver]

Merging, the UI PR is in now.