Api on the fly - GraphQL schema #3469

oliver-sanders · 2019-12-18T21:20:53Z

Addresses the data-provision component of #2123

Not ready for review, putting this up early as it effects downstream activities.

Improve the GraphQL schema so that it contains all of the information required to auto-generate interfaces (e.g. HTML forms or CLI option parsers).

Split the TaskActions mutation into separate mutations:
- So we can auto-generate an interface for each mutation individually.
- So we can document each mutation individually.
- So we can assign options/arguments to each mutation individually.
Combine the many hold_*, release_*, broadcast_* and stop_* methods
- So they form a more logical single interface
- (and also just for niceness)
- Still need to re-implement:
  - expire broadcast
  - display broadcast (do we need a mutation for this, a query should do, but what about the CLI)?
Create new input types for mutation arguments
- So we know the meaning of fields and can handle them accordingly (e.g. validate and auto-complete cycle points in an HTML form).
- These new types mostly just extend the built-in ones with no modifications, it's just for documentation and name-spacing.
Use enumerations where applicable.
- We can use these to auto-generate options lists client side.
- Also permits centralisation of documentation.

This PR pushes mutations through the same cylc.flow.network.server.SuiteRuntimeServer interface as the CLI currently uses. This is temporary, hence the docstrings have been left unchanged, this interface will be removed partially/entirely when the CLI is migrated to use the mutations interface.

Example Mutation:

mutation {
  stop(workflows: ["sanderso|generic"], mode: Now) {
    result
  }
}

Caveats:

A little messy but much of the mess will get deleted when the CLI is refactored.
Not quite complete but we can fill in the gaps later.
Not quite perfectly documented but the documentation needs a review anyway.

TODO:

Standardise argument names.
Improve documentation.
~~Testing.~~ Mutation testing will come for free once the CLI is converted, replicating these tests will be a lot of work.
Document enumerations.
Review required/optional arguments.

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).
Appropriate tests are included (unit and/or functional).
Appropriate change log entry included.
(master branch) I have opened a documentation PR at cylc/cylc-doc/pull/XXXX.

dwsutherland · 2019-12-19T03:57:59Z

Looks good so far @oliver-sanders 👍

oliver-sanders · 2019-12-19T19:53:16Z

Thanks for logging it over, will clean up and submit.

oliver-sanders · 2020-01-03T05:55:09Z

@dwsutherland the last commit is a little more aggressive, aimed at making the mutations better match the CLI e.g. by merging the various broadcast methods. Also copied some docs over from the CLI.

oliver-sanders · 2020-01-05T21:05:59Z

I've introduced new "input types" which are more declarative about the meaning of certain types i.e. cycle point is now a special field not just a string (well its still a string underneath but now it's called a cycle point). I've used these as inputs (i.e. arguments to mutators) but haven't gone the whole hog and used them for outputs (i.e. data type returned by GraphQL query/subscription) as this would be a little more destructive, work for later perhaps.

cylc/flow/network/schema.py

dwsutherland · 2020-01-08T20:46:40Z

@dwsutherland the last commit is a little more aggressive, aimed at making the mutations better match the CLI e.g. by merging the various broadcast methods. Also copied some docs over from the CLI.

Looks good to me.

I've introduced new "input types" which are more declarative about the meaning of certain types i.e. cycle point is now a special field not just a string (well its still a string underneath but now it's called a cycle point). I've used these as inputs (i.e. arguments to mutators) but haven't gone the whole hog and used them for outputs (i.e. data type returned by GraphQL query/subscription) as this would be a little more destructive, work for later perhaps.

Yes, there's definitely more we could do:

Refactor Query/Subscriptions to be more explicit about ObjectType fields (although there still needs to be a clear mapping to data-store, and the data element definitions are probably clear enough as they are so more descriptions would likely be enough) and possibly InputObjectType.
Once the CLI uses GraphQL, refactor the command scheduler command methods to match the GraphQL layout (and remove the ugly mapping in the resolvers.py)..
Others..

