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

Node (Network) Coordinated Subscriptions (part 1/3) - Centralise REP and PUB into single runtime server #4274

Merged

Conversation

dwsutherland
Copy link
Member

@dwsutherland dwsutherland commented Jun 18, 2021

This change:

This PR completes step one of the following changes needed for GraphQL subscriptions at the workflow:

  1. Centralise REP and PUB into single runtime server.
  2. Setup coordination scheme for REQ of PUB topics to SUB to.
  3. Implement a GraphQL-WS style handler for GraphQL subscriptions.

In order to coordinate between PUB and REP sockets it makes sense to revive the single runtime server concept to initialize both and manage requests (REQ) for publishing (PUB) topics to subscribe (SUB) to.

  • Step 1 - Can now publish in response to a REQ (in addition to REP).

This also results in less network code in the scheduler.

At present we have each network object (publisher, replier/server) instantiated by the scheduler in the main thread, then their startup/socket-bind happening in a separate thread:
image

This PR reduces the number of threads, with the publisher, subscriber coordinated in the same thread by the parent server object (instantiated by the scheduler still):
image

Published articles/data/deltas are now put into a (threadsafe) queue, instead of called directly by the main thread...

Since the threading is setup by the scheduler now, the client no longer needs to import the threading library.

Running the complex workflow takes about the same time as the current master:

        Command being timed: "cylc play complex --color=never -n"
        User time (seconds): 515.45
        System time (seconds): 36.81
        Percent of CPU this job got: 36%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 25:18.28
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 117568
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 20396247
        Voluntary context switches: 182301
        Involuntary context switches: 172133
        Swaps: 0
        File system inputs: 8
        File system outputs: 3725008
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

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

@dwsutherland dwsutherland added efficiency For notable efficiency improvements WIP labels Jun 18, 2021
@dwsutherland dwsutherland added this to the cylc-8.0.0 milestone Jun 18, 2021
@dwsutherland dwsutherland self-assigned this Jun 18, 2021
@dwsutherland dwsutherland added the POC Proof of Concept label Jun 19, 2021
@dwsutherland dwsutherland force-pushed the node-coordinated-subscriptions branch 2 times, most recently from ce6fa2f to c871eb7 Compare June 22, 2021 05:26
@hjoliver hjoliver modified the milestones: cylc-8.0rc1, cylc-8.0.0 Nov 30, 2021
@dwsutherland dwsutherland changed the title Node (Network) Coordinated Subscriptions - GraphQL subs and Efficiency Node (Network) Coordinated Subscriptions (part 1/3) - Centralise REP and PUB into single runtime server Feb 11, 2022
@dwsutherland dwsutherland removed the WIP label Feb 11, 2022
@dwsutherland dwsutherland marked this pull request as ready for review February 11, 2022 01:16
@dwsutherland dwsutherland removed the POC Proof of Concept label Feb 11, 2022
@dwsutherland dwsutherland modified the milestones: cylc-8.0.0, cylc-8.0rc2 Feb 11, 2022
@MetRonnie MetRonnie self-requested a review February 11, 2022 10:28
@dwsutherland dwsutherland force-pushed the node-coordinated-subscriptions branch 2 times, most recently from 0ad693c to 988bde4 Compare February 16, 2022 04:50
@dwsutherland dwsutherland force-pushed the node-coordinated-subscriptions branch 5 times, most recently from 4b5a474 to f06ca20 Compare March 11, 2022 05:12
@dwsutherland dwsutherland force-pushed the node-coordinated-subscriptions branch 3 times, most recently from 349bd08 to 0a9306d Compare March 17, 2022 22:55
@dwsutherland dwsutherland force-pushed the node-coordinated-subscriptions branch 4 times, most recently from 9ff56e8 to 71da6cb Compare April 14, 2022 06:47
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looks good, need to go through this one very thoroughly and do some hands-on-testing, so far so good.

cylc/flow/network/__init__.py Outdated Show resolved Hide resolved
cylc/flow/network/server.py Show resolved Hide resolved
cylc/flow/scheduler.py Show resolved Hide resolved
@hjoliver
Copy link
Member

hjoliver commented Jun 1, 2022

This also results in less network code in the scheduler.

Well that's nice 😁

@dwsutherland dwsutherland force-pushed the node-coordinated-subscriptions branch from 38eb201 to 3a4fb6e Compare June 21, 2022 00:24
@dwsutherland dwsutherland force-pushed the node-coordinated-subscriptions branch 3 times, most recently from 3549c54 to 25dd454 Compare July 1, 2022 22:31
@dwsutherland dwsutherland force-pushed the node-coordinated-subscriptions branch from 25dd454 to 80bcdd8 Compare July 4, 2022 02:12
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Works great, cheers.

I've poked and prodded this as best I'm able, works great.


Args:
message (dict): message contents
"""
# TODO: If requested, coordinate publishing response/stream.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# TODO: If requested, coordinate publishing response/stream.
# TODO: If requested, coordinate publishing response/stream.
# https://github.com/cylc/cylc-flow/issues/3329

@oliver-sanders oliver-sanders merged commit e2256b4 into cylc:master Jul 4, 2022
@hjoliver hjoliver modified the milestones: cylc-8.0rc4, cylc-8.0.0 Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
efficiency For notable efficiency improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pytest Unhandled Thread Exception Warning Calling publisher coroutine from another thread
4 participants