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

Client server article #17

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

RoboDoig
Copy link
Collaborator

This PR contains an article added to the ZeroMQ docs that describes the construction of a workflow running a simple client-server network architecture.

Loosely related to issue #15.

@CLAassistant
Copy link

CLAassistant commented Nov 7, 2023

CLA assistant check
All committers have signed the CLA.

@banchan86
Copy link
Contributor

Hi @glopesdev I would like to update the zeromq to the new docs theme but there's this article that hasn't been merged in yet, can you review and merge it in before I do the update?

@glopesdev
Copy link
Member

@banchan86 @RoboDoig now that the docs infrastructure has been updated, it would probably be best to rebase this PR and build off that. There is no immediate rush though, just leaving this here as a note.

@RoboDoig RoboDoig force-pushed the client-server-article branch from 3a18a02 to fde25b4 Compare January 13, 2025 11:48
@RoboDoig
Copy link
Collaborator Author

I've done the rebase, I'm just going to run through and check that all the steps still work with the current version of ZeroMQ. Please hold off on merging until then!

@RoboDoig
Copy link
Collaborator Author

@banchan86 @glopesdev all looks good now

@RoboDoig
Copy link
Collaborator Author

Note to self: svgs not required anymore, test locally with build script and delete the svgs

@RoboDoig
Copy link
Collaborator Author

RoboDoig commented Jan 14, 2025

@glopesdev @banchan86 in the docs build script, should the argument for Export-Image here be pointing to the artifacts\release folder?

@glopesdev
Copy link
Member

@RoboDoig It should, this was my mistake when I updated to the new artifacts build layout.

I will create a new PR soon to both fix this along with introducing separate member pages.

@banchan86
Copy link
Contributor

banchan86 commented Jan 16, 2025

Hey @RoboDoig! I'm not assigned as a reviewer yet but I can leave several comments:

  • Consider moving the client server article to a new Tutorials subsection of the website

  • Local .bonsai environment needs to be updated with the Windows Input library as the exported SVGs are missing the KeyDown operator

  • Consider breaking out the steps into bullet points, and use note alert boxes to highlight key explanations. For example, you can follow https://bonsai-rx.org/docs/tutorials/state-machines.html. This helps break up the walls of text and makes it slightly more easier to follow.

  • Reformat operator formatting to make it more distinguishable from properties.

Instead of doing

[**`Dealer`**](xref:Bonsai.ZeroMQ.Dealer)

which looks like this

image

do

[`Dealer`](xref:Bonsai.ZeroMQ.Dealer)

Which will look like this in the newer docfx template. I think the formatting might have changed from the old docfx template which you were used to, this was the last site to be migrated haha.

image

  • Add links to pattern articles when mentioning them

Router-Dealer

  • there is a spelling error - "server feedbasck"

@RoboDoig
Copy link
Collaborator Author

@banchan86 - would you like it to be moved to a new Tutorials section at the top-level (e.g. Manual, Reference, Tutorials) or a Tutorials subsection in the Manual (along with Recipes)?

@RoboDoig
Copy link
Collaborator Author

Note to self - replace references to 'node' with 'operator'

@RoboDoig
Copy link
Collaborator Author

I definitely have ideas for other tutorials but no immediate plans to write them up. My feeling is that even if there is only one tutorial it would be nice to stay consistent with the main bonsai-rx documentation so it's a bit more intuitive to navigate.

@RoboDoig
Copy link
Collaborator Author

@banchan86 - Added a separate tutorials section

Copy link
Contributor

@banchan86 banchan86 left a comment

Choose a reason for hiding this comment

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

Did an initial review of the first half, will need to digest more to finish it! Left some initial comments if you want to work on it if not can wait till I finish my review. One thing I did notice though was that the PR history looks a bit weird, not sure if you want to fix it for a cleaner PR history before adding more commits. Looks like some of the commits were repeated.

docs/tutorials/client-server.md Outdated Show resolved Hide resolved
docs/tutorials/client-server.md Outdated Show resolved Hide resolved
docs/tutorials/client-server.md Outdated Show resolved Hide resolved
docs/tutorials/client-server.md Outdated Show resolved Hide resolved
docs/tutorials/client-server.md Outdated Show resolved Hide resolved
docs/tutorials/client-server.md Outdated Show resolved Hide resolved
@RoboDoig RoboDoig requested a review from banchan86 January 24, 2025 10:15
Copy link
Contributor

@banchan86 banchan86 left a comment

Choose a reason for hiding this comment

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

Hey @RoboDoig article is looking better! I just have some suggested changes to one section that I saw some duplication in and that the steps were a little long. The rest of the text looks fine to me, and the examples I tested work, so that might be all from my side!

