Allow Ktile vis server to work when Jupyter notebook is run on arbitrary port #144
Conversation
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
|
There is a question about whether it is worthwhile to eliminate the handlers that exist, as far as I can tell, in order to register and delete layers and kernels. There is a simplified version of this PR that can be made which only communicates port information. I have yet to remove the handlers in question, so I'm not entirely sure if their existence is superfluous in the presence of this change, or if their GET methods are being called from other places in the code aside from the vis server. |
…ironments Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
|
@jpolchlo Thanks for this PR, it looks like a really great start. I haven't had a chance to pull this down and play around with it but I will in the next day or two. |
|
Just for context, I did what I could figure to get the POST method to work, but operating within a JupyterHub environment requires an XSRF token to accompany the POST. That token should be acquired via a call to At any rate, the fact that these tokens are required or not depending on how Jupyter was launched (hub or non) gives me the willies. It makes the 0MQ solution seem less like overengineering. If it works out, then I wonder if all but the KtileTileHandler should be eliminated? |
kotfic
left a comment
There was a problem hiding this comment.
@jpolchlo this is looking AWESOME! I've left a few comments, mostly nitpicks and one refactor.
I know there isn't really a way to easily unit test this code, but it would be great if you could create a notebook in tests/integration that exercised some of this functionality and runs a few asserts. The goal is just to smoke test the functionality
Finally, I know documentation isn't in a great state right now. But if you could just write three or four sentences about this approach and drop it in a comment that would be a big help. I can integrate it into the documentation when i change some of the diagrams to reflect the changes in this PR.
Thanks again!
geonotebook/vis/ktile/ktile.py
Outdated
| def default_cache(self): | ||
| return dict(self.config.items(self.default_cache_section)) | ||
|
|
||
| def webapp_comm(self, msg): |
There was a problem hiding this comment.
Can we rename to webapp_comm_send?
geonotebook/vis/ktile/ktile.py
Outdated
| os.remove("/tmp/{}".format(user)) | ||
| except OSError: | ||
| pass | ||
| socket.bind("ipc:///tmp/{}".format(user)) |
There was a problem hiding this comment.
Can we also namespace the connection file by kernel_id (see get_kernel_id())? Otherwise a user running multiple kernels is going to run into trouble. Also let's use tempfile.gettempdir() instead of /tmp
There was a problem hiding this comment.
Namespacing to the kernel_id may not be necessary. The intent of the comms channel is to provide a per-user config repository; the individual accesses to the config repo should be properly scoped to the relevant kernel.
geonotebook/vis/ktile/ktile.py
Outdated
| socket.bind("ipc:///tmp/{}".format(user)) | ||
| stream = zmqstream.ZMQStream(socket, io_loop) | ||
|
|
||
| def _recv(msg): |
There was a problem hiding this comment.
I'd like to pull this _recv function out into a class in geontoebook/vis/utils.py
Something like:
def make_callback(transport, protocol):
def _recv(msg):
transport.send(protocol(msg))
return _recv
class WebAppProtocol(object):
def __init__(self, nbapp):
self.nbapp = nbapp
def __call__(self, msg):
data = pickle.loads(msg[0])
action = data.get('action', 'err_missing_action')
try:
return getattr(cls, action)(**data)
except Exception as e:
return {
'success': False,
'err_msg': 'Unidentified protocol error: {}'.format(str(e))
}
def register_kernel(self, **data):
kernel_id = data['kernel_id']
kwargs = {} if 'json' not in data else data['json']
try:
self.nbapp.web_app.ktile_config_manager.add_config(kernel_id, **kwargs)
return { 'success': True }
except Exception as exc:
return { 'success': False,
'err_msg': exc
}
def delete_kernel(self, **data):
kernel_id = data['kernel_id']
try:
del self.nbapp.web_app.ktile_config_manager[kernel_id]
return { 'success': True }
except KeyError:
return { 'success': False,
'err_msg': u'Kernel %s not found' % kernel_id
}
def add_layer(self, **data):
try:
kernel_id = data['kernel_id']
layer_name = data['layer_name']
self.nbapp.web_app.ktile_config_manager.add_layer(kernel_id, layer_name, data['json'])
return { 'success': True }
except Exception:
import sys
import traceback
t, v, tb = sys.exc_info()
return { 'success': False,
'err_msg': traceback.format_exception(t, v, tb)
}
def request_port(self, **data):
return { 'port': self.nbapp.port }
def err_missing_action(self, **data):
return {
'success': False,
'err_msg': 'Unrecognized request, no action'
}This lets us extend the protocol more easily without it becoming a huge pile of spaghetti
geonotebook/vis/ktile/ktile.py
Outdated
| log.info("Sending response: {}".format(result)) | ||
| stream.send(pickle.dumps(result)) | ||
|
|
||
| stream.on_recv(_recv) |
There was a problem hiding this comment.
Given the above class, this becomes stream.on_recv(make_callback(stream, WebAppProtocol(nbapp)) Assuming we tweak the call to initialize_webapp() to just take the conf and the nbapp instead of the webapp, and kwargs for the log and port.
geonotebook/vis/ktile/ktile.py
Outdated
| "class": "geonotebook.vis.ktile.provider:MapnikPythonProvider", | ||
| "kwargs": options | ||
| port_request = self.webapp_comm({ 'action': 'request_port' }) | ||
| base_url = 'http://127.0.0.1:{}/ktile/{}/{}'.format(port_request['port'], kernel_id, name) |
There was a problem hiding this comment.
I think we can use webapp.settings['base_url'] here instead of 127.0.0.1
There was a problem hiding this comment.
In fact, webapp.settings['base_url'] == '/' and seems to have no knowledge of the host. Localhost seems to be the best default, but I'm sure there are scenarios where this will fail. I'll entertain other suggestions on this point if you have them.
There was a problem hiding this comment.
So, scratch that. The webapp-provided base URL is fine. Eliminates the need for the port at all, and makes the extension agnostic to whether it is being run under JupyterHub or vanilla Jupyter.
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
|
@jpolchlo This is looking really good to me. It seems like i neglected to turn on CI builds for forked repos, and it doesn't look like there is a way i can trigger the build now that i've turned it on. Can you push a trivial change just so we can run this through the CI testing? Thanks! |
|
@jpolchlo I am seeing a slightly strange issue and i'm wondering if you can see if you can replicate. When I build the docker container on this branch and run the tests in If you could just build the docker container, mount |
|
I do observe that behavior in the container, but not, oddly, on my local machine. The comm request which adds the layer (the new replacement for the old {
'action': 'add_layer',
'json': {
'provider': {
'class': 'geonotebook.vis.ktile.provider:MapnikPythonProvider',
'kwargs': {
'attribution': None,
'bands': (1, 2, 3),
'colormap': [],
'dtype': 'float32',
'gamma': 1.0,
'interval': None,
'layer_type': None,
'name': 'WELD_8701612853329736007',
'nodata': -9999.0,
'opacity': 1.0,
'path': '/home/jpolchlopek/work/geonotebook/tests/integration/data/WELD.tif',
'projection': 'EPSG:3857',
'raster_x_size': 7243,
'raster_y_size': 1900,
'transform': (-123.06229313641593, 0.0007517610843588367, 0.0, 48.571429239198785, 0.0, -0.0007517610843588367),
'zIndex': 0
}
}
},
'kernel_id': 'dad2b4ef-fff7-4022-9fca-02e4722979ee',
'layer_name': 'WELD_8701612853329736007'
}Only the name field is slightly different, as expected.
DOCKER CONTAINER LOCAL MACHINE
---------------- -------------
affine==2.1.0 affine==2.1.0
ansible==2.1.1.0 ansible==2.1.1.0
* asn1crypto==0.23.0 asn1crypto==0.22.0
astroid==1.5.3
backports-abc==0.5 backports-abc==0.5
backports.shutil-get-terminal-size==1.0.0
bcrypt==3.1.3 bcrypt==3.1.3
beautifulsoup4==4.4.1
* bleach==2.1.1 bleach==2.0.0
boost==0.1
* certifi==2017.7.27.1 certifi==2017.4.17
* cffi==1.11.0 cffi==1.10.0
* chardet==2.3.0 chardet==3.0.4
click==6.7 click==6.7
click-plugins==1.0.3 click-plugins==1.0.3
cligj==0.4.0 cligj==0.4.0
configparser==3.5.0
* cryptography==2.0.3 cryptography==2.0.2
cvxopt==1.1.4
cycler==0.10.0 cycler==0.10.0
Cython==0.23.4
dateutils==0.6.6
decorator==4.1.2 decorator==4.1.2
entrypoints==0.2.3 entrypoints==0.2.3
enum34==1.1.6
Fiona==1.7.1 Fiona==1.7.1
Flask==0.12.2
functools32==3.2.3.post2
futures==3.0.5 futures==3.0.5
GDAL==2.1.0 GDAL==2.1.0
* geonotebook==0.0.0 e git+git@github.com:jpolchlo/geonotebook.git@151563bb91a1f9339f698ba2781d772f07542c06#egg=geonotebook
geopyspark==0.2.0rc5
gevent==1.2.2
greenlet==0.4.12
* html5lib==1.0b10 html5lib==0.999999999
* idna==2.6 idna==2.5
ipaddress==1.0.18
* ipykernel==4.5.2 ipykernel==4.6.1
* ipython==5.5.0 ipython==6.1.0
ipython-genutils==0.2.0 ipython-genutils==0.2.0
* ipywidgets==7.0.1 ipywidgets==7.0.0
isort==4.2.15
itsdangerous==0.24
jdcal==1.0
jedi==0.10.2
Jinja2==2.9.6 Jinja2==2.9.6
joblib==0.9.4
jsonschema==2.6.0 jsonschema==2.6.0
jupyter==1.0.0
jupyter-client==5.1.0 jupyter-client==5.1.0
jupyter-console==5.2.0
jupyter-core==4.3.0 jupyter-core==4.3.0
lazy-object-proxy==1.3.1
* lxml==4.0.0 lxml==3.8.0
mapnik==0.1 mapnik==0.1
MarkupSafe==1.0 MarkupSafe==1.0
Mastodon.py==1.0.8
matplotlib==2.0.2 matplotlib==2.0.2
mccabe==0.6.1
mistune==0.7.4 mistune==0.7.4
ModestMaps==1.4.7 ModestMaps==1.4.7
munch==2.2.0 munch==2.2.0
* nbconvert==5.3.1 nbconvert==5.2.1
* nbformat==4.4.0 nbformat==4.3.0
ndg-httpsclient==0.4.2
networkx==2.0
nose==1.3.7
* notebook==5.1.0 notebook==5.0.0
npm==0.1.1
numexpr==2.4.3
* numpy==1.13.3 numpy==1.11.1
olefile==0.44 olefile==0.44
openpyxl==2.3.0
optional-django==0.1.0
pandas==0.20.3 pandas==0.20.3
pandocfilters==1.4.2 pandocfilters==1.4.2
* paramiko==2.3.1 paramiko==2.2.1
pathlib2==2.3.0
patsy==0.4.1
pexpect==4.2.1 pexpect==4.2.1
pickleshare==0.7.4 pickleshare==0.7.4
* Pillow==4.3.0 Pillow==4.1.1
ply==3.7
promise==0.4.2 promise==0.4.2
prompt-toolkit==1.0.15 prompt-toolkit==1.0.15
* protobuf==3.1.0 protobuf==3.3.0
ptyprocess==0.5.2 ptyprocess==0.5.2
* py==1.4.31 py==1.4.34
* pyasn1==0.3.6 pyasn1==0.3.1
* pycparser==2.18 pycparser==2.17
pycrypto==2.6.1 pycrypto==2.6.1
Pygments==2.2.0 Pygments==2.2.0
pylint==1.7.2
PyNaCl==1.1.2 PyNaCl==1.1.2
* pyOpenSSL==17.3.0 pyOpenSSL==17.2.0
pyparsing==2.2.0 pyparsing==2.2.0
pyproj==1.9.5.1 pyproj==1.9.5.1
* pytest==2.8.7 pytest==3.1.3
python-dateutil==2.6.1 python-dateutil==2.6.1
python-pam==1.8.2
pytz==2017.2 pytz==2017.2
PyWavelets==0.5.2
PyYAML==3.12 PyYAML==3.12
pyzmq==16.0.2 pyzmq==16.0.2
qtconsole==4.3.1 qtconsole==4.3.1
rasterio==0.36.0 rasterio==0.36.0
requests==2.11.1 requests==2.11.1
scandir==1.6
scikit-image==0.13.1
scikit-learn==0.17
scipy==0.19.1 scipy==0.19.1
seaborn==0.6.0
* Shapely==1.5.17 Shapely==1.6b4
simplegeneric==0.8.1 simplegeneric==0.8.1
* simplejson==3.8.1 simplejson==3.10.0
singledispatch==3.4.0.3
six==1.10.0 six==1.10.0
snuggs==1.4.1 snuggs==1.4.1
SQLAlchemy==1.1.11
statsmodels==0.6.1
subprocess32==3.2.7
tables==3.2.2
terminado==0.6 terminado==0.6
testpath==0.3.1 testpath==0.3.1
TileStache==1.51.6 TileStache==1.51.6
* tornado==4.5.2 tornado==4.5.1
traitlets==4.3.2 traitlets==4.3.2
typing==3.6.2
urllib3==1.21.1
wcwidth==0.1.7 wcwidth==0.1.7
webencodings==0.5.1 webencodings==0.5.1
* Werkzeug==0.11.13 Werkzeug==0.12.2
* widgetsnbextension==3.0.3 widgetsnbextension==3.0.2
wrapt==1.10.11
xlrd==0.9.4
xlwt==0.7.5Surely there is other information that might be useful. Let me know what you'd like to see. |
|
@jpolchlo can we close this issue? |
Previously, it was required to specify a
urlin the[ktile]section of the geonotebook.ini file, which essentially set the port number that the Ktile vis server would look for the Jupyter notebook instance on. This meant that notebook instances launched with a--portthat did not match that in theurlwould fail to display layer data. This affected any notebook instance launched from JupyterHub.To determine the port on which the Tornado webapp was running, I used of a ZeroMQ socket plugged into the Tornado event loop. Now, it is possible to make queries on port 14253 to determine the port number of the running process.
Along the way, I used this 0MQ system to eliminate the use of Python's requests library in the ktile server. This ought to allow for a simplified handler structure in the webapp, and should remove a layer of indirection.
On the downside, this mod won't allow multiple users on the same system to run concurrently due to contention for the port that 0MQ is listening on.