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

schema broken (GraphQLLocatedError for 'state') #3216

Closed
sadielbartholomew opened this issue Jul 13, 2019 · 15 comments · Fixed by #3219
Closed

schema broken (GraphQLLocatedError for 'state') #3216

sadielbartholomew opened this issue Jul 13, 2019 · 15 comments · Fixed by #3219
Assignees
Labels
bug Something is wrong :(
Milestone

Comments

@sadielbartholomew
Copy link
Collaborator

sadielbartholomew commented Jul 13, 2019

Attempting some development today I found that the end-to-end system has broken so that using the current master on all three repositories cylc-flow, cylc-ui & cylc-uiserver means that (at least for the several suites I tested on), the UI Tree View gives a blank page apart from the top bar with the suite name (see the screenshot below), & further digging uncovers some error messages.

I have traced this via reverting commits (on each of the three repos independently until I hit the case where the full system went from working as previous to hitting the above problem) to #3211, which unfortunately has broken the schema, throwing up errors relating to 'state', as below.

I'm going to trace back the logic to work out which aspect(s) of that PR (with one commit) are to blame, & put in a PR to fix them. Thanks.

Error messages

GraphiQL

From interactive GraphQL, I got the following error response returned:

  "errors": [
    {
      "message": "state",
      "locations": [
        {
          "line": 21,
          "column": 5
        }
      ],
      "path": [
        "workflows",
        0,
        "tasks"
      ]
    }
  ],

(if relevant, this was from a basic query on one of my running workflows:)

query workflows($tIds: [ID]){
  workflows(ids: "subsuite-app-domingo"){
    id
    name
    status
    stateTotals {
      runahead
      waiting
      held
      queued
      expired
      ready
      submitFailed
      submitRetrying
      submitted
      retrying
      running
      failed
      succeeded
    }
    tasks(ids: $tIds){
      name
      meta {
        title
        description
        URL
        userDefined
      }
      proxies(ids: $tIds){
        cyclePoint
        state
      }
    }
  }
}

Terminal

And a traceback to the terminal was consistent with it being an issue on state, emerging from the schema.py.

ERROR:graphql.execution.utils:Traceback (most recent call last):
  File "/home/h06/sbarth/miniconda3/envs/py3env/lib/python3.7/site-packages/promise/promise.py", line 842, in handle_future_result
    resolve(future.result())
  File "/net/home/h06/sbarth/cylc.git/cylc/flow/network/schema.py", line 263, in get_nodes_by_id
    return await resolvers.get_nodes_by_id(node_type, args)
  File "/net/home/h06/sbarth/cylc-uiserver/resolvers.py", line 65, in get_nodes_by_id
    return [node for node in nodes
  File "/net/home/h06/sbarth/cylc-uiserver/resolvers.py", line 67, in <listcomp>
    node_filter(node, args))]
  File "/net/home/h06/sbarth/cylc.git/cylc/flow/network/resolvers.py", line 90, in node_filter
    (args.get('ghosts') or node.state != '') and
graphql.error.located_error.GraphQLLocatedError: state

Release version(s) and/or repository branch(es) affected?
8.0a0.

Steps to reproduce the bug

  • Update all three repos to the current master branches.
  • Start up the UI Server & open the UI, as usual.
  • Run some suites.
  • Open them in Tree View, & see that the page is there but the table is missing, with no data there emerging.

Expected behavior

  • A Tree View (nested) table populated with its task data, for each suite.

Screenshots

Current master After reverting commit from #3211
Screenshot from 2019-07-13 19-02-04 Screenshot from 2019-07-13 19-06-10
@kinow
Copy link
Member

kinow commented Jul 15, 2019

@sadielbartholomew

I will look into the cause of those errors tomorrow, if someone else doesn't diagnose the problem before then (feel free to!).

Replying here so that after the PR is merged/closed the comment is still easy to find. I think this will have to wait @dwsutherland...

The query can be simplified to

query workflows($tIds: [ID]){
  workflows(ids: "five"){
    id
    tasks(ids: $tIds){
      name
    }
  }
}

And that will still produce the same error with state. The issue is that (and @dwsutherland can correct me later if I say anything silly here) the Query you are sending is for workflows, which resolves the tasks part with the resolver get_nodes_by_id and the arguments def_args.