docs/tutorials/client-server.md Outdated Show resolved Hide resolved
docs/tutorials/client-server.md Outdated Show resolved Hide resolved
docs/tutorials/client-server.md Outdated Show resolved Hide resolved
docs/tutorials/client-server.md Outdated Show resolved Hide resolved
docs/tutorials/client-server.md Outdated Show resolved Hide resolved
@RoboDoig
Copy link
Collaborator Author

@banchan86 - thank you again for your comments! I've added most of those suggestions, modified slightly here and there.

@RoboDoig RoboDoig requested a review from banchan86 January 29, 2025 10:28
Copy link
Contributor

@banchan86 banchan86 left a comment

Choose a reason for hiding this comment

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

Article looks good! Just a few comments about the infrastructure changes

  1. In the .bonsai local env add *.exe.settings to the .gitignore, and manually remove the .exe.settings from git tracking.
  2. I think you shouldn't need to add Bonsai.ZeroMQ and its dependencies to the .bonsai local environment, as the svg export in docs/build.ps1 should be using the built package in the artifacts folder.

@RoboDoig
Copy link
Collaborator Author

Yes you're right, forgot to remove those after doing some debugging related to something else, will make that change now

@glopesdev glopesdev force-pushed the client-server-article branch from b79e548 to c326648 Compare February 2, 2025 10:54
Copy link
Member

@glopesdev glopesdev left a comment

Choose a reason for hiding this comment

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

@RoboDoig I have rebased the branch to avoid adding unnecessary changes to review from an interim merge. I have kept a copy of the original branch, so let me know if you are unable to work on the branch and we can figure it out, one way of the other.