Happy to approve once tests and end-to-end Cylc is working.

cylc/flow/network/schema.py

cylc/flow/network/resolvers.py

* collect stop, hold, broadcast * use enumerations where appropriate * route GQL mutations via the traditional endpoints this is a temporary interface for simplicity it can be torn down later

oliver-sanders · 2020-01-22T01:39:28Z

Okay, should be good to go...

dwsutherland

Tested and working! GraphiQL via UIS works, didn't try all the mutations, but a few 👍

For me this is pretty much good to go, although I would like to hear your rationale on using server endpoints/methods bound for deprecation in your mapping to internal commands by the GraphQL end-point.

cylc/flow/network/resolvers.py

cylc/flow/network/schema.py

cylc/flow/scheduler.py

cylc/flow/scripts/cylc_ext_trigger.py

oliver-sanders · 2020-01-28T01:39:52Z

I would like to hear your rationale on using server endpoints/methods bound for deprecation in your mapping to internal commands by the GraphQL end-point.

So we only have one set of endpoints
The server endpoints have authorisation
Makes it clearer in my head what we need to do to make the CLI conform to the new interface
Doesn't really matter, it's all going to get re-written anyway

dwsutherland · 2020-01-28T01:49:39Z

I would like to hear your rationale on using server endpoints/methods bound for deprecation in your mapping to internal commands by the GraphQL end-point.

So we only have one set of endpoints

Not really sure what you mean here... There is one set with or without this method of mapping hitting them twice before going to the internal commands..

The server endpoints have authorisation

Makes it clearer in my head what we need to do to make the CLI conform to the new interface

Doesn't really matter, it's all going to get re-written anyway

So it's a temporary measure to avoid the lack of fine-grained authorisation (because the graphql endpoint does have authorisation, but will be in the form of middleware at some point presumably) in the current graphql endpoint?

I agree, it's definitely a more obvious guide of how to rewrite the internal Scheduler commands.

Ok.. satisfied.

oliver-sanders · 2020-01-28T01:50:10Z

Sorry I should have explained at the outset, this is not how I think it should be done! Just didn't have the guts to go whole hog in this PR.

dwsutherland · 2020-01-28T02:00:10Z

The server endpoints have authorisation
So it's a temporary measure to avoid the lack of fine-grained authorisation (because the graphql endpoint does have authorisation, but will be in the form of middleware at some point presumably) in the current graphql endpoint?

You use getuser():
https://github.com/oliver-sanders/cylc/blob/5ee4c555afc542c95adf54b3240414f19636b224/cylc/flow/network/resolvers.py#L346-L349
Doesn't that use the suite owner as the user:
https://docs.python.org/3.8/library/getpass.html#getpass.getuser

So it gives no extra authorisation, anyway?

oliver-sanders · 2020-01-28T02:08:01Z

So it gives no extra authentication, anyway?

Not authentication, authorisation.

Each of the server endpoints are protected by an authorisation method which provides fine-ish-grained-authorisation. This is the vestigial remains of the old Cylc7 system, it's effectively inactive as at the moment we always assume an authenticated user is the suite owner but anyway.

I'm not fussed about the server interface (as it's going to get torn up anyway), just don't want to re-write it in this PR because it's a blocker to aotf and the alpha2 release.

* this client-side logic doesn't fit into aoft very nicely * will require a bespoke solution later, perhaps an interface?

oliver-sanders · 2020-01-28T02:18:03Z

FYI the codacy error is kinda un-avoidable* as the function is now tethered to the mutation interface so the argument names must match. The id built-in is overridden but only within the local scope of the function (within which id is not used).

dwsutherland · 2020-01-28T02:40:09Z

So it gives no extra authentication, anyway?

Not authentication, authorisation.

Yes, corrected that ☝️
So the scenic route to the internal commands via the server endpoints (apart form adding more function calls) is a way to tidy it up and instruct on what changes are needed in the Scheduler for their removal?... Like you said; it's all gonna be re-written anyway.

