Bennaaym/yaml support by bennaaym · Pull Request #58 · facebookresearch/labgraph

bennaaym · 2022-02-08T22:31:56Z

Description

This extension provides an API to generate a serialized version of the labgraph topology. The serialized graph topology can be used in different applications E.g: server-client communication or to get a simplified overview of the topology in case of complicated graphs.

Fixes #53

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

validation/testing

Tested the new API using python builtin unit testing framework

python -m extensions.yaml_support.labgraph_monitor.tests.test_lg_monitor_api

python -m extensions.yaml_support.labgraph_yaml_parser.tests.test_lg_yaml_api

Checklist:

Have you added tests that prove your fix is effective or that this feature works?
New and existing unit tests pass locally with these changes?
Has code been commented, particularly in hard-to-understand areas?
Have you made corresponding changes to the documentation?

bennaaym · 2022-02-08T22:36:24Z

Hello @jfResearchEng
I'm creating this draft PR to get feedback regarding the way I'm using Labgraph Websockets API (I'm creating a sender graph to broadcast the serialized representation of the graph. I wonder if there is a simpler way to do that). Also, I'd like to get feedback on the current serialized graph schema

…us version didn't map upstream_messages to upstreams

…ed version of the graph

jfResearchEng · 2022-02-15T17:49:05Z

extensions/yaml_support/labgraph_monitor/tests/test_lg_monitor_api.py

@@ -0,0 +1,101 @@
+#!/usr/bin/env python3


A nice-to-have feature is to also add in a latency test for the end-to-end graph. Latency can be defined as the end_time_at_the_sink_node_of_the_graph minus start_time_at_the_source_node_of_the_graph. 20ms would be a good latency for this application.
This can be in this PR or a separate PR.

Will the latency test be for WSSenderGraph or for the main graph we are sending through WS?

It will be the main graph, in your test case, i.e. the Demo graph.

Hi @jfResearchEng, I was wondering how can we calculate the start/end time without adding extra information(Timestamp) to the Node(source/sink) config. The current code is extracting information from the graph instance, but it's not really visiting each node in the standard way (DFS/BFS)

For example, demo_graph you have created, you can get the start time from node NOISE_GENERATOR, and end time from node ATTENUATOR.
Reference: https://github.com/facebookresearch/labgraph/blob/main/extensions/graphviz_support/graphviz_support/tests/demo_graph/demo.py

jfResearchEng · 2022-02-15T17:53:36Z

extensions/yaml_support/labgraph_monitor/server/lg_monitor_server.py

+DEFAULT_IP = WS_SERVER.DEFAULT_IP
+DEFAULT_PORT = WS_SERVER.DEFAULT_PORT
+DEFAULT_API_VERSION = WS_SERVER.DEFAULT_API_VERSION
+SAMPLE_RATE = 5


Shall this constant be in a config?

should I put the default values directly inside the SerializerConfig & WSAPIServerConfig instead of passing them as params?

The default sample_rate in WSAPIServerConfig is 100. For latency test, I would suggest to use 200 or 500 (ideally test at 2000 but potentially the latency could increase more significantly). For a regular lg monitor server, 5 is fine.
https://github.com/facebookresearch/labgraph/blob/0484817381207eb86972248a07242b2a7a668b41/labgraph/websockets/ws_server/ws_api_node_server.py

dtemir · 2022-03-07T14:43:19Z

Hi @jfResearchEng! I just wanted to let you know that this PR is ready for merging as everything works correctly in terms of #53.

jfResearchEng · 2022-03-07T23:52:01Z

Some of the suggestions in this PR could be addressed in a separate PR. If you would like to include them in this PR, please let me know. Otherwise, I can merge this diff after you have update the PR #60 .

bennaaym · 2022-03-08T09:27:15Z

@jfResearchEng I think addressing the new suggestion in separate PR will be better.

dtemir · 2022-03-11T19:39:03Z

extensions/yaml_support/labgraph_monitor/server/lg_monitor_server.py

+            self.SERIALIZER.configure(
+                SerializerConfig(
+                    data=data,
+                    sample_rate=SAMPLE_RATE,
+                    stream_name=STREAM.LABGRAPH_MONITOR,
+                    stream_id=STREAM.LABGRAPH_MONITOR_ID
+                )
+            )


Just to be sure @bennaaym, does this SERIALIZER publish:

The current state of the graph (such as it is running the Demo graph lg.run(Demo) as well as the WSSenderNode graph lg.run(WSSenderNode))

Or does it just publish the data that it received from serialize_graph() without running lg.run(Demo)

I think there might be a small issue with the fact that it is not running the Demo graph while running the WSSenderNode, which makes it impossible to fetch real-time data right now

do you know if there is a good way of running the Demo graph as well as the WSSenderNode?

Hey, @dtemir I was assuming that the graph is running in a different process. using, lg.run will block the main thread till the execution end.

a way around that might be to use a threading library to run both graphs:

import labgraph as lg from labgraph.examples.simple_viz import Demo from extensions.yaml_support.labgraph_monitor.generate_lg_monitor.generate_lg_monitor import generate_labgraph_monitor from threading import Thread def run_graph_in_new_thread(graph: lg.Graph): runner = lg.ParallelRunner(graph=graph) runner.run() if __name__ == '__main__': graph = Demo() thread = Thread(target=run_graph_in_new_thread, args=(graph, )) thread.start() generate_labgraph_monitor(graph) thread.join()

thanks a lot @bennaaym! This is very-very useful. Now I gotta find a way to move the messages between two graphs

dtemir · 2022-03-11T19:43:21Z

extensions/yaml_support/labgraph_monitor/server/serializer_node.py

+    TOPIC = lg.Topic(WSStreamMessage)
+    config: SerializerConfig
+
+    @lg.publisher(TOPIC)


I was thinking, as a simple solution for now:

Maybe we could have this Serializer node to subscribe to the publishers in Demo (NOISE_GENERATOR.OUTPUT, ROLLING_AVERAGE.OUTPUT etc.) and then match them with their source nodes, as we discussed?

That might work for the current demo graph, however changing the graph (changing nodes names, topic names ...) will require changing the Serializer node

I agree. It is definitely not a desirable solution

dtemir · 2022-03-11T20:01:00Z

extensions/yaml_support/labgraph_monitor/generate_lg_monitor/generate_lg_monitor.py

+
+    # Send the serialized graph to Front-End
+    # using LabGraph Websockets API
+    run_server(serialized_graph)


Just to be sure @bennaaym, does this SERIALIZER publish:

The current state of the graph (such as it is running the Demo graph lg.run(Demo) as well as the WSSenderNode graph lg.run(WSSenderNode))

Or does it just publish the data that it received from serialize_graph() without running lg.run(Demo)

I think there might be a small issue with the fact that it is not running the Demo graph while running the WSSenderNode, which makes it impossible to fetch real-time data right now

My question above is also related to this particular line, such as the run_server() function here is called with the already prepared version of the seralized_graph message, which does not update if the graph itself updates (i.e. the message data changes)

Click to view

{ 'name': 'Demo', 'nodes': { 'NoiseGenerator': { 'upstreams': { } }, 'RollingAverager': { 'upstreams': { 'NoiseGenerator': [ { 'name': 'RandomMessage', 'fields': { 'timestamp': { 'type': 'float' }, 'data': { 'type': 'ndarray' } } } ] } }, 'Amplifier': { 'upstreams': { 'NoiseGenerator': [ { 'name': 'RandomMessage', 'fields': { 'timestamp': { 'type': 'float' }, 'data': { 'type': 'ndarray' } } } ] } }, 'Attenuator': { 'upstreams': { 'NoiseGenerator': [ { 'name': 'RandomMessage', 'fields': { 'timestamp': { 'type': 'float' }, 'data': { 'type': 'ndarray' } } } ] } }, 'Sink': { 'upstreams': { 'RollingAverager': [ { 'name': 'RandomMessage', 'fields': { 'timestamp': { 'type': 'float' }, 'data': { 'type': 'ndarray' } } } ], 'Amplifier': [ { 'name': 'RandomMessage', 'fields': { 'timestamp': { 'type': 'float' }, 'data': { 'type': 'ndarray' } } } ], 'Attenuator': [ { 'name': 'RandomMessage', 'fields': { 'timestamp': { 'type': 'float' }, 'data': { 'type': 'ndarray' } } } ] } } } }

yeah, that's true, actually, the whole serialization process ( identify_graph_nodes, connect_to_upstream, serialize_graph) should be moved from generate_labgraph_monitor to the SerializerNode. currently I'm only sending the result which was ok for only sending the topology without real-time values

could you please let me know what you think of this approach?

#64 (comment)

@dtemir great work! I went through your current solution and it looks pretty promising however I can see a few challenges related to the current approach:

The current approach is changing the topology of the main graph ( the graph to be serialized and streamed). for example, Serializer and WSAPIServerNode are serialized and sent to the frontend however they are not a part of the original topology of the demo graph.

The current approach is static streaming the topology of a new graph will require making different changes to the current code. Also automating the subscription process might be challenging.

dtemir · 2022-03-11T20:03:42Z

extensions/yaml_support/labgraph_monitor/server/lg_monitor_server.py

+            self.SERIALIZER.configure(
+                SerializerConfig(
+                    data=data,
+                    sample_rate=SAMPLE_RATE,
+                    stream_name=STREAM.LABGRAPH_MONITOR,
+                    stream_id=STREAM.LABGRAPH_MONITOR_ID
+                )
+            )


do you know if there is a good way of running the Demo graph as well as the WSSenderNode?

dtemir · 2022-03-29T19:28:01Z

@bennaaym do you think we could try merging this PR? Or would you like to add some more stuff to it?

dtemir · 2022-03-29T19:30:08Z

Some of the suggestions in this PR could be addressed in a separate PR. If you would like to include them in this PR, please let me know. Otherwise, I can merge this diff after you have update the PR #60 .

@jfResearchEng I think this PR #58 should be merged first as it doesn't interfere with the PR #60 and is basis for that PR

bennaaym · 2022-03-31T08:26:13Z

@dtemir, I believe the current PR implement the main functionalities to fix #58, other feature and enhancement can be added as separated PRs.

dtemir · 2022-03-31T20:32:16Z

@dtemir, I believe the current PR implement the main functionalities to fix #58, other feature and enhancement can be added as separated PRs.

I agree! Can you un-draft this PR for merging?

bennaaym added 5 commits February 7, 2022 23:51

bennaaym/yaml_support: LabgraphMonitorNode class implemented

ad636c7

bennaaym/yaml_support: generate_labgraph_monitor initial implementation

6ce32a0

bennaaym/yaml_support: yaml_support directory refactored

ffbe3fe

bennaaym/yaml_support: unittests added for labgraph_monitor

d3e376f

bennaaym/yaml_support: labgraph monitor initial implementation

9f8d47c

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 8, 2022

bennaaym added 2 commits February 13, 2022 17:25

bennaaym/yaml_support: serialized graph representation update, previo…

fe95a1d

…us version didn't map upstream_messages to upstreams

bennaaym/yaml_support: improve message representation in the serializ…

2c13bff

…ed version of the graph

bennaaym force-pushed the bennaaym/yaml_support branch from 70c030a to 2c13bff Compare February 15, 2022 13:43

jfResearchEng reviewed Feb 15, 2022

View reviewed changes

bennaaym mentioned this pull request Feb 18, 2022

Bennaaym/labgraph monitor #60

Merged

7 tasks

dtemir approved these changes Mar 7, 2022

View reviewed changes

dtemir reviewed Mar 11, 2022

View reviewed changes

This was referenced Mar 12, 2022

[WIP] prototyping real-time messaging bennaaym/labgraph#1

Closed

Real Time Messages #64

Merged

bennaaym marked this pull request as ready for review March 31, 2022 20:42

jfResearchEng approved these changes Apr 3, 2022

View reviewed changes

jfResearchEng merged commit 3658f8d into facebookresearch:main Apr 3, 2022

Conversation

bennaaym commented Feb 8, 2022

Description

Type of change

validation/testing

Checklist:

Uh oh!

bennaaym commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfResearchEng Feb 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtemir commented Mar 7, 2022

Uh oh!

jfResearchEng commented Mar 7, 2022

Uh oh!

bennaaym commented Mar 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bennaaym Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtemir Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bennaaym Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtemir commented Mar 29, 2022

Uh oh!

dtemir commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bennaaym commented Mar 31, 2022

Uh oh!

dtemir commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

bennaaym commented Feb 8, 2022 •

edited

Loading

jfResearchEng Feb 20, 2022 •

edited

Loading

bennaaym Mar 12, 2022 •

edited

Loading

dtemir Mar 11, 2022 •

edited

Loading

bennaaym Mar 31, 2022 •

edited

Loading

dtemir commented Mar 29, 2022 •

edited

Loading