@@ -4,7 +4,7 @@
"src": [
{
"files": [
"Bonsai.ZeroMQ/*.csproj"
"Bonsai.ZeroMQ/Bonsai.ZeroMQ.csproj"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to specify the .csproj file name directly? There shouldn't be more than one file at this location, or could it be perhaps a backup? I usually have this convention to avoid having to type the name of the project twice, but maybe there is a good reason to be more explicit.

@@ -64,7 +70,6 @@
"_enableNewTab": true,
"_enableSearch": true,
"_gitContribute": {
"repo": "https://github.com/bonsai-rx/zeromq",
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to remove the explicit repo URL? Reading the docfx docs it seems like docfx does use automatically the current repo if it is not specified, so maybe there isn't a need to have it, but just confirming.

If we do go for this, it might be better to make this change in a separate PR, just to avoid mixing unrelated configuration changes from the article development.

Comment on lines +1 to +7
---
uid: client-server
title: "Client-Server"
---

Client-server tutorial
======================
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
---
uid: client-server
title: "Client-Server"
---
Client-server tutorial
======================
Client-Server
=============

If we set the main article header to the desired article name, then we do not need to specify the article title and uid as it should be inferred correctly from the title and file name, respectively.

I guess we did use this explicit scheme in the first version of the articles, so we could keep it now for consistency and change in a separate PR, but maybe we can consider changing the main header text?

Comment on lines +9 to +13
The Bonsai.ZeroMQ package allows us to harness the powerful [ZeroMQ](https://zeromq.org/) library to build networked applications in Bonsai. Applications could include:
- Interfacing with remote experimental rigs via network messages
- Performing distributed work across pools of machines (e.g. for computationally expensive deep-learning inference)
- Streaming video data between clients across a network
- **Real-time interaction between clients in a multiplayer game**
Copy link
Member

Choose a reason for hiding this comment

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

This bit seems like a summary of the ZeroMQ library. Do we need to include it as part of the article? Maybe some of these examples could be interesting to move to the intro itself?

In this article, we will use Bonsai.ZeroMQ to explore this final example and build a basic client-server architecture similar to one that might be used in a multiplayer game.

## Network design
The basic network architecture we want to achieve will be composed of a number of clients sending their state to a server, which then updates the other connected clients with that clients’ state. This is comparable to a multiplayer game in which client players move through the game world and must synchronise that movement via a dedicated server so that all players see each other in their ‘true’ position in the world.
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
The basic network architecture we want to achieve will be composed of a number of clients sending their state to a server, which then updates the other connected clients with that clients’ state. This is comparable to a multiplayer game in which client players move through the game world and must synchronise that movement via a dedicated server so that all players see each other in their ‘true’ position in the world.
The basic network architecture we want to achieve will be composed of a number of clients sending their state to a server, which then updates the other connected clients with that clients’ state. This is comparable to a multiplayer game in which players connect to a game server, move through a shared game world, and must synchronise that movement via a dedicated server so that all players see each other in their ‘true’ position in the world.

![Basic server response](~/workflows/server-basic-response.bonsai)
:::

The [`SendResponse`](xref:Bonsai.ZeroMQ.SendResponse) operator has a couple of interesting properties which may not be immediately obvious from this simple example. First, this operator always transmits its response back to the ZeroMQ socket that initiated the request (in this case one of our [`Dealer`](xref:Bonsai.ZeroMQ.Dealer) clients) and we therefore do not need to specify an address in its processing logic. Second, the internal flow of [`SendResponse`](xref:Bonsai.ZeroMQ.SendResponse) logic is computed asynchronously. This is very useful for responses that require more intensive computation and allows a [`Router`](xref:Bonsai.ZeroMQ.Router) to deal with frequent incoming [`Dealer`](xref:Bonsai.ZeroMQ.Dealer) requests efficiently.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if an analogy or connection with SelectMany should be made here, explaining that it is an operator that also runs in parallel for each request, but with an additional side-effect of sending the response back to the client.

> [!Note]
> Imagine, for example, that our Dealer sockets were sending video snippets to a Router server that is tasked with doing some processing of the video and returning the results back to the Dealers. If the responses were not computed in an asynchronous manner we would start to incur a bottleneck on the router if there were many connected Dealers or frequent Dealer requests.

Running this workflow, you should see a 'bounceback' where any [`Dealer`](xref:Bonsai.ZeroMQ.Dealer) client that sends a message receives a reply from the [`Router`](xref:Bonsai.ZeroMQ.Router) server. However, in order to address these messages to specific other clients we need to take a slightly different approach. Instead of using the [`SendResponse`](xref:Bonsai.ZeroMQ.SendResponse) operator we are going to build messages ourselves and pass them directly into the [`Router`](xref:Bonsai.ZeroMQ.Router).
Copy link
Member

Choose a reason for hiding this comment

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

I guess adding the previous suggestion could help explain why we are going the route of SelectMany in this case.

Now, our `SelectAllClients` will produce a sequence of all unique client addresses every time the server receives a message. Connect the output of `SelectAllClients` to a [`WithLatestFrom`](xref:Bonsai.Reactive.WithLatestFrom) with the [`Router`](xref:Bonsai.ZeroMQ.Router) as its second input. In this context [`WithLatestFrom`](xref:Bonsai.Reactive.WithLatestFrom) combines each client address from `SelectAllClients` with the most recent received message. The result is that when a message is received from the client, we produce several copies of the message 'addressed' to each connected client.

:::workflow
![Select all clients and package message](~/workflows/format-select-all-clients.bonsai)
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this workflow has a disabled or invalid BounceBack node which perhaps should be removed to avoid confusion if not needed?

To apply the logic of the [`SelectMany`](xref:Bonsai.Reactive.SelectMany) example to server broadcast, we need something to trigger the [`SelectMany`](xref:Bonsai.Reactive.SelectMany) sequence creation, and something to trigger termination. We already have a trigger for sequence creation in the output of the [`Router`](xref:Bonsai.ZeroMQ.Router) since we want to run our [`SelectMany`](xref:Bonsai.Reactive.SelectMany) sequence every time a client message is received. For our sequence temination trigger, we want something that is guaranteed to fire after the server receives a client message, but before the next message is received so that our [`SelectMany`](xref:Bonsai.Reactive.SelectMany) sequence for each message responds only to that particular message. A simple solution is therefore to use the arrival of the next message as our sequence termination trigger.

- Add a [`Skip`](xref:Bonsai.Reactive.Skip) operator after the [`Router`](xref:Bonsai.ZeroMQ.Router) in a separate branch and connect this to a [`PublishSubject`](xref:Bonsai.Reactive.PublishSubject).
- Set the [`Skip`](xref:Bonsai.Reactive.Skip) operator's `Count` property is set to 1, and name the [`PublishSubject`](xref:Bonsai.Reactive.PublishSubject) 'NextMessage'.
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
- Set the [`Skip`](xref:Bonsai.Reactive.Skip) operator's `Count` property is set to 1, and name the [`PublishSubject`](xref:Bonsai.Reactive.PublishSubject) 'NextMessage'.
- Set the [`Skip`](xref:Bonsai.Reactive.Skip) operator's `Count` property is set to 1, and name the [`PublishSubject`](xref:Bonsai.Reactive.PublishSubject) `NextMessage`.

Using verbatim literals for string property values.

Copy link
Member

Choose a reason for hiding this comment

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

Is this a BOM-diff? Sometimes it can happen if you were committing partial changes to the file, but looks like there are no changes, so maybe we can just remove this?

@glopesdev glopesdev added the documentation Improvements or additions to documentation label Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants