fix graphql node state and remove ghosts filters #4791

dwsutherland · 2022-03-31T02:11:36Z

These changes partially address cylc/cylc-uiserver#341

Delta's without a state field set will be filtered by state arguments... This is done by supplementing the node state using the data-store when not set..

Requirements 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.
Already covered by existing tests.
Appropriate change log entry included.
No documentation update required.

hjoliver · 2022-03-31T02:43:12Z

Already covered by existing tests.

How can we test that non-state updates get through though? (Damn, "through though" is indictment of the English language, innit).

dwsutherland · 2022-03-31T02:55:45Z

Already covered by existing tests.

How can we test that non-state updates get through though? (Damn, "through though" is indictment of the English language, innit).

Opps, I meant graphql functional tests (and the entire CLI) all use the node filter I changed.
WRT graphql subscriptions, yes we can't test at the scheduler yet.

hjoliver · 2022-03-31T03:01:32Z

WRT graphql subscriptions, yes we can't test at the scheduler yet.

Yeah, but a UIS test maybe?

dwsutherland · 2022-03-31T03:04:48Z

WRT graphql subscriptions, yes we can't test at the scheduler yet.

Yeah, but a UIS test maybe?

Don't think we have and websockets tests for graphql subscriptions.. We just mock them all for queries.. Be easier once we can via the scheduler or command line via the hub.

Something to look into for sure.

oliver-sanders · 2022-03-31T09:32:38Z

closes cylc/cylc-uiserver#328

Right issue?

hjoliver

Yep, that works 🎉

CHANGES.md

hjoliver · 2022-04-01T01:24:24Z

Integration test failures:

FAILED tests/integration/test_resolvers.py::test_get_nodes_all - assert 1 == 0
FAILED tests/integration/test_scheduler.py::test_illegal_config_load[illegal item-True]
FAILED tests/integration/test_scheduler.py::test_illegal_config_load[illegal cycle point-True]

The last two are fixed by #4794

The first one is presumably genuine.

dwsutherland · 2022-04-01T02:26:52Z

Integration test failures:

FAILED tests/integration/test_resolvers.py::test_get_nodes_all - assert 1 == 0
FAILED tests/integration/test_scheduler.py::test_illegal_config_load[illegal item-True]
FAILED tests/integration/test_scheduler.py::test_illegal_config_load[illegal cycle point-True]

The last two are fixed by #4794

The first one is presumably genuine.

Yeah, genuine.. I fixed the filter, we shouldn't really have any nodes (with state field) that have empty state, but possible if the node is added but not updated yet.. so handle this now.

cylc/flow/network/resolvers.py

hjoliver · 2022-04-01T03:17:35Z

 tests/integration/test_task_pool.py::test_preparing_tasks_on_restart 
Error: The action has timed out.

I can't get this to hang locally though.

dwsutherland · 2022-04-01T03:22:21Z

 tests/integration/test_task_pool.py::test_preparing_tasks_on_restart 
Error: The action has timed out.

I can't get this to hang locally though.

I put an error suppression around accessing the state from the data-store, I think it must be either the mock tests or the scenarios causing that point to fail (the store not to be available or something).. it was intermittent though..

dwsutherland · 2022-04-01T11:05:19Z

closes cylc/cylc-uiserver#328

Right issue?

It fixes an issue that make subscriptions appear not to work..

But probably not the same thing..

cylc/flow/network/resolvers.py

hjoliver · 2022-04-04T05:54:07Z

(Tests as working, but I'm slightly bamboozled by the logic above ... but maybe my brain is fried)

oliver-sanders · 2022-04-04T08:53:05Z

closes cylc/cylc-uiserver#328

Right issue?

It fixes an issue that make subscriptions appear not to work..

GraphiQL still broken for me, could you explain what you mean by "appear not to work" to help me review.

hjoliver

Thanks @dwsutherland

dwsutherland · 2022-04-13T07:31:24Z

closes cylc/cylc-uiserver#328

Right issue?

It fixes an issue that make subscriptions appear not to work..

GraphiQL still broken for me, could you explain what you mean by "appear not to work" to help me review.

Yours doesn't even get any data.. So I think it's a different issue, as this one you still got sent something but missed deltas.

oliver-sanders

I don't think I'm able to test this with GraphiQL broken for me (any ideas)? I'm also not sure I understand the problem this is fixing.

I've added a couple of questions in the review, would be really helpful to add a couple of code comments to clarify them.

cylc/flow/network/resolvers.py

oliver-sanders · 2022-04-13T09:24:39Z

cylc/flow/network/resolvers.py

                or get_state_from_selectors(item) == state
            )
        )
        for item in uniq(items)
    )


