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

Added shutting down calls to the visualization server #241

Merged
merged 13 commits into from
Jun 18, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
Added shutting down calls to the visualization server
Fixes #232
  • Loading branch information
sahilshekhawat committed Jun 17, 2015
commit f7474d93908acbbc1a4a241a5b568c1f9f1ee6de
5 changes: 3 additions & 2 deletions pydy/viz/scene.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

# local
from .camera import PerspectiveCamera
from .server import run_server
from .server import Server
from .light import PointLight
from ..system import System
from ..utils import PyDyImportWarning
Expand Down Expand Up @@ -463,7 +463,8 @@ def remove_static_html(self, force=False):
def display(self):
"""Displays the scene in the default webbrowser."""
self.create_static_html()
run_server(scene_file=self._scene_json_file)
server = Server(scene_file=self._scene_json_file)
server.run_server()

def _rerun_button_callback(self, btn):
"""Callback for the "Rerun Simulation" button. When executed the
Expand Down
134 changes: 101 additions & 33 deletions pydy/viz/server.py
Original file line number Diff line number Diff line change
@@ -1,37 +1,105 @@
#!/usr/bin/env python

from __future__ import print_function
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't do this. Just make sure to use parentheses on all print statements.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay


import os
import sys
import signal
Copy link
Member

Choose a reason for hiding this comment

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

This needs to stay in for Python 2/3 compat.

Copy link
Member Author

Choose a reason for hiding this comment

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

I totally missed that. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Maybe make a comment in your PR that says why that is there.

import socket
import webbrowser
if sys.version_info < (3, 0):
from SimpleHTTPServer import SimpleHTTPRequestHandler
from BaseHTTPServer import HTTPServer
else:
from http.server import SimpleHTTPRequestHandler
from http.server import HTTPServer


__all__ = ['run_server']


def run_server(port=8000,scene_file="Null"):
#change dir to static first.
os.chdir("static/")
HandlerClass = SimpleHTTPRequestHandler
ServerClass = HTTPServer
Protocol = "HTTP/1.0"
server_address = ('127.0.0.1', port)
HandlerClass.protocol_version = Protocol
httpd = ServerClass(server_address, HandlerClass)
sa = httpd.socket.getsockname()
print("Serving HTTP on", sa[0], "port", sa[1], "...")
print("hit ctrl+c to stop the server..")
print("To view visualization, open:\n")
url = "http://localhost:"+ str(sa[1]) + "/index.html?load=" + scene_file
print(url)
webbrowser.open(url)
httpd.serve_forever()


if __name__ == "__main__":
run_server()
import BaseHTTPServer
from SimpleHTTPServer import SimpleHTTPRequestHandler


__all__ = ['Server']


class StoppableHTTPServer(BaseHTTPServer.HTTPServer):
"""
Overrides BaseHTTPServer.HTTPServer to include a stop
function.
"""

def server_bind(self):
BaseHTTPServer.HTTPServer.server_bind(self)
self.socket.settimeout(1)
self.run = True

def get_request(self):
while self.run:
try:
sock, addr = self.socket.accept()
sock.settimeout(None)
return (sock, addr)
except socket.timeout:
pass

def stop(self):
self.run = False

def serve(self):
while self.run:
try:
self.handle_request()
except TypeError:
# When server is being closed, while loop can run once
# after setting self.run = False depending on how it
# is scheduled.
pass


class Server(object):
"""
Parameters
----------
:param port : integer
Copy link
Member

Choose a reason for hiding this comment

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

We try to follow the numpydoc style for docstrings, so this would be:

Parameters
----------
port : integer
    Defines the port on which...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Defines the port on which the server will run.
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't the case if the port is taken.

:param scene_file : name of the scene_file generated for visualization
Used here to display the url
:param directory : path of the directory which contains static and scene files.
Server is started in this directory itself.

Example
-------
>>> server = Server(scene_file=_scene_json_file)
Copy link
Member

Choose a reason for hiding this comment

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

Don't indent here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

>>> server.run_server()

"""
def __init__(self, port=8000, scene_file="Null"):
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't "Null" None?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we need to concatenate it with a string and None will raise an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, user must know that the there is no file name.

Copy link
Member

Choose a reason for hiding this comment

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

"Null" is just very anit-python-pattern. I don't like it. None should be used and then handled correctly in the code in __init__., but my other suggestion is that this should be a required argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

And I don't know it yet but the url we get from "Null" can be used to give user an option to choose the json files like in ipython notebook.

Copy link
Member

Choose a reason for hiding this comment

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

We can worry about that when the feature is implemented. I think scene_file should be a required arg.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I change it to None?

Copy link
Member Author

Choose a reason for hiding this comment

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

or make it a required argument?

Copy link
Member

Choose a reason for hiding this comment

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

Seems to me like it is a requirement. What use is the server if I don't have a scene file? I guess you could load it from a url ?scene_file=... after running the server. If you want to support that use case, then you can make it a kwarg and =None or =""but not="Null". I'd preferNone` as it follows common python patterns.

self.port = port
self.scene_file = scene_file
Copy link
Member

Choose a reason for hiding this comment

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

My suggestion in the previous commit was to have this call sig:

def __init__(scene_file, directory=None, port=8000)

The scene file should be required and the directory can default to the cwd if None is passed in.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add that in my new pr on static directory creation. Trying to make it atomic

Copy link
Member

Choose a reason for hiding this comment

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

But this is a whole new server code. It seems atomic to add this.

self.directory = "static/"
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 should be pass in as an argument.


def run_server(self):
Copy link
Member

Choose a reason for hiding this comment

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

What happens if port 8000 is taken?

Copy link
Member Author

Choose a reason for hiding this comment

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

It throws an error. This needs to be fixed.

# Change dir to static first.
os.chdir(self.directory)
handler_class = SimpleHTTPRequestHandler
server_class = StoppableHTTPServer
protocol = "HTTP/1.0"
server_address = ('127.0.0.1', self.port)
handler_class.protocol_version = protocol
self.httpd = server_class(server_address, handler_class)
sa = self.httpd.socket.getsockname()
print("Serving HTTP on", sa[0], "port", sa[1], "...")
print("To view visualization, open:\n")
url = "http://localhost:"+str(sa[1]) + "/index.html?load=" + \
self.scene_file
print(url)
webbrowser.open(url)
print("Hit Ctrl+C to stop the server...")
signal.signal(signal.SIGINT, self.stop_server)
self.httpd.serve()

def stop_server(self, signal, frame):
"""
Confirms and stops the visulisation server
:param signal:
Required by signal.signal
:param frame:
Required by signal.signal

"""
res = raw_input("Shutdown this visualization server ([y]/n)? ")
Copy link
Member

Choose a reason for hiding this comment

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

This will fail in Python 3. Oliver had the fix for this in this file before this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

What are the options then, should I use input()?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it!

if res is (None or 'y'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this eval to True if the user inputs 'Y'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, I must add it as well

print("Shutdown confirmed")
print("Shutting down server...")
self.httpd.stop()