FYI the codacy error is kinda un-avoidable* as the function is now tethered to the mutation interface so the argument names must match. The id built-in is overridden but only within the local scope of the function (within which id is not used).

Roger that.

dwsutherland

Happy to move forward, and merge this once Travis CI passes (queries have been answered).

kinow

A few docstrings need updating, but no blocker, can be done in this PR or later.

@oliver-sanders thanks for the new comments, that'll be very helpful when working on the UI too.

I think this can close #3340 ? WDYT?

@oliver-sanders I also tested it with the following setup:

Install this PR in a venv
cd ../cylc-uiserver && pip install -e . then cd -; pip install -e. (i.e. install Cylc UI Server and re-install cylc-flow from this branch)
Copied jupyterhub_config.py and changed static files location
jupyterhub
Launched suite five
Executed the following mutation:

mutation {
  stop(workflows: ["five"]) {
    result
  }
}

The suite did not stop, and also got the following exception in the terminal:

2020-01-29 16:42:59,644 tornado.application ERROR    Exception in callback functools.partial(<bound method IOLoop._discard_future_result of <tornado.platform.asyncio.AsyncIOMainLoop object at 0x7fb3a2cedfd0>>, <Task finished coro=<WorkflowsManager.gather_workflows() done, defined at /home/kinow/Development/python/workspace/cylc-uiserver/cylc/uiserver/workflows_mgr.py:112> exception=KeyError('CYLC_SUITE_PUBLISH_PORT')>)
Traceback (most recent call last):
  File "/home/kinow/Development/python/workspace/cylc-flow/venv/lib/python3.7/site-packages/tornado/ioloop.py", line 743, in _run_callback
    ret = callback()
  File "/home/kinow/Development/python/workspace/cylc-flow/venv/lib/python3.7/site-packages/tornado/ioloop.py", line 767, in _discard_future_result
    future.result()
  File "/home/kinow/Development/python/workspace/cylc-uiserver/cylc/uiserver/workflows_mgr.py", line 125, in gather_workflows
    for arg in scan_args:
  File "/home/kinow/Development/python/workspace/cylc-uiserver/cylc/uiserver/workflows_mgr.py", line 117, in <genexpr>
    (reg, host, port, pub_port, self.context, CLIENT_TIMEOUT)
  File "/home/kinow/Development/python/workspace/cylc-flow/cylc/flow/network/scan.py", line 283, in get_scan_items_from_fs
    contact_data[ContactFileFields.PUBLISH_PORT],
KeyError: 'CYLC_SUITE_PUBLISH_PORT'

Is that expected? Maybe I have something wrong with my setup, or forgot to do something? Just let me know and I'll approve if there's nothing wrong here 👍 everything else looks good.

Thanks

kinow · 2020-01-29T02:56:47Z

cylc/flow/tests/network/test_resolvers.py

-                no_check=False
+                check_point=True


cylc/flow/network/schema.py

cylc/flow/network/server.py

kinow · 2020-01-29T03:26:40Z

cylc/flow/network/server.py

@@ -858,12 +921,12 @@ def put_broadcast(

    @authorise(Priv.CONTROL)
    @expose
-    def put_ext_trigger(self, event_message, event_id):
+    def put_ext_trigger(self, message, id):


id is also a built-in, but I think it's harmless here.

So the function signature is bound to the mutation interface. The mutation has an argument called id so the method must have one too.

We can use a **kwargs hack here but as you say it's harmless.

Codacy has flagged this as an error, "redefining a built-in". I suppose we should really change the mutation interface as well. (Can be a follow-up though).

cylc/flow/network/server.py

cylc/flow/scripts/cylc_insert.py

cylc/flow/network/server.py

oliver-sanders · 2020-01-29T05:31:21Z

Thanks @kinow, the docs aren't perfect ATM, (mostly just cut-n-paste from the CLI) so I think leave #3340 open as a fuller "documentation review" to undertake once the dust has settled.

hjoliver · 2020-01-29T20:06:18Z

Here is an overview of what got changed by this pull request:

Issues
======
- Added 1

See the complete overview on Codacy

hjoliver · 2020-01-29T21:20:38Z

Still have a few unchecked requirements in the description above. E.g. test coverage?

oliver-sanders · 2020-01-29T21:22:49Z

Not so sure what to do about test coverage.

We would need to duplicate every existing cylc subcommand test. Once the CLI is migrated it will be covered by existing tests...

hjoliver · 2020-01-29T21:24:11Z

Not so sure what to do about test coverage.

We would need to duplicate every existing cylc subcommand test. Once the CLI is migrated it will be covered by existing tests...

OK that's fine.

hjoliver · 2020-01-29T21:24:48Z

So if @kinow approves now, we can merge this.

oliver-sanders added the WIP label Dec 18, 2019

oliver-sanders self-assigned this Dec 18, 2019

oliver-sanders changed the title ~~Api on the fly~~ Api on the fly - QraphQL schema Dec 19, 2019

oliver-sanders force-pushed the api-on-the-fly branch from ef1e2ba to 08bea55 Compare January 3, 2020 05:52

kinow changed the title ~~Api on the fly - QraphQL schema~~ Api on the fly - GraphQL schema Jan 3, 2020

oliver-sanders force-pushed the api-on-the-fly branch from 08bea55 to 11247e6 Compare January 5, 2020 20:58

dwsutherland reviewed Jan 8, 2020

View reviewed changes

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

This was referenced Jan 9, 2020

api-on-the-fly cylc/cylc-ui#339

Closed

api on the fly - part 1 - generate forms from mutations cylc/cylc-ui#357

Merged

api-on-the-fly - part 2 - mutation association cylc/cylc-ui#359

Merged

oliver-sanders force-pushed the api-on-the-fly branch 3 times, most recently from a03d42a to 34b7a04 Compare January 14, 2020 23:33

oliver-sanders commented Jan 15, 2020

View reviewed changes

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

dwsutherland reviewed Jan 15, 2020

View reviewed changes

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

oliver-sanders force-pushed the api-on-the-fly branch from ad21693 to e060d12 Compare January 15, 2020 22:40

oliver-sanders added this to the cylc-8.0a2 milestone Jan 16, 2020

oliver-sanders force-pushed the api-on-the-fly branch 2 times, most recently from 3ec35ff to cd200bf Compare January 21, 2020 22:47

oliver-sanders added 2 commits January 22, 2020 14:33

aotf: make graphql mutation arguments more declaritive

da9dd63

aotf: bring mutations inline with cli

c61e77e

* collect stop, hold, broadcast * use enumerations where appropriate * route GQL mutations via the traditional endpoints this is a temporary interface for simplicity it can be torn down later

oliver-sanders force-pushed the api-on-the-fly branch from cd200bf to f90d4dd Compare January 22, 2020 01:38

oliver-sanders marked this pull request as ready for review January 22, 2020 01:39

oliver-sanders requested a review from dwsutherland January 22, 2020 01:39

oliver-sanders mentioned this pull request Jan 24, 2020

api-on-the-fly - part 4 - mutations cylc/cylc-ui#385

Merged

10 tasks

oliver-sanders force-pushed the api-on-the-fly branch from 8b17d56 to 5ee4c55 Compare January 28, 2020 00:54

dwsutherland requested changes Jan 28, 2020

View reviewed changes

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

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

cylc/flow/scheduler.py Show resolved Hide resolved

cylc/flow/scripts/cylc_ext_trigger.py Show resolved Hide resolved

aotf: disable the files field of the broadcast mutation

29a1775

* this client-side logic doesn't fit into aoft very nicely * will require a bespoke solution later, perhaps an interface?

oliver-sanders force-pushed the api-on-the-fly branch from 5ee4c55 to 29a1775 Compare January 28, 2020 02:12

dwsutherland approved these changes Jan 28, 2020

View reviewed changes

oliver-sanders requested a review from kinow January 28, 2020 21:16

kinow reviewed Jan 29, 2020

View reviewed changes

doc fixes

b961362

oliver-sanders force-pushed the api-on-the-fly branch from 3ffcd63 to b961362 Compare January 29, 2020 20:03

kinow approved these changes Jan 29, 2020

View reviewed changes

kinow merged commit 95371b8 into cylc:master Jan 29, 2020

oliver-sanders deleted the api-on-the-fly branch January 29, 2020 22:02

oliver-sanders mentioned this pull request Jan 29, 2020

Suite API - On-The-Fly #2123

Open

3 tasks

oliver-sanders restored the api-on-the-fly branch March 22, 2020 20:52

This was referenced Feb 1, 2022

Pass authenticated user to cylc-flow cylc/cylc-uiserver#288

Merged

Log Authenticated User from UI-Server in Workflow Log #4522

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api on the fly - GraphQL schema #3469

Api on the fly - GraphQL schema #3469

oliver-sanders commented Dec 18, 2019 •

edited

Loading

dwsutherland commented Dec 19, 2019

oliver-sanders commented Dec 19, 2019

oliver-sanders commented Jan 3, 2020

oliver-sanders commented Jan 5, 2020

dwsutherland commented Jan 8, 2020

oliver-sanders commented Jan 22, 2020

dwsutherland left a comment

oliver-sanders commented Jan 28, 2020

dwsutherland commented Jan 28, 2020 •

edited

Loading

oliver-sanders commented Jan 28, 2020 •

edited

Loading

dwsutherland commented Jan 28, 2020 •

edited

Loading

oliver-sanders commented Jan 28, 2020 •

edited

Loading

oliver-sanders commented Jan 28, 2020

dwsutherland commented Jan 28, 2020

dwsutherland left a comment

kinow left a comment

kinow Jan 29, 2020

kinow Jan 29, 2020

oliver-sanders Jan 29, 2020

hjoliver Jan 29, 2020

oliver-sanders commented Jan 29, 2020

hjoliver commented Jan 29, 2020

hjoliver commented Jan 29, 2020

oliver-sanders commented Jan 29, 2020

hjoliver commented Jan 29, 2020

hjoliver commented Jan 29, 2020

Api on the fly - GraphQL schema #3469

Api on the fly - GraphQL schema #3469

Conversation

oliver-sanders commented Dec 18, 2019 • edited Loading

dwsutherland commented Dec 19, 2019

oliver-sanders commented Dec 19, 2019

oliver-sanders commented Jan 3, 2020

oliver-sanders commented Jan 5, 2020

dwsutherland commented Jan 8, 2020

oliver-sanders commented Jan 22, 2020

dwsutherland left a comment

Choose a reason for hiding this comment

oliver-sanders commented Jan 28, 2020

dwsutherland commented Jan 28, 2020 • edited Loading

oliver-sanders commented Jan 28, 2020 • edited Loading

dwsutherland commented Jan 28, 2020 • edited Loading

oliver-sanders commented Jan 28, 2020 • edited Loading

oliver-sanders commented Jan 28, 2020

dwsutherland commented Jan 28, 2020

dwsutherland left a comment

Choose a reason for hiding this comment

kinow left a comment

Choose a reason for hiding this comment

kinow Jan 29, 2020

Choose a reason for hiding this comment

kinow Jan 29, 2020

Choose a reason for hiding this comment

oliver-sanders Jan 29, 2020

Choose a reason for hiding this comment

hjoliver Jan 29, 2020

Choose a reason for hiding this comment

oliver-sanders commented Jan 29, 2020

hjoliver commented Jan 29, 2020

hjoliver commented Jan 29, 2020

oliver-sanders commented Jan 29, 2020

hjoliver commented Jan 29, 2020

hjoliver commented Jan 29, 2020

oliver-sanders commented Dec 18, 2019 •

edited

Loading

dwsutherland commented Jan 28, 2020 •

edited

Loading

oliver-sanders commented Jan 28, 2020 •

edited

Loading

dwsutherland commented Jan 28, 2020 •

edited

Loading

oliver-sanders commented Jan 28, 2020 •

edited

Loading