-def node_filter(node, node_type, args):
+def node_filter(node, node_type, args, state):


I'm not too clear on which bits belong to the node and which bits belong to the query?

Is the "state" the node state or the query state?

args will have the query state to filter against
state is the node state or data-store node state (if state isn't set in the delta, i.e. with outputs).

This ensures we always have a state to filter against (i.e. if we ever wanted to filter deltas by state, then deltas without a state field set can still be filtered by their data-store state)

Cheers, so would this be correct?:

def node_filter(node, node_type, args, state): """Filter nodes based on attribute arguments Args: node: The node to filter (from the data or delta store). node_type: The type of the node being filtered (or is it the type to filter by?) args: The query arguments. state: The state of the node that is being filtered. Note: can be None for non-tasks e.g. task definitions where state filtering does not apply. """

Yes, that's right.

oliver-sanders · 2022-04-13T09:25:42Z

cylc/flow/network/resolvers.py

+                state is None
+                or not args.get('states')


So if the node has no state (how does this happen?) then we include it in filter results irrespective of whether a query state was specified? Or am I misunderstanding?

So if the node has no state (how does this happen?)

It can happen if it's a task definition or something like that.

then we include it in filter results irrespective of whether a query state was specified?

Correct, as we use the same filter for nodes without states (task defs, family defs)

oliver-sanders · 2022-04-13T09:30:03Z

cylc/flow/network/resolvers.py

+                getattr(node, 'state', None)
+                or self.data_store_mgr.data[
+                    Tokens(node.id).workflow_id
+                ][node_type][node.id].state


Could you explain what these two different methods mean?

The first is getting the state from the node, the node as it exists in the data store?

The second is getting the state from the data store directly. How does this differ from the node?

The first is getting the state from the node, the node as it exists in the data store?

node could be from a data-store or a delta-store.

The second is getting the state from the data store directly. How does this differ from the node?

Yes, if the first returns None or empty string, fetch the state from the data-store...

If we want only deltas of specified state(s), then we need to filter out those deltas that don't contain state.

Ok, that makes sense, is this right?:

return ( # try to retrieve the state from the node (could be a delta-store node) getattr(node, 'state', None) # fall back to the data store (because this could be a delta-store # node which might not have the state field set) or self.data_store_mgr.data[ Tokens(node.id).workflow_id ][node_type][node.id].state

Correct 👍

oliver-sanders · 2022-04-13T09:35:42Z

cylc/flow/network/schema.py

@@ -633,7 +632,6 @@ class Meta:
        lambda: TaskProxy,
        description="""Task cycle instances.""",
        args=PROXY_ARGS,
-        ghosts=GHOSTS_DEFAULT,


I don't really understand what "ghosts" were, why we had them or why we can dispense of them now?

Ghosts where part of a crude n-window (n=1) prototype... It has long been deprecated, and need to be removed..
It relates to this PR because it was in the part of the filter I had to change.

Ok, so I can test this is still working correctly just be running the UI on a workflow and making sure the N=1 window shows as expected.

(Note the original motivation for this PR was cylc/cylc-uiserver#341 - i.e., custom task outputs not showing up at the UI in real time... which this PR definitely fixes.)

Ok, so I can test this is still working correctly just be running the UI on a workflow and making sure the N=1 window shows as expected.

The current n-window implementation is orthogonal to that old prototype. These Ghosts literally weren't used anywhere, but the args were still in the API and in the node filtering.
But sure, test n=1 if you like :)

oliver-sanders · 2022-04-13T09:39:57Z

(will see if anyone else has working GraphiQL to review...)

dwsutherland · 2022-04-14T06:21:10Z

I don't think I'm able to test this with GraphiQL broken for me (any ideas)? I'm also not sure I understand the problem this is fixing.

It fixes this issue:
cylc/cylc-uiserver#341

And the above gif in the comment;
#4791 (comment)
Shows the deltas coming through with outputs (which were being filtered out previously, as the delta had to state field)

The fix is viewable in the UI tree view (don't need graphiql), see the above mentioned issue.

oliver-sanders · 2022-04-21T10:12:33Z

Replicated cylc/cylc-uiserver#341, fixed the problem for me, 👍.

oliver-sanders · 2022-04-21T10:13:05Z

(btw tested Tui too, all good)

dwsutherland added this to the cylc-8.0rc3 milestone Mar 31, 2022

dwsutherland self-assigned this Mar 31, 2022

dwsutherland mentioned this pull request Mar 31, 2022

remove ghosts from graphql args cylc/cylc-ui#980

Merged

6 tasks

dwsutherland requested a review from hjoliver March 31, 2022 02:20

dwsutherland mentioned this pull request Mar 31, 2022

graphiql: subscriptions broken cylc/cylc-uiserver#328

Closed

dwsutherland force-pushed the delta-node-filter-fix branch from 012f667 to 42fb448 Compare March 31, 2022 02:59

dwsutherland mentioned this pull request Mar 31, 2022

2022 Cylc Meetings cylc/cylc-admin#143

Closed

dwsutherland force-pushed the delta-node-filter-fix branch from 42fb448 to 160d54c Compare March 31, 2022 03:16

oliver-sanders added the bug Something is wrong :( label Mar 31, 2022

hjoliver approved these changes Apr 1, 2022

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

dwsutherland force-pushed the delta-node-filter-fix branch from 772af16 to f1fee39 Compare April 1, 2022 02:16

hjoliver reviewed Apr 1, 2022

View reviewed changes

cylc/flow/network/resolvers.py Outdated Show resolved Hide resolved

dwsutherland force-pushed the delta-node-filter-fix branch from f1fee39 to 433b93f Compare April 1, 2022 03:12

dwsutherland force-pushed the delta-node-filter-fix branch from 433b93f to 08dcb83 Compare April 1, 2022 03:18

hjoliver self-requested a review April 1, 2022 03:20

dwsutherland requested a review from oliver-sanders April 1, 2022 21:04

hjoliver reviewed Apr 4, 2022

View reviewed changes

cylc/flow/network/resolvers.py Show resolved Hide resolved

cylc/flow/network/resolvers.py Outdated Show resolved Hide resolved

hjoliver self-requested a review April 4, 2022 09:15

dwsutherland added 3 commits April 13, 2022 15:46

fix graphql node state and remove ghosts filters

cf43477

node state data-store lookup

4cece6e

change log entry

7fa56dd

dwsutherland force-pushed the delta-node-filter-fix branch from 08dcb83 to 7fa56dd Compare April 13, 2022 03:51

Update CHANGES.md

307d1fc

hjoliver approved these changes Apr 13, 2022

View reviewed changes

oliver-sanders reviewed Apr 13, 2022

View reviewed changes

oliver-sanders approved these changes Apr 21, 2022

View reviewed changes

oliver-sanders merged commit 3f830e2 into cylc:master Apr 21, 2022

oliver-sanders mentioned this pull request Apr 21, 2022

Task outputs not shown immediately cylc/cylc-uiserver#341

Closed

fix graphql node state and remove ghosts filters #4791

fix graphql node state and remove ghosts filters #4791

Conversation

dwsutherland commented Mar 31, 2022 • edited Loading

hjoliver commented Mar 31, 2022

dwsutherland commented Mar 31, 2022

hjoliver commented Mar 31, 2022

dwsutherland commented Mar 31, 2022 • edited Loading

oliver-sanders commented Mar 31, 2022

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver commented Apr 1, 2022

dwsutherland commented Apr 1, 2022

hjoliver commented Apr 1, 2022

dwsutherland commented Apr 1, 2022 • edited Loading

dwsutherland commented Apr 1, 2022

hjoliver commented Apr 4, 2022

oliver-sanders commented Apr 4, 2022

hjoliver left a comment

Choose a reason for hiding this comment

dwsutherland commented Apr 13, 2022

oliver-sanders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwsutherland Apr 14, 2022 • 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 Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjoliver Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

dwsutherland Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

oliver-sanders commented Apr 13, 2022

dwsutherland commented Apr 14, 2022

oliver-sanders commented Apr 21, 2022

oliver-sanders commented Apr 21, 2022

dwsutherland commented Mar 31, 2022 •

edited

Loading

dwsutherland commented Mar 31, 2022 •

edited

Loading

dwsutherland commented Apr 1, 2022 •

edited

Loading

oliver-sanders Apr 19, 2022 •

edited

Loading

dwsutherland Apr 14, 2022 •

edited

Loading

dwsutherland Apr 14, 2022 •

edited

Loading

hjoliver Apr 19, 2022 •

edited

Loading

dwsutherland Apr 20, 2022 •

edited

Loading