Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix graphql node state and remove ghosts filters #4791

Merged
merged 4 commits into from
Apr 21, 2022
Merged

fix graphql node state and remove ghosts filters #4791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf43477
fix graphql node state and remove ghosts filters
dwsutherland Mar 31, 2022
4cece6e
node state data-store lookup
dwsutherland Mar 31, 2022
7fa56dd
change log entry
dwsutherland Mar 31, 2022
307d1fc
Update CHANGES.md
hjoliver Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ distinguishing between workflow not running and not in run-directory.
platforms, any files configured to be installed will be updated on the remote
platform.

[#4791](https://github.com/cylc/cylc-flow/pull/4791) - Fix GraphQL node
filtering, affecting non-state updates in the UI.

[#4777](https://github.com/cylc/cylc-flow/pull/4777) -
Reinstate the Cylc 7 template variables for xtriggers with deprecation warnings.

Expand Down
63 changes: 44 additions & 19 deletions cylc/flow/network/resolvers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,15 +194,18 @@ def node_ids_filter(tokens, state, items) -> bool:
)
# match cycle/task/job state
and (
not get_state_from_selectors(item)
not (
state is not None
and get_state_from_selectors(item)
)
dwsutherland marked this conversation as resolved.
Show resolved Hide resolved
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):
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

@oliver-sanders oliver-sanders Apr 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.
    """

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's right.

"""Filter nodes based on attribute arguments"""
tokens: Tokens
if node_type in DEF_TYPES:
Expand All @@ -216,20 +219,17 @@ def node_filter(node, node_type, args):
else:
# live objects can be represented by a universal ID
tokens = Tokens(node.id)
state = getattr(node, 'state', None)
return (
(
args.get('ghosts')
or state
or node_type in DEF_TYPES
)
and (
not args.get('states')
(
state is None
or not args.get('states')
Comment on lines +225 to +226
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@dwsutherland dwsutherland Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)

)
or state in args['states']
)
and not (
args.get('exstates')
and state in args['exstates']
and (
not args.get('exstates')
or state not in args['exstates']
)
and (
args.get('is_held') is None
Expand All @@ -252,9 +252,9 @@ def node_filter(node, node_type, args):
not args.get('ids')
or node_ids_filter(tokens, state, args['ids'])
)
and not (
args.get('exids')
and node_ids_filter(tokens, state, args['exids'])
and (
not args.get('exids')
or not node_ids_filter(tokens, state, args['exids'])
)
)

Expand Down Expand Up @@ -329,14 +329,34 @@ async def get_workflows(self, args):
args)

# nodes
def get_node_state(self, node, node_type):
"""Return state, from node or data-store."""
if node_type in DEF_TYPES:
return None
# Don't lookup data-store state in getattr, as it might not be there.
# (args are evaluated first)
# If node has no state, it must be in the data-store.
with suppress(Exception):
return (
getattr(node, 'state', None)
or self.data_store_mgr.data[
Tokens(node.id).workflow_id
][node_type][node.id].state
Comment on lines +341 to +344
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct 👍

)

async def get_nodes_all(self, node_type, args):
"""Return nodes from all workflows, filter by args."""
return sort_elements(
[
n
node
for flow in await self.get_workflows_data(args)
for n in flow.get(node_type).values()
if node_filter(n, node_type, args)
for node in flow.get(node_type).values()
if node_filter(
node,
node_type,
args,
self.get_node_state(node, node_type)
)
],
args,
)
Expand Down Expand Up @@ -365,7 +385,12 @@ async def get_nodes_by_ids(self, node_type, args):
for flow in flow_data
for node_type in node_types
for node in get_data_elements(flow, nat_ids, node_type)
if node_filter(node, node_type, args)
if node_filter(
node,
node_type,
args,
self.get_node_state(node, node_type)
)
],
args,
)
Expand Down
25 changes: 0 additions & 25 deletions cylc/flow/network/schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@ class SortArgs(InputObjectType):
reverse = Boolean(default_value=False)


GHOSTS_DEFAULT = Boolean(default_value=False)
STRIP_NULL_DEFAULT = Argument(
Boolean, description="A flag that when enabled strips out those fields "
"not set in the protobuf object. And when this flag "
Expand Down Expand Up @@ -633,7 +632,6 @@ class Meta:
lambda: TaskProxy,
description="""Task cycle instances.""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@dwsutherland dwsutherland Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@hjoliver hjoliver Apr 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

(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.)

Copy link
Member Author

@dwsutherland dwsutherland Apr 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand All @@ -642,7 +640,6 @@ class Meta:
lambda: FamilyProxy,
description="""Family cycle instances.""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand All @@ -665,7 +662,6 @@ class Meta:
nodes_edges = Field(
lambda: NodesEdges,
args=NODES_EDGES_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand Down Expand Up @@ -757,7 +753,6 @@ class Meta:
lambda: TaskProxy,
description="""Associated cycle point proxies""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand Down Expand Up @@ -912,7 +907,6 @@ class Meta:
lambda: FamilyProxy,
description="""Task parents.""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand All @@ -921,7 +915,6 @@ class Meta:
lambda: FamilyProxy,
description="""Task first parent.""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand All @@ -930,7 +923,6 @@ class Meta:
lambda: FamilyProxy,
description="""First parent ancestors.""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand All @@ -948,7 +940,6 @@ class Meta:
lambda: FamilyProxy,
description="""Associated cycle point proxies""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand Down Expand Up @@ -1014,7 +1005,6 @@ class Meta:
TaskProxy,
description="""Descendant task proxies.""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand All @@ -1023,7 +1013,6 @@ class Meta:
lambda: FamilyProxy,
description="""Descendant family proxies.""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand All @@ -1032,7 +1021,6 @@ class Meta:
lambda: FamilyProxy,
description="""Task first parent.""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand All @@ -1041,7 +1029,6 @@ class Meta:
lambda: FamilyProxy,
description="""First parent ancestors.""",
args=PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
delta_store=DELTA_STORE_DEFAULT,
delta_type=DELTA_TYPE_DEFAULT,
Expand Down Expand Up @@ -1152,7 +1139,6 @@ class Meta:
TaskProxy,
description=TaskProxy._meta.description,
args=ALL_PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
resolver=get_nodes_all)
family = Field(
Expand All @@ -1177,7 +1163,6 @@ class Meta:
FamilyProxy,
description=FamilyProxy._meta.description,
args=ALL_PROXY_ARGS,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
resolver=get_nodes_all)
edges = graphene.List(
Expand All @@ -1190,7 +1175,6 @@ class Meta:
NodesEdges,
description=NodesEdges._meta.description,
args=NODES_EDGES_ARGS_ALL,
ghosts=GHOSTS_DEFAULT,
strip_null=STRIP_NULL_DEFAULT,
resolver=get_nodes_edges)

Expand Down Expand Up @@ -1943,7 +1927,6 @@ class Delta(Interface):
FamilyProxy,
description="""Family cycle instances.""",
args=PROXY_ARGS,
ghosts=Boolean(default_value=True),
strip_null=Boolean(),
delta_store=Boolean(default_value=True),
delta_type=String(default_value=DELTA_ADDED),
Expand Down Expand Up @@ -1971,7 +1954,6 @@ class Delta(Interface):
TaskProxy,
description="""Task cycle instances.""",
args=PROXY_ARGS,
ghosts=Boolean(default_value=True),
strip_null=Boolean(),
delta_store=Boolean(default_value=True),
delta_type=String(default_value=DELTA_ADDED),
Expand Down Expand Up @@ -2020,7 +2002,6 @@ class Meta:
FamilyProxy,
description="""Family cycle instances.""",
args=PROXY_ARGS,
ghosts=Boolean(default_value=False),
strip_null=Boolean(),
delta_store=Boolean(default_value=True),
delta_type=String(default_value=DELTA_UPDATED),
Expand Down Expand Up @@ -2048,7 +2029,6 @@ class Meta:
TaskProxy,
description="""Task cycle instances.""",
args=PROXY_ARGS,
ghosts=Boolean(default_value=False),
strip_null=Boolean(),
delta_store=Boolean(default_value=True),
delta_type=String(default_value=DELTA_UPDATED),
Expand Down Expand Up @@ -2172,7 +2152,6 @@ class Meta:
TaskProxy,
description=TaskProxy._meta.description,
id=ID(required=True),
ghosts=Boolean(default_value=True),
strip_null=Boolean(default_value=True),
delta_store=Boolean(default_value=True),
delta_type=String(default_value=DELTA_ADDED),
Expand All @@ -2184,7 +2163,6 @@ class Meta:
TaskProxy,
description=TaskProxy._meta.description,
args=ALL_PROXY_ARGS,
ghosts=Boolean(default_value=True),
strip_null=Boolean(default_value=True),
delta_store=Boolean(default_value=True),
delta_type=String(default_value=DELTA_ADDED),
Expand Down Expand Up @@ -2218,7 +2196,6 @@ class Meta:
FamilyProxy,
description=FamilyProxy._meta.description,
id=ID(required=True),
ghosts=Boolean(default_value=True),
strip_null=Boolean(default_value=True),
delta_store=Boolean(default_value=True),
delta_type=String(default_value=DELTA_ADDED),
Expand All @@ -2230,7 +2207,6 @@ class Meta:
FamilyProxy,
description=FamilyProxy._meta.description,
args=ALL_PROXY_ARGS,
ghosts=Boolean(default_value=True),
strip_null=Boolean(default_value=True),
delta_store=Boolean(default_value=True),
delta_type=String(default_value=DELTA_ADDED),
Expand All @@ -2253,7 +2229,6 @@ class Meta:
NodesEdges,
description=NodesEdges._meta.description,
args=NODES_EDGES_ARGS_ALL,
ghosts=Boolean(default_value=True),
strip_null=Boolean(default_value=True),
delta_store=Boolean(default_value=True),
delta_type=String(default_value=DELTA_ADDED),
Expand Down
3 changes: 0 additions & 3 deletions tests/integration/test_resolvers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ def flow_args():
@pytest.fixture
def node_args():
return {
'ghosts': False,
'workflows': [],
'exworkflows': [],
'ids': [],
Expand Down Expand Up @@ -114,7 +113,6 @@ async def test_get_nodes_all(mock_flow, node_args):
node_args['states'].append('failed')
nodes = await mock_flow.resolvers.get_nodes_all(TASK_PROXIES, node_args)
assert len(nodes) == 0
node_args['ghosts'] = True
node_args['states'] = []
node_args['ids'].append(Tokens(mock_flow.node_ids[0]))
nodes = [
Expand All @@ -136,7 +134,6 @@ async def test_get_nodes_by_ids(mock_flow, node_args):
nodes = await mock_flow.resolvers.get_nodes_by_ids(TASK_PROXIES, node_args)
assert len(nodes) == 0

node_args['ghosts'] = True
node_args['native_ids'] = mock_flow.node_ids
nodes = [
n
Expand Down