-
Notifications
You must be signed in to change notification settings - Fork 249
Conversation
Installs Python 3 and py3 versions of our supported packages, including the python3 jupyter kernel, and adds a dropdown to the notebook page to allow users to switch kernels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good overall, except for a few UI things I think must've broken.
Are we ready to make these changes though?
containers/base/Dockerfile
Outdated
pip3 install -U --upgrade-strategy only-if-needed --no-cache-dir seaborn==0.7.0 && \ | ||
pip3 install -U --upgrade-strategy only-if-needed --no-cache-dir PyYAML==3.11 && \ | ||
pip3 install -U --upgrade-strategy only-if-needed --no-cache-dir six==1.10.0 && \ | ||
# Install notebook after ipywidgets, and keep it pinned to 4.2.3, higher versions may |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should move down to line 136?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or be removed entirely, since we have a comment below explaining we're overriding this anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,175 @@ | |||
# Copyright 2015 Google Inc. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes here from the original Dockerfile, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is just a branch.
containers/base/build.sh
Outdated
@@ -32,3 +32,4 @@ fi | |||
|
|||
trap 'rm -rf pydatalab' exit | |||
docker build ${DOCKER_BUILD_ARGS} -t datalab-base . | |||
docker build ${DOCKER_BUILD_ARGS} -t datalab-base-py2 -f Dockerfile.py2 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script is used for local development only, not release, so we should just put datalab-base
here, and maybe add an optional argument to also build datalab-base-py2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
containers/datalab/build.sh
Outdated
@@ -34,6 +34,7 @@ fi | |||
cd $(dirname $0) | |||
|
|||
cat Dockerfile.in | sed $VERSION_SUBSTITUTION | sed $COMMIT_SUBSTITUTION > Dockerfile | |||
cat Dockerfile.in | sed $VERSION_SUBSTITUTION | sed $COMMIT_SUBSTITUTION | sed "s/datalab-base/datalab-base-py2/" > Dockerfile.py2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above for this file, I think we should only build the py2py3 docker file for local development, and have the py2 image built on demand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
<span class="toolbar-text">Kernel: </span> | ||
<span id="currentKernelName"></span> | ||
</button> | ||
<ul class="dropdown-menu" id="kernelSelectorDropdown"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a screenshot for this to start discussion on the design? I didn't actually put much thought into how usable this is at the time I made those changes, it was the simplest thing to do for switching kernels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -161,6 +161,15 @@ | |||
<li id="interruptKernelButton"><a href="#">Interrupt Execution</a></li> | |||
</ul> | |||
</div> | |||
<div class="btn-group"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding another button to the toolbar will likely need changes to the minimum width of screen, and the media size decorators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Did some investigation with the first official build that has this. Docker image compressed on GCR is 790MB vs 566 MB for the previous version. Expectation is that initial startup time for VMs is increased by ~15s. RAM usage didn't seem to change significantly, this and the previous version appeared to use within tens of MB of each other. |
That's good to hear that there are no immediate significant issues. |
Installs Python 3 and py3 versions of our supported packages,
including the python3 jupyter kernel, and adds a dropdown to
the notebook page to allow users to switch kernels.
Docker Image size goes from 1.734GB -> 2.507GB, almost all of which is due to the size of the python3 packages. I've added a "py2 only" container build in case someone needs the smaller image.
Fixes #902
Somewhat based on yebrahim's py3 branch:
master...yebrahim:yebrahim/py3