Skip to content

feat: stop ModularServer closes #1642 #1675

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

feat: stop ModularServer closes #1642 #1675

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f30847c
feat: stop ModularServer closes #1642
reka Apr 26, 2023
fbe7e62
missing message type
reka Apr 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions mesa/visualization/ModularVisualization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,19 +223,24 @@ def on_message(self, message):
if self.application.verbose:
print(message)
msg = tornado.escape.json_decode(message)
try:
msg_type = msg["type"]
except KeyError:
print("Unexpected message. Key 'type' missing.")
Copy link
Contributor

@rht rht Apr 28, 2023

Choose a reason for hiding this comment

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

I haven't encountered this branch point yet.
I think the safeguard is not necessary. Printing this error message instead of raising the exception results in an information loss about the location in which the error happens.
Replacing with msg_type = msg["type"] should do.

Copy link
Author

@reka reka Apr 28, 2023

Choose a reason for hiding this comment

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

I haven't encountered this branch point yet.

I ran into this, as I was trying out various changes in mesa/visualization/templates/js/runcontrol.js.
When I sent a message that didn't contain a key "type"

=>

  • The SocketHandler threw an error
  • The Tornado server shut down.

If we add an error handling, the server will continue running despite of the unexpected message.
I'm not sure whether this is a good or bad thing.

Printing this error message instead of raising the exception results in an information loss about the location in which the error happens.

That's a good point.
Is there more info that should be included in the error message?
Or do you think that raising the exception and shutting down the server is the way to go?

Copy link
Contributor

@rht rht May 9, 2023

Choose a reason for hiding this comment

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

When I sent a message that didn't contain a key "type"

This is a change that is made by the developer instead of the user, as such, I believe the server should halt violently with tracebacks instead of continue running.

return

if msg["type"] == "get_step":
if msg_type == "get_step":
if not self.application.model.running:
self.write_message({"type": "end"})
else:
self.application.model.step()
self.write_message(self.viz_state_message)

elif msg["type"] == "reset":
elif msg_type == "reset":
self.application.reset_model()
self.write_message(self.viz_state_message)

elif msg["type"] == "submit_params":
elif msg_type == "submit_params":
param = msg["param"]
value = msg["value"]

Expand All @@ -246,6 +251,10 @@ def on_message(self, message):
else:
self.application.model_kwargs[param] = value

elif msg_type == "shut_down":
Copy link
Contributor

@rht rht Apr 28, 2023

Choose a reason for hiding this comment

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

Why is it necessary to be in debug mode in order for the shutdown to be functional?

Copy link
Author

@reka reka Apr 28, 2023

Choose a reason for hiding this comment

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

My way of thoughts was:

  • It's generally not a good idea to allow a client to shut down a web server.
  • In a local environment, in debug mode, this might be OK.

By the way, I have a general question:
Do I understand correctly that this Tornado server is expected to run only in a local environment, on the user's computer?
Or is it also deployed somewhere as an actual web server? 🤔

Copy link
Contributor

@rht rht May 9, 2023

Choose a reason for hiding this comment

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

Or is it also deployed somewhere as an actual web server? 🤔

Some people might have done this, that I am not aware of. The relevant issue is #481. We are also currently exploring the possibility of having the UI stack fully in Python (#1622), in a way that can be run on a Jupyter notebook. Hosting Jupyter notebooks is so ubiquitous that there are canned ways to do it securely.

Copy link
Contributor

@rht rht May 9, 2023

Choose a reason for hiding this comment

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

I am in the opinion that if the pure Python implementation goes well, it might replace the current clunky Tornado server, in a way that is easy to extend, and based on existing established data interaction frameworks. cc: @ankitk50.

Another past attempt at revamping the frontend: #1334 by @Corvince.

if self.application.is_debug_mode():
self.application.stop()

else:
if self.application.verbose:
print("Unexpected message!")
Expand Down Expand Up @@ -407,7 +416,13 @@ def launch(self, port=None, open_browser=True):
try:
tornado.ioloop.IOLoop.current().start()
except KeyboardInterrupt:
tornado.ioloop.IOLoop.current().stop()
self.stop()

def stop(self):
tornado.ioloop.IOLoop.current().stop()

def is_debug_mode(self):
return bool(self.settings["debug"])

@staticmethod
def _is_stylesheet(filename):
Expand Down
30 changes: 28 additions & 2 deletions mesa/visualization/templates/js/runcontrol.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const vizElements = [];
const startModelButton = document.getElementById("play-pause");
const stepModelButton = document.getElementById("step");
const resetModelButton = document.getElementById("reset");
const shutDownButton = document.getElementById("shut-down");
const stepDisplay = document.getElementById("currentStep");

/**
Expand Down Expand Up @@ -67,6 +68,16 @@ function ModelController(tick = 0, fps = 3, running = false, finished = false) {
send({ type: "reset" });
};

this.shutDown = function shutDown() {
disableNavItem(startModelButton);
disableNavItem(stepModelButton);
disableNavItem(resetModelButton);
disableNavItem(shutDownButton);

addShutDownAlert();
send({ "type": "shut_down" });
};

/** Stops the model and put it into a finished state */
this.done = function done() {
this.stop();
Expand Down Expand Up @@ -124,15 +135,17 @@ stepModelButton.onclick = () => {
};
resetModelButton.onclick = () => controller.reset();

shutDownButton.onclick = () => controller.shutDown();

/*
* Websocket opening and message handling
*/

/** Open the websocket connection; support TLS-specific URLs when appropriate */
const ws = new WebSocket(
(window.location.protocol === "https:" ? "wss://" : "ws://") +
location.host +
"/ws"
location.host +
"/ws"
);

/**
Expand Down Expand Up @@ -171,6 +184,19 @@ const send = function (message) {
ws.send(msg);
};

const disableNavItem = function (navItem) {
const navLink = navItem.querySelector(".nav-link");
navLink.classList.add("disabled");
navItem.onclick = function () { };
};

const addShutDownAlert = function () {
const statusInfo = document.getElementById('status-info');
const alertElement = document.createElement('div');
alertElement.innerHTML = '<div class="alert alert-danger" role="alert">The server has been shut down.</div>';
statusInfo.appendChild(alertElement);
}

/*
* GUI initialization (for input parameters)
*/
Expand Down
4 changes: 4 additions & 0 deletions mesa/visualization/templates/modular_template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,17 @@
</li>
<li id="reset" class="nav-item"><a href="#" class="nav-link">Reset</a>
</li>
<li id="shut-down" class="nav-item"><a href="#" class="nav-link">Shut Down</a>
</li>
</ul>
</div>
</div>
</nav>
<div class="container d-flex flex-row">
<div class="col-xl-4 col-lg-4 col-md-4 col-3" id="sidebar"></div>
<div class="col-xl-8 col-lg-8 col-md-8 col-9" id="elements">
<div id="status-info">
</div>
<div id="elements-topbar">
<div>
<label class="badge bg-primary" for="fps" style="margin-right: 15px">Frames Per Second</label>
Expand Down