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

NerlMonitor #236

Open
wants to merge 41 commits into
base: master
Choose a base branch
from
Open

NerlMonitor #236

wants to merge 41 commits into from

Conversation

GuyPerets106
Copy link
Collaborator

For your review @leondavi @halfway258 @galhilu

inputJsonFiles/Architecture/arch_1PCSIM6WorkerMNist.json Outdated Show resolved Hide resolved
@@ -1,7 +1,7 @@
{
"Features": 784,
"Labels": ["0", "1", "2", "3", "4", "5", "6", "7", "8", "9"],
"CSV path": "mnist",
Copy link
Owner

Choose a reason for hiding this comment

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

@halfway258 Please approve this change

disp.plot(ax=axes[i, j], colorbar=False)
disp.ax_.set_title(f'{worker}, class #{j}\nAccuracy={round(accuracy_score(workerNeuronRes[worker][globe.TRUE_LABLE_IND][j], workerNeuronRes[worker][globe.PRED_LABLE_IND][j]), 3)}')
if i < len(workersList) - 1:
if i < len(workersList) - 1: # ? Ask David why this is needed
Copy link
Owner

Choose a reason for hiding this comment

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

@halfway258 Haran can you please clarify this line
@GuyPerets106 Please add a comment with description gave by Haran


Also, install **Pyrlang** and **Term** libraries for Python-Erlang communication (follow their instructions **carefully**):

Pyrlang - https://github.com/Pyrlang/Pyrlang
Copy link
Owner

@leondavi leondavi Aug 21, 2023

Choose a reason for hiding this comment

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

@GuyPerets106 @galhilu

We should add an installation step in bash script of NerlMonitor.
The bash script should accept --set-env flag.
If called with this flag then it installs an environment with required python modules (add requirement.txt with all required modules except of pyrlang that is cloned).
This python virtualenv should be installed to /tmp/nerlmonitor/venv
Each time --set-env flag is called it removes venv directory and reinstall the virtualenv.

Of course there is an assumption that virtualenv is already installed on machine.

src_erl/NerlMonitor/LICENSE.md Outdated Show resolved Hide resolved

# Dependencies
`pip install` these libraries (In addition to the src_py/requirements.txt file):
- NetowrkX
Copy link
Owner

Choose a reason for hiding this comment

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

@GuyPerets106
Create a requirements.txt file
You can use pipreqs
https://stackoverflow.com/questions/31684375/automatically-create-requirements-txt

Change instructions to:

  1. Create a python virtualenv for NerlMonitor.
  2. Install modules: pip3 install -r /usr/local/lib/nerlnet-lib/NErlNet/src_py/NerlMonitor/requirements.txt

@leondavi
Copy link
Owner

@GuyPerets106 @galhilu

Please resolve conflicts and validate with @halfway258

Edges = lists:droplast(lists:last(Devices)),

io:format("got graph: ~p~n", [Devices]),
DeviceList = lists:droplast(Devices),

{FileName, G} = gui_tools:makeGraphIMG(DeviceList, Edges),
{FileName, G} = gui_tools:makeGraphIMG(22),
Copy link
Owner

Choose a reason for hiding this comment

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

@GuyPerets106 @galhilu
What is this magic number?!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it's a mistake, I don't think we meant to change anything in NerlGUI
@galhilu correct me if I'm wrong

src_erl/NerlGUI/src/graphScreen.erl Outdated Show resolved Hide resolved
@@ -76,4 +76,4 @@ Minimum Python version: 3.8
4. Run Jupyter notebook with ```jupyter-notebook``` and create a new notebook in the created dir from step 3.
5. Follow the example: https://github.com/leondavi/NErlNet/blob/master/examples/example_run.ipynb

Contact Email: [email protected]
Contact Email: [email protected]
Copy link
Owner

Choose a reason for hiding this comment

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

From some reason you change the README.md of the root.
I guess it was a mistake.
We should perform rebase on master together

Workers = ets:lookup_element(EtsRef, workersNames, ?ETS_KV_VAL_IDX),
cast_message_to_workers(EtsRef, {NewState}),
{next_state, waitforWorkers, State#client_statem_state{nextState = NewState, waitforWorkers = Workers}};

waitforWorkers(cast, In ={worker_kill , Body}, State = #client_statem_state{waitforWorkers = WaitforWorkers,etsRef = EtsRef}) ->
%got kill command for a worker, kiil and take off waitforWorkers list
Copy link
Owner

Choose a reason for hiding this comment

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

typo - kill

waitforWorkers(cast, In ={worker_kill , Body}, State = #client_statem_state{waitforWorkers = WaitforWorkers,etsRef = EtsRef}) ->
%got kill command for a worker, kiil and take off waitforWorkers list
ets:update_counter(EtsRef, msgCounter, 1),
ets:update_counter(EtsRef, infoIn, nerl_tools:calculate_size(In)),
Copy link
Owner

Choose a reason for hiding this comment

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

infoIn?
What is it? add a documentation please.
If it is a size of in message, then more informative name is required.

Workers = ets:lookup_element(EtsRef, workersNames, ?ETS_KV_VAL_IDX),
cast_message_to_workers(EtsRef, {NewState}),
{next_state, waitforWorkers, State#client_statem_state{nextState = NewState, waitforWorkers = Workers}};

waitforWorkers(cast, In ={worker_kill , Body}, State = #client_statem_state{waitforWorkers = WaitforWorkers,etsRef = EtsRef}) ->
%got kill command for a worker, kiil and take off waitforWorkers list
Copy link
Owner

Choose a reason for hiding this comment

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

got a kill command from who?
Please more information in documentation, think about a new student that should handle this code.

ets:update_counter(EtsRef, infoIn, nerl_tools:calculate_size(EventContent)),
case EventContent of
{'DOWN',_Ref,process,Pid,_Reason}-> %worker down
[[WorkerName]] = ets:match(EtsRef,{'$1',Pid,'_','_','_'}),
Copy link
Owner

Choose a reason for hiding this comment

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

What is going on in this line?

-export([init/2]).

init(Req0 , [MainServerPid]) ->
%handler for a tool connection req
Copy link
Owner

Choose a reason for hiding this comment

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

What?
Please extend your explanation with more details.

Graph = gen_server:call(MainServerPid , getGraph),
WorkersMap = ets:lookup_element(nerlnet_data , workers , ?DATA_IDX),
WorkersClients = maps:to_list(WorkersMap),
Workers = lists:flatten([atom_to_list(X)++"-"++atom_to_list(Y)++"!" || {X , Y} <- WorkersClients]),
Copy link
Owner

Choose a reason for hiding this comment

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

What is the role of "-" and "!" ?
"-" is very very risky it can be a part of worker or client name.
Please explain what this line does and change marks to some safer patern e.g. @#@

{ok, Req, MainServerPid};

init(Req0 , [Action , MainServerPid]) ->
%handler for a tool's requested action (not connection)
Copy link
Owner

Choose a reason for hiding this comment

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

More info about this method

Copy link
Owner

Choose a reason for hiding this comment

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

NerlGUI directory should be deprecated.
Please move desired content to NerlMonitor and remove NerlGUI directory.

Copy link
Owner

Choose a reason for hiding this comment

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

Nice!

@leondavi leondavi requested a review from galhilu September 2, 2023 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants