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.

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

N-Distance nodes & edges #3337

Merged
merged 4 commits into from
Sep 11, 2019
Merged

Conversation

dwsutherland
Copy link
Member

@dwsutherland dwsutherland commented Aug 30, 2019

These changes close #3248
Accompanying UI Server PR: cylc/cylc-uiserver#70

Changes:

  • NodesEdges: type and query added to the GraphQL schema.
  • edges field added to the PbTaskProxy nodes: for efficient resolution of the nodesEdges query.
  • Reformed data-store: This was done to have a uniform access for the resolvers at both WS an UIS.
  • Faster resolvers! All resolvers now use the native data-store map structure, instead of the entire_workflow protobuf message (attributes and lists), so all resolvers that use id to directly retrieve data (not root queries) should be faster (in theory).
  • BaseResolvers class created: Contains all resolvers for import and/or use by both WS an UIS, with local Resolvers class inheriting this.
  • get_nodes_edges resolver created: (in BaseResolvers) to gather the data elements for nodesEdges query resolution.

Resolver & Query
The resolver works by filtering for root or starting nodes with similar filter args as taskProxies:
image

the one new argument is distance which, inline with graph theory, is the number of edges (in shortest path) between two nodes... To the resolver, distance is how many iterations of the following:

  • Start with root/new nodes (distance n-1).
  • Use the edges field of the nodes to acquire new edges.
  • Add new edges to edges (edges should be unique).
  • Gather the source & target nodes from these edges, the difference of these and current set of nodes give new nodes (nodes should be unique after use of difference).
  • Add new nodes to nodes.
  • Repeat to n

Notes: distance defaults to 1, a value of zero (or negative values) return the root nodes only, the algorithm will keep going until either:

  • there are no more new nodes
  • distance is reached

Because sets and set operations are used, then when new edges meet, the search won't overlap.. Only new node edges are used...

The GraphQL query looks like:

query {
  nodesEdges(
      workflows: ["*|*"],
  	  ids: ["*|*"],
      states: ["running", "succeeded"],
      distance: 0,
      sort: {keys: ["cycle_point"], reverse: false}) {
    nodes {
      name
      cyclePoint
      state
      isHeld
    }
    edges {
      id
    }
  }
}

(nodes having taskProxy fields)

Example:
Take the following suite:

[cylc]
    cycle point format = CCYYMMDDThh
    UTC mode = True
[scheduling]
    initial cycle point = 20190101T00
    max active cycle points = 2
    [[graph]]
        P1D = """
foo[-P1D] => foo
foo => baa => bar => baz  # long chain
qux  # isolate
"""
[runtime]
    [[root]]
        script = sleep 15; echo "$CYLC_TASK_NAME"
    [[qux]]
        script = sleep 60; echo "$CYLC_TASK_NAME"

The above query yields

{
  "data": {
    "nodesEdges": {
      "nodes": [
        {
          "name": "foo",
          "cyclePoint": "20190101T00",
          "state": "running"
        },
        {
          "name": "qux",
          "cyclePoint": "20190101T00",
          "state": "running"
        },
        {
          "name": "baa",
          "cyclePoint": "20190101T00",
          "state": "waiting"
        },
        {
          "name": "qux",
          "cyclePoint": "20190102T00",
          "state": "running"
        },
        {
          "name": "foo",
          "cyclePoint": "20190102T00",
          "state": "waiting"
        }
      ],
      "edges": [
        {
          "id": "sutherlander|noz|foo.20190101T00|foo.20190102T00"
        },
        {
          "id": "sutherlander|noz|foo.20190101T00|baa.20190101T00"
        }
      ]
    }
  }
}

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).
  • Already covered with extension of existing tests.
  • No change log entry required (invisible to users).
  • No documentation update required.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Change looks good! Worked with no issues testing the Cylc UI + UI Server. Question on the response attributes (more out of curiosity, as I'm not using the nodesEdge/nodes for the tree component).

  1. suicide I believe is a boolean flag to say whether the node is representing a suicide task or not. What is the cond boolean for? The GraphiQL interface gives no description for that field.

image

  1. What about targetNode and sourceNode? They appear to only contain __typename attribute, which will always be TaskProxy I guess?

image

And a note, probably more useful to @MartinRyan, I hacked a bit the Graph.vue locally to link the data returned from GraphQL and see if it would work with Cytoscape.

I had a few issues with the Graph.vue, and Cytoscape. But the only issue I had with the data returned, was that the edges returned contained nodes that were not in the list of nodes.

I hacked - some more - code to filter edges with nodes missing source or target from the list of nodesEdges.nodes.

Alas, the best that I could get was:

image

Sometimes 5, sometimes 6 nodes. Which matches with what I normally get in the Tree view as well 🎉

No blocker from me, so +1. Thanks @dwsutherland !

cylc/flow/network/resolvers.py Outdated Show resolved Hide resolved
cylc/flow/network/resolvers.py Outdated Show resolved Hide resolved
@kinow
Copy link
Member

kinow commented Sep 2, 2019

Should we care about negative or zero distance? I tested to make sure the UI wouldn't crash, but it seems like it simply ignores the distance when 0 or -1 are given as argument to nodeEdges(distance: -1){}.

@kinow
Copy link
Member

kinow commented Sep 2, 2019

Oh, would be nice to have some docs in GraphiQL too. Here's the maxDepth documentation for Tasks`:

image

But some fields in Edge - for instance - are returning "No Description", e.g. Edge.targetNode:

image

And Edge.cond and Edge.suicide have both the same documentation "The Boolean scalar type represents true or false".

image

Would be nice to have some text describing what the flag does/enables/disables/etc.

Thanks!

@kinow
Copy link
Member

kinow commented Sep 2, 2019

@dwsutherland, moved the part about documenting the returned data to: #3340

@cylc cylc deleted a comment Sep 2, 2019
@cylc cylc deleted a comment Sep 2, 2019
@dwsutherland
Copy link
Member Author

@kinow

  1. suicide I believe is a boolean flag to say whether the node is representing a suicide task or not. What is the cond boolean for? The GraphiQL interface gives no description for that field.

Those are edge attributes (not node) .. So they describe something about the nature of the dependency between source and target nodes. (conditional relationship foo | bar => qux , and suicide foo => !bar).. They were in the get_graph_raw edges so didn't take them out.

  1. What about targetNode and sourceNode? They appear to only contain __typename attribute, which will always be TaskProxy I guess?

They are fragment fields, because originally the nodes could be family or task proxies... So you can do this:

read -r -d '' nodesEdges <<_args_
{
  "request_string": "
fragment tProxy on TaskProxy {
  state
  cyclePoint
  latestMessage
  jobs {
    id
    state
    host
    batchSysName
    batchSysJobId
    submittedTime
    startedTime
    finishedTime
    submitNum
  }
}

fragment fProxy on FamilyProxy {
  state
  cyclePoint
}

query {
  nodesEdges(workflows: [\"${1}\"], ids: [\"foo\"], distance: ${DISTANCE}, sort: {keys: [\"state\", \"id\"], reverse: false}) {
    nodes {
      id
      state
    }
    edges {
      id
      source
      sourceNode {
        ...tProxy
        ...fProxy
      }
      target
      targetNode {
        ...tProxy
        ...fProxy
      }
    }
  }
}",
"variables": null
}
_args_

I had a few issues with the Graph.vue, and Cytoscape. But the only issue I had with the data returned, was that the edges returned contained nodes that were not in the list of nodes.

I hacked - some more - code to filter edges with nodes missing source or target from the list of nodesEdges.nodes.

The only "nodes" that should be missing from the nodesEdges query output are the xtriggers....
Otherwise there's a bug..

@kinow
Copy link
Member

kinow commented Sep 3, 2019

@dwsutherland brilliant! Thanks for the answers! I've still got some more homework to learn more about GraphQL 😞 but getting there!

@hjoliver
Copy link
Member

hjoliver commented Sep 3, 2019

The boolean for "conditional dependency" is used to style edges differently (hollow arrow-head) in Cylc 7.

@cylc cylc deleted a comment Sep 8, 2019
Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Some initial comments. I am half way through looking at the changes by 👀.

cylc/flow/network/resolvers.py Show resolved Hide resolved
cylc/flow/network/resolvers.py Outdated Show resolved Hide resolved
cylc/flow/network/resolvers.py Show resolved Hide resolved
Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

I have now eyeballed the changes. Will have a go testing the branch tomorrow.

cylc/flow/ws_data_mgr.py Outdated Show resolved Hide resolved
@cylc cylc deleted a comment Sep 10, 2019
@cylc cylc deleted a comment Sep 10, 2019
@matthewrmshin matthewrmshin modified the milestones: cylc-8.0a1, cylc-8.0a2 Sep 10, 2019
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

No problems found in playing with the branch, or seen today in the code ... LGTM 👍

@hjoliver
Copy link
Member

That's three approvals.

@hjoliver hjoliver merged commit db88720 into cylc:master Sep 11, 2019
@cylc cylc deleted a comment Sep 11, 2019
@matthewrmshin matthewrmshin modified the milestones: cylc-8.0a2, cylc-8.0a1 Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Active Node-Edge N-Distance GraphQL Query
5 participants