The cylc-uiserver's resolver get_nodes_by_id will call cylc-flow's resolver with the same name. In cylc-flow's get_nodes_by_id, it will filter using the node state (node here is a PbTask in cylc-flow which gives the data for the Task in cylc-uiserver).

But in cylc-flow, a PbTask does not have a state attribute at the moment.

Testing

Here's how to get the query to work on your PR branch... though I am not confident this is a good fix.

diff --git a/cylc/flow/network/schema.py b/cylc/flow/network/schema.py
index 2fa0ae641..6c37a0e8d 100644
--- a/cylc/flow/network/schema.py
+++ b/cylc/flow/network/schema.py
@@ -157,6 +157,8 @@ def_args = dict(
     exids=List(ID, default_value=[]),
     mindepth=Int(default_value=-1),
     maxdepth=Int(default_value=-1),
+    states=List(String, default_value=[]),
+    exstates=List(String, default_value=[]),
 )
 
 all_def_args = dict(
@@ -166,6 +168,8 @@ all_def_args = dict(
     exids=List(ID, default_value=[]),
     mindepth=Int(default_value=-1),
     maxdepth=Int(default_value=-1),
+    states=List(String, default_value=[]),
+    exstates=List(String, default_value=[]),
 )
 
 proxy_args = dict(
diff --git a/cylc/flow/ws_messages.proto b/cylc/flow/ws_messages.proto
index 4a1ff7675..72497e272 100644
--- a/cylc/flow/ws_messages.proto
+++ b/cylc/flow/ws_messages.proto
@@ -135,6 +135,7 @@ message PbTask {
     int32 depth = 6;
     repeated string proxies = 7;
     repeated string namespace = 8;
+    string state = 9;
 }
 
 message PbPollTask {

Then, run protoc ws_messages.proto --python_out=. in the cylc.flow package directory, and re-install cylc-flow with something like pip install .. After this, your query should return (I have cylc run --no-detach --hold five running, so changed the ids to five):

{
  "data": {
    "workflows": [
      {
        "id": "kinow/five",
        "name": "five",
        "status": "held",
        "stateTotals": {
          "runahead": 0,
          "waiting": 0,
          "held": 3,
          "queued": 0,
          "expired": 0,
          "ready": 0,
          "submitFailed": 0,
          "submitRetrying": 0,
          "submitted": 0,
          "retrying": 0,
          "running": 0,
          "failed": 0,
          "succeeded": 0
        },
        "tasks": []
      }
    ]
  }
}

If you are working on a Cylc UI View that needs this data, you may try this quick fix in your local branch and continue the development (assuming it works for you), but it may be easier to work on another ticket and wait for @dwsutherland to return to look if there is a better fix.

Hope that helps
B

@dwsutherland dwsutherland mentioned this issue Jul 15, 2019
6 tasks
@dwsutherland
Copy link
Member

dwsutherland commented Jul 15, 2019

@sadielbartholomew - Thanks for raising this issue (and sorry for the trouble), I've created a pull request with the appropriate fix #3219 .

@kinow - Yes, it was an issue with trying to access state information of task nodes.. However task (& family) definitions are static data that only changes when the suite/workflow definition is updated (run/restart/reload), so tasks should never have a state.

@sadielbartholomew
Copy link
Collaborator Author

Great, thanks for the explanations @kinow & @dwsutherland, I'll ponder those so I can gain some understanding of what happened here & what the corresponding logic in the schema is trying to do.

I'll test the fix PR once you are ready for it to be reviewed (as a formal reviewer or otherwise).

@dwsutherland
Copy link
Member

Self-correction:

so tasks should never have a state.

It could have a state, by showing/adopting the highest priority state of it's cycle children (like we do with families), but that hasn't been requested yet XD

Great, thanks for the explanations @kinow & @dwsutherland, I'll ponder those so I can gain some understanding of what happened here & what the corresponding logic in the schema is trying to do.

Yes please! need more than just my own perspective on how things should be :-)

I'll test the fix PR once you are ready for it to be reviewed (as a formal reviewer or otherwise).

I think it's ready for review, quite a simple fix... For some reason the unit tests keep failing even though I cannot reproduce..

@kinow
Copy link
Member

kinow commented Jul 15, 2019

For some reason the unit tests keep failing even though I cannot reproduce..

Just kicked Travis. Looks like it got stuck before even running the tests. Weird.

@dwsutherland
Copy link
Member

For some reason the unit tests keep failing even though I cannot reproduce..

Just kicked Travis. Looks like it got stuck before even running the tests. Weird.

This change/fix, would in no way break existing tests XD ... Works by hand (both queries and pytest )

@kinow
Copy link
Member

kinow commented Jul 15, 2019

master giving the error for the query.

image

Query worked on this branch, with no errors in the console.

image

One strange failure in the unit tests, but not sure how that can be related.

=================================== FAILURES ===================================
___________________ TestHostAppointer.test_remove_bad_hosts ____________________
self = <flow.tests.test_host_appointer.TestHostAppointer testMethod=test_remove_bad_hosts>
    def test_remove_bad_hosts(self):
        """Test the '_remove_bad_hosts' method.
        Test using 'localhost' only since remote host functionality is
        contained only inside remote_cylc_cmd() so is outside of the scope
        of HostAppointer.
        """
        self.mock_global_config(set_hosts=['localhost'])
>       self.assertTrue(self.app._remove_bad_hosts().get('localhost', False))
E       AssertionError: False is not true
cylc/flow/tests/test_host_appointer.py:233: AssertionError
------------------------------ Captured log call -------------------------------
host_appointer.py          200 WARNING  can't get host metric from 'localhost'get-host-metrics  # returncode=1, err=Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/pkg_resources/__init__.py", line 583, in _build_master
    ws.require(__requires__)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/pkg_resources/__init__.py", line 900, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/pkg_resources/__init__.py", line 791, in resolve
    raise VersionConflict(dist, req).with_context(dependent_req)
pkg_resources.ContextualVersionConflict: (graphql-core 2.2 (/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages), Requirement.parse('graphql-core<2,>=0.5.0'), {'graphql-relay'})
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.7.1/bin/cylc-get-host-metrics", line 4, in <module>
    __import__('pkg_resources').require('cylc-flow==8.0a0')
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/pkg_resources/__init__.py", line 3191, in <module>
    @_call_aside
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/pkg_resources/__init__.py", line 3175, in _call_aside
    f(*args, **kwargs)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/pkg_resources/__init__.py", line 3204, in _initialize_master_working_set
    working_set = WorkingSet._build_master()
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/pkg_resources/__init__.py", line 585, in _build_master
    return cls._build_from_requirements(__requires__)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/pkg_resources/__init__.py", line 598, in _build_from_requirements
    dists = ws.resolve(reqs, Environment())
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/pkg_resources/__init__.py", line 791, in resolve
    raise VersionConflict(dist, req).with_context(dependent_req)
pkg_resources.ContextualVersionConflict: (graphql-core 2.2 (/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages), Requirement.parse('graphql-core<2,>=0.5.0'), {'graphql-relay'})

@dwsutherland
Copy link
Member

dwsutherland commented Jul 15, 2019

@kinow - Ah, looks like a graphql-core update causing it.. Will check for new version of graphene or set version of graphql-core

@kinow
Copy link
Member

kinow commented Jul 15, 2019

I can investigate it and update your branch too @dwsutherland :-) holiday!! 🌞 🎉

@dwsutherland
Copy link
Member

graphql-python/graphene#1031

@dwsutherland
Copy link
Member

@sadielbartholomew
Copy link
Collaborator Author

pkg_resources.ContextualVersionConflict: (graphql-core 2.2 ...

@wxtim has hit into this one too via Rose, on his Rose packaging branch metomi/rose#2366, Travis run. Thanks for commenting here as that meant we were aware of this & avoided investigation time for debugging it!

@kinow
Copy link
Member

kinow commented Jul 17, 2019

Comment from yesterday in the ticket about the wrong wheel

The wheel has been deleted. Sorry for any inconvenience this may have caused.

And version 2.1.7 of graphene has been release. Should we update it here too @dwsutherland ?

@dwsutherland
Copy link
Member

Comment from yesterday in the ticket about the wrong wheel

The wheel has been deleted. Sorry for any inconvenience this may have caused.

And version 2.1.7 of graphene has been release. Should we update it here too @dwsutherland ?

Ok Will do. Separate PR I suppose.

@kinow
Copy link
Member

kinow commented Jul 18, 2019

Your other PR passed, and fixed the issue with state reported in this issue. So looks like there's no hurry in updating graphene 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants