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

Refactor client/server & GraphQL workflow mutations #4529

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Nov 23, 2021

These changes partially address cylc/cylc-uiserver#271

Sibling: cylc/cylc-uiserver#290

  • Refactored the message format sent from server to client. Instead of just the raw result of the GraphQL mutation (or whatever request was sent1), the server now responds with a tuple of (content, error, user)
    • This allows errors to be returned over the ZMQ socket without mixing/confusing with the data content
    • user is included like this instead of adding it to the GraphQL {data: ..., errors: ...} response (the spec does not permit adding extra keys)
    • A NamedTuple called ResponseTuple is used instead of a regular tuple for better clarity and type safety
  • RefactoredGraphQL workflow mutation output type. Instead of the sometimes-inconsistent GenericResponse output which could be any object in practice, the output is now a graphene.List of GenericReponses, and GenericResponse has 3 fields: workflowId = graphene.String(), success = graphene.Boolean() and message = graphene.String()
    • This pins down what the expected response should be, which should avoid traceback issues like handling scheduler error cylc-uiserver#271
    • In practice this looks like:
      mutation {
        hold (
          workflows: ["temp4"]
          tasks: ["foo.2"]
        ) {
          results {
            workflowId
            success
            message
          }
        }
      }
      {
        "data": {
          "hold": {
            "results": [
              {
                "workflowId": "rdutta|temp4",
                "success": true,
                "message": "Command queued"
              }
            ]
          }
        }
      }

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.py and conda-environment.yml.
  • 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.

Footnotes

  1. The exception to this is that Protobuf results are sent back as-is in bytes format instead of the tuple, because bytes is not JSON-serializable

@MetRonnie MetRonnie added this to the cylc-8.0rc1 milestone Nov 23, 2021
@MetRonnie MetRonnie self-assigned this Nov 23, 2021
@MetRonnie MetRonnie marked this pull request as draft November 23, 2021 17:57
Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

So looks good.. Few minor comments below..

The main issue I'm having (when trying to run this and the UIS sibling), is getting it working
image
(even after reinstall of venvs)

Perhaps after a rebase, it would help, as there are breaking changes in this and master (?).

cylc/flow/network/__init__.py Outdated Show resolved Hide resolved
cylc/flow/network/__init__.py Outdated Show resolved Hide resolved
@@ -206,7 +219,7 @@ def parse_node_id(item, node_type=None):

# Field args (i.e. for queries etc):
class SortArgs(InputObjectType):
keys = List(String, default_value=['id'])
keys = graphene.List(String, default_value=['id'])
Copy link
Member

Choose a reason for hiding this comment

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

I originally had it graphene.List( but it was deemed unnecessary by reviewers.. I guess it can't be helped with typing..

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 it's an unfortunate clash. But seeing as typing is widely-used now I think it should take precedence

cylc/flow/network/schema.py Outdated Show resolved Hide resolved
@MetRonnie
Copy link
Member Author

MetRonnie commented Nov 24, 2021

So looks good.. Few minor comments below..

The main issue I'm having (when trying to run this and the UIS sibling), is getting it working

Sorry, should have mentioned: It's not working from the UI yet as some changes need to be made there. But it's working in GraphiQL.

@MetRonnie
Copy link
Member Author

MetRonnie commented Dec 14, 2021

Not going to be finished by Christmas so bumping to 8.0rc2

@MetRonnie MetRonnie modified the milestones: cylc-8.0.0, cylc-8.0rc2 Dec 14, 2021
@MetRonnie MetRonnie mentioned this pull request Feb 11, 2022
7 tasks
@MetRonnie MetRonnie added could be better Not exactly a bug, but not ideal. and removed WIP bug labels Aug 10, 2022
Don't unnecessarily process graphql errors
Tidy resolvers
Add integration test for WorkflowRuntimeClient.async_request() response format
Add unit tests for WorkflowRuntimeServer.receiver()
@MetRonnie MetRonnie modified the milestones: cylc-8.2.0, cylc-8.x Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
could be better Not exactly a bug, but not ideal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants