Bennaaym/labgraph monitor #60

bennaaym · 2022-02-18T20:17:59Z

Description

This PR is an improvement of the existing Labgraph Monitor application

The PR adds the following :

Migrate the old application from Javascript to Typescript( static/strong typing support)
Use a much simpler serialized representation of the computational graph (old representation was complex, hard to process, and contained redundant data)
Add input/output messages (name, fields & datatypes)
Add REALTIME / MOCK features

(!) this PR is complementary to the following PR Bennaaym/yaml support #58

Fixes #45

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
This change requires a documentation update

Feature/Issue validation/testing

Tested the different components of the application using react-testing-library & jest

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?

…nitor

…tion

…nd server

…vious graph structure is different than the new one

…with realdata

…e extensions/prototypes/labgraph_monitor

…rectory

dtemir · 2022-03-01T20:44:29Z

hey @bennaaym. again, awesome work! i just wanted to let you know that there are two sub-directories under labgraph_monitor/src/pages: Home and home. Having both of them threw me an error, so I deleted the home one and everything worked.

bennaaym · 2022-03-02T07:45:07Z

@dtemir thank you for reporting that, "/home" wasn't supposed to be committed. I removed it.

fzchriha · 2022-03-05T16:09:01Z

Hi @bennaaym, I reviewed the changes I believe it is enough to fix #45 , could you please tell me what are the features your current solution doesn't have? Great job!

jfResearchEng · 2022-03-07T23:08:33Z

extensions/prototypes/labgraph_monitor/.husky/pre-commit

@@ -0,0 +1,8 @@
+#!/bin/sh


Is .husky file a compulsory file for this commit?

Currently I'm using prettier, eslint, husky to automate formatting, identifying patterns which keeps the code consistent and production-ready.
I believe that aligns with the approach that LabGraph is using with python by using flake8. The point of using husky is to automate the process through git-hooks, my current code will run a check whenever a contributor tries to add a new commit(pre-commit git-hook) that will prevent bad commits from being committed if there are any (e.g: unwanted console.log(), unused packages, unformatted code ...).
The husky script will run only if there is a change in the extensions/prototypes/labgraph_monitor directory.

jfResearchEng · 2022-03-07T23:10:28Z

extensions/prototypes/labgraph_monitor/public/robots.txt

@@ -0,0 +1,3 @@
+# https://www.robotstxt.org/robotstxt.html


What is this robots.txt used for?

robots.txt file tells search engine crawlers which URLs the crawler can access on the site (Robots Exclusion Protocol). This is used mainly to avoid overloading the site with requests. It is mainly useful in case Labgraph Monitor will be deployed. It can be deleted in case of localhost usage.

jfResearchEng · 2022-03-07T23:32:19Z

extensions/prototypes/labgraph_monitor/yarn.lock

@@ -0,0 +1,9601 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


Do we need to have this file?

yarn unlike npm provides a deterministic approach to set up node_modules using Lockfiles that ensures that every install results in the exact same file structure in node_modules across all machines. Deleting yarn.lock may create errors when trying to set up the dependencies. ref: https://engineering.fb.com/2016/10/11/web/yarn-a-new-package-manager-for-javascript/

jfResearchEng

Thank you for contributing this PR! This looks good and please have a quick review of my comments.

bennaaym · 2022-03-08T09:21:02Z

Hi, @fzchriha thanks for helping review this PR. The current application makes use of all the data provided by the WebSocket API, adding new features to the frontend should always be associated with updates in the backend.

These are some features that I think are useful and align with LabGraph Monitor vision:

Currently when an edge is clicked, "message_name", "message_fields" and "fields_datatypes" are displayed, it will be useful to add the "fields_values" (The changes can be made on the Mocks then transferred to be real-time when the backend is updated)
After adding "fiedls_values", keeping track of the values may be hard since the values will be changing every 200ms(depends on the sampling rate, currently it's 5 samples/s), So I think adding visualisation to the data might be useful (something like Chart.js can be used for that ).
Storing snapshots of the data might be useful for future analysis. (something like Redis can be used for that)
Another feature is to change the flow of messages in realtime, setting up the UI for that is relatively simple(updatable-edge), but it will require a lot of work in terms of backend and the serialized graph representation

jfResearchEng · 2022-03-08T14:25:05Z

Thanks, bennaaym, for replying to the comments and recommendations on additional features! I've asked an additional reviewer to help review this PR by end of this week.

navn-r

Overall, this PR is ready for @dtemir and @fzchriha to use as a base. However please still consider my comments.

Awesome Job! 🌮 💯

navn-r · 2022-03-11T21:48:05Z

extensions/prototypes/labgraph_monitor/.prettierrc.json

@@ -0,0 +1,7 @@
+{
+    "trailingComma": "es5",
+    "tabWidth": 4,


Quick Question, is this tab width also consistent with the formatting on the Python side?

currently, flake8 show a warning if the tab size isn't a multiple of 4 however the code can be always committed (that might need to be automated more). For typescript Airbnb guidelines suggest using 2 as tab size however I changed it to 4 to keep it consistent with Python.

ref: 949ffc9

Personally, I'm used to the Airbnb spec with 2, and probably most Front End contributors would be as well. However, if the repo wants to be consistent for both React and Python then yea I guess thats okay.

navn-r · 2022-03-11T21:50:31Z

extensions/prototypes/labgraph_monitor/README.md


-Once Node is installed, you can verify that node/npm was installed successfully by running the following: 
+-   [Node.js](https://nodejs.org/en/)
+-   [Yarn](https://classic.yarnpkg.com/lang/en/docs/install) (**RECOMMENDED**)


I wouldn't put this as recommended, rather it should be compulsery. What we don't want is a contributor thinking its okay to just use npm and the repo would end up with two lockfiles.

Done. however, I don't think there is a way to prevent the user from using npm. I added package-lock.json to .gitignore to prevent it from being committed

Yea I think that works. My suggestion was really to tell a contributor

"hey don't use npm, just stick with yarn"

navn-r · 2022-03-11T21:50:51Z

extensions/prototypes/labgraph_monitor/README.md

+**Set up the application**

-The above should display the version of node/npm that were installed. For example:
+(!) The following tutorial utilizes **yarn** (recommended) however **npm** can be utilized as well.


See above statement about yarn and npm.

navn-r · 2022-03-11T21:53:18Z

extensions/prototypes/labgraph_monitor/README.md

+labgraph> cd extensions/prototypes/labgraph_monitor
+labgraph\extensions\prototypes\labgraph_monitor>


nitpick, since everyone has different prompts for their terminals, It would be better to have just

Suggested change

labgraph> cd extensions/prototypes/labgraph_monitor

labgraph\extensions\prototypes\labgraph_monitor>

cd extensions/prototypes/labgraph_monitor

as the step, so contributors could just copy paste the code snippet as they go through.

navn-r · 2022-03-11T21:55:53Z

extensions/prototypes/labgraph_monitor/README.md

-Note: npx is a package that comes with npm 5.2+ 
 ```
-npx create-react-app labgraph_monitor_extension
+labgraph\extensions\prototypes\labgraph_monitor> yarn


[Nitpick] Same applies here, at this step, its assumed that the user is already at the given directory, so we could just have

Suggested change

labgraph\extensions\prototypes\labgraph_monitor> yarn

yarn

Again this is just a nitpick, but if proceeding with my suggestion, make sure to do it to the other code snippets as well.

🤠

navn-r · 2022-03-11T22:38:42Z

extensions/prototypes/labgraph_monitor/src/components/SettingPanel/GraphSettings.tsx

+        dispatch(setConnection(WS_STATE.IS_CONNECTING));
+    };
+
+    const handleDiconnect = (event: React.FormEvent<HTMLFormElement>) => {


Suggested change

const handleDiconnect = (event: React.FormEvent<HTMLFormElement>) => {

const handleDisconnect = (event: React.FormEvent<HTMLFormElement>) => {

be sure to change the other places in which this function is called :)

navn-r · 2022-03-11T22:40:34Z

extensions/prototypes/labgraph_monitor/src/components/SettingPanel/GraphSettings.tsx

+    const [mock, setMock] = useState<string>('');
+    const dispatch = useDispatch();
+
+    const handleChange = (_: React.SyntheticEvent, newValue: string) => {


This seems a bit redundant, we could just have (_, newValue) => setValue(newValue) inside the onChange, rather than having a brand new function. Either remove it, or rename it to something less generic

navn-r · 2022-03-11T22:43:23Z

extensions/prototypes/labgraph_monitor/src/components/SettingPanel/NodeSettings.tsx

+                {selectedNode.id && <Typography>{selectedNode.id}</Typography>}
+                {!selectedNode.id && (
+                    <Typography style={{ fontSize: '.8rem', fontWeight: 400 }}>
+                        Click on a node to see its information
+                    </Typography>
+                )}


Suggested change

{selectedNode.id && <Typography>{selectedNode.id}</Typography>}

{!selectedNode.id && (

<Typography style={{ fontSize: '.8rem', fontWeight: 400 }}>

Click on a node to see its information

</Typography>

)}

{selectedNode?.id ? (<Typography>{selectedNode.id}</Typography>) : (

<Typography style={{ fontSize: '.8rem', fontWeight: 400 }}>

Click on a node to see its information

</Typography>

)}

Better to switch to ternary in this case

navn-r · 2022-03-11T22:48:30Z

extensions/prototypes/labgraph_monitor/src/contexts/WSContext/WSContext.tsx

+const WSContextProvider: React.FC<ReactNode> = ({ children }): JSX.Element => {
+    const { connection, graph } = useSelector((state: RootState) => state.ws);
+
+    const clientRef = useRef<any>(null);


nitpick, not helpful to have any, could we replace that with its actual type?

navn-r · 2022-03-11T22:53:21Z

extensions/prototypes/labgraph_monitor/src/redux/reducers/ws/interfaces/IGraph.ts

+ */
+import INode from './INode';
+
+interface IGraph {


It seems like this interface is present in multiple files. Any way we could just have one central source for the IGraph, INode interfaces, rahter than multiple IGraph.ts files?

…pos fixed

…r messages

bennaaym and others added 25 commits February 10, 2022 15:59

bennaaym/labgraph_monitor: set up typescript template for labgraph mo…

2b94805

…nitor

bennaaym/labgraph_monitor: graph layout set up using mock data

5b2cc11

bennaaym/labgraph_monitor: organize the graph layout using dagre

80c42a9

bennaaym/labgraph_monitor: graph style updated

a411e88

bennaaym/labgraph_monitor: SettingPanel initial implementation

712a69b

bennaaym/labgraph_monitor: SettingPanel updated

7289e37

bennaaym/labgraph_monitor: UIContext implemented

ac5d453

bennaaym/labgraph_monitor: GraphSettings component initial implementa…

f1a115f

…tion

bennaaym/labgraph_monitor: mock examples added

32c99de

bennaaym/labgraph_monitor: GraphSettings connect form added

2220577

bennaaym/labgraph_monitor: websocket connection set up with the backe…

377aed5

…nd server

bennaaym/labgraph_monitor: optimize rendering, render only if the pre…

41de247

…vious graph structure is different than the new one

bennaaym/labgraph_monitor: NodeSetting, EdgeSetting comonent updated …

26c8022

…with realdata

bennaaym/labgraph_monitor: EdgeSetting component updated

a689772

bennaaym/labgraph_monitor: migrate ConfigContext to redux store

033ee90

bennaaym/labgraph_monitor: migrating WSContext to redux store

56327cb

bennaaym/labgraph_monitor: copyright header added

203f52e

bennaaym/labgraph: updated .husky script to detect changes only insid…

cc5529e

…e extensions/prototypes/labgraph_monitor

bennaaym/labgraph_monitor: first unittest added, test Home component

09598cf

bennaaym/labgraph_monitor: unittest added

b421b4a

bennaaym/labgraph_monitor: adding documentation comments

b72e544

bennaaym/labgraph_monitor: README updated

8e68cb5

bennaaym/labgraph: update from graph/ to Graph/ issue resolved

1983088

bennaaym/labgraph_monitor: github action added

0666023

Delete extensions/prototypes/labgraph_monitor/src/components/graph di…

538147a

…rectory

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 18, 2022

Delete extensions/prototypes/labgraph_monitor/src/pages/home directory

3bb54ee

dtemir approved these changes Mar 7, 2022

View reviewed changes

jfResearchEng reviewed Mar 7, 2022

View reviewed changes

jfResearchEng suggested changes Mar 7, 2022

View reviewed changes

jfResearchEng mentioned this pull request Mar 7, 2022

Bennaaym/yaml support #58

Merged

6 tasks

bennaaym requested a review from jfResearchEng March 8, 2022 09:35

navn-r approved these changes Mar 11, 2022

View reviewed changes

bennaaym added 5 commits March 12, 2022 13:40

bennaaym/labgraph_monitor: directories refactored, README updated, ty…

b3c97de

…pos fixed

bennaaym/labgraph_monitor: WSContext updated

4739bd6

bennaaym/labgraph_monitor: github action updated

64c8342

bennaaym/labgraph_monitor: public/ updated

5938270

bennaaym/labgraph_monitor: get rid of inline-style | add interface fo…

3d2d640

…r messages

jfResearchEng approved these changes Apr 3, 2022

View reviewed changes

jfResearchEng merged commit adf27ac into facebookresearch:main Apr 3, 2022

		@@ -0,0 +1,9601 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

		labgraph> cd extensions/prototypes/labgraph_monitor
		labgraph\extensions\prototypes\labgraph_monitor>

	labgraph> cd extensions/prototypes/labgraph_monitor
	labgraph\extensions\prototypes\labgraph_monitor>
	cd extensions/prototypes/labgraph_monitor

	const handleDiconnect = (event: React.FormEvent<HTMLFormElement>) => {
	const handleDisconnect = (event: React.FormEvent<HTMLFormElement>) => {

Bennaaym/labgraph monitor #60

Bennaaym/labgraph monitor #60

Uh oh!

Conversation

bennaaym commented Feb 18, 2022

Description

Type of change

Feature/Issue validation/testing

Checklist:

Uh oh!

dtemir commented Mar 1, 2022

Uh oh!

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

Uh oh!

Uh oh!

fzchriha commented Mar 5, 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!

bennaaym Mar 8, 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!

bennaaym Mar 8, 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!

jfResearchEng left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jfResearchEng commented Mar 8, 2022

Uh oh!

navn-r left a comment

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!

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!

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!

navn-r 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

bennaaym commented Mar 2, 2022 •

edited

Loading

fzchriha commented Mar 5, 2022 •

edited

Loading

bennaaym Mar 8, 2022 •

edited

Loading

bennaaym Mar 8, 2022 •

edited

Loading

bennaaym commented Mar 8, 2022 •

edited

Loading

bennaaym Mar 12, 2022 •

edited

Loading

bennaaym Mar 12, 2022 •

edited

Loading

navn-r Mar 11, 2022 •

edited

Loading