-
Notifications
You must be signed in to change notification settings - Fork 23
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
Change internal container ports 2.0 #181
base: main
Are you sure you want to change the base?
Conversation
@Abby-Wheelis @jpfleischer I am finally getting caught up with code reviews in the plane on the way to DC, and I am not sure if/how this works. I don't see any "Testing done" in the comments, and I don't see how this mapping will fix anything, given that the http server is still starting at port The intent behind the change is good, and there is a way to make it work, but I don't think this is it.
Unless one of you can show that it works, or plans to fix this as a way to better understand docker, I am going to close this PR. |
Hi all,
My PR changes the logs to say 3274 because it changes the http-server to serve on port 3274. In main it serves on 6060. The port 6060 is arbitrarily taken from an example prg and can be changed. This makes it more straightforward. |
Log messages I see when I run for the notebooks
Copy pasted url works, same as when I used to change it manually. There is both a message to change the port and the port is changed in the log message. We probably only need one or the other of those things. for the dashboard
|
Hi Abby, I am getting some different output in my branch called dashboard-1 | Available on:
dashboard-1 | http://127.0.0.1:3274
dashboard-1 | http://172.21.0.4:3274 The first one works, but just like main, the second link won't work as it is using the docker container's hostname which won't be accessible on host. I tried looking for a solution for http-server to stop printing that second useless link but it doesn't exist. Still, the advantage is that the user will see the correct port from the get-go and does not need to do manual substitution for the port. For the notebook server, I just pushed a commit that will give us only one correct URL instead of that second one (it also printed one with the docker container's hostname) so it looks like this now notebook-server-1 | [I 05:02:57.841 NotebookApp] Jupyter Notebook 6.5.7 is running at:
notebook-server-1 | [I 05:02:57.841 NotebookApp] localhost:47962/?token=70a243abdf4fb6f516501d1e37df84439ab54f579f97c969
notebook-server-1 | [I 05:02:57.841 NotebookApp] Use Control-C to stop this server and shut down all kernels (twice to skip confirmation).
notebook-server-1 | [C 05:02:57.844 NotebookApp]
notebook-server-1 | Thanks to diff --git a/viz_scripts/docker/start_notebook.sh b/viz_scripts/docker/start_notebook.sh
index e0deb7e..4bbb171 100755
--- a/viz_scripts/docker/start_notebook.sh
+++ b/viz_scripts/docker/start_notebook.sh
@@ -29,8 +29,8 @@ cd saved-notebooks
# launch the notebook server
# tail -f /dev/null
if [ -z ${CRON_MODE} ] ; then
- echo "Running notebook in docker, change host:port to localhost:47962 in the URL below"
- PYTHONPATH=/usr/src/app jupyter notebook --no-browser --ip=0.0.0.0 --port=47962 --allow-root
+ echo "Running notebook in docker..."
+ PYTHONPATH=/usr/src/app jupyter notebook --no-browser --ip=0.0.0.0 --port=47962 --NotebookApp.custom_display_url=localhost:47962 --allow-root
else
echo "Running crontab without user interaction, setting python path"
export PYTHONPATH=/usr/src/app |
I just tested again, and am seeing the same results as described above. The dashboard works the same as it does on main, but I don't need to manually change 6060 to 3274. Same with the notebooks. Behavior wise, this looks good to me |
NOTE: The previous pull request was closed due to an accidental commit.
The intention of this PR is for the docker logs to tell me straightaway the right port to connect to (instead of having to remember that the right port is 47962).