From 18955c2fa9ab8a587df7f7a1eea1f7c4529821a1 Mon Sep 17 00:00:00 2001 From: Tom Palmer Date: Thu, 12 Sep 2024 09:56:46 +0100 Subject: [PATCH 01/12] feat: add rstudio subcommand --- opensafely/__init__.py | 2 + opensafely/rstudio.py | 137 +++++++++++++++++++++++++++++++++++++++++ tests/test_rstudio.py | 38 ++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 opensafely/rstudio.py create mode 100644 tests/test_rstudio.py diff --git a/opensafely/__init__.py b/opensafely/__init__.py index 4825306..5ddc630 100644 --- a/opensafely/__init__.py +++ b/opensafely/__init__.py @@ -22,6 +22,7 @@ info, jupyter, pull, + rstudio, unzip, upgrade, ) @@ -141,6 +142,7 @@ def add_subcommand(cmd, module): add_subcommand("info", info) add_subcommand("exec", execute) add_subcommand("clean", clean) + add_subcommand("rstudio", rstudio) warn_if_updates_needed(sys.argv) args = parser.parse_args() diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py new file mode 100644 index 0000000..1791bfb --- /dev/null +++ b/opensafely/rstudio.py @@ -0,0 +1,137 @@ +import os +import socket +import sys +import threading +import time +import webbrowser +from pathlib import Path +from sys import platform +from urllib import request + +from opensafely import utils + + +DESCRIPTION = "Run an RStudio Server session using the OpenSAFELY environment" + + +# poor mans debugging because debugging threads on windows is hard +if os.environ.get("DEBUG", False): + + def debug(msg): + # threaded output for some reason needs the carriage return or else + # it doesn't reset the cursor. + sys.stderr.write("DEBUG: " + msg.replace("\n", "\r\n") + "\r\n") + sys.stderr.flush() + +else: + + def debug(msg): + pass + + +def add_arguments(parser): + parser.add_argument( + "--directory", + "-d", + default=os.getcwd(), + type=Path, + help="Directory to run the RStudio Server session in (default is current dir)", + ) + parser.add_argument( + "--name", + help="Name of docker image (defaults to use directory name)", + default="rstudio", + ) + + parser.add_argument( + "--port", + "-p", + default=8787, + help="Port to run on", + ) + + +def open_browser(name, port): + try: + url = f"http://localhost:{port}" + debug(f"open_browser: url={url}") + + # wait for port to be open + debug("open_browser: waiting for port") + start = time.time() + while time.time() - start < 60.0: + try: + response = request.urlopen(url, timeout=1) + except (request.URLError, OSError): + pass + else: + break + + if not response: + debug("open_browser: open_browser: could not get response") + return + + # open a webbrowser pointing to the docker container + debug("open_browser: open_browser: opening browser window") + webbrowser.open(url, new=2) + + except Exception: + # reformat exception printing to work from thread + import traceback + + sys.stderr.write("Error in open browser thread:\r\n") + tb = traceback.format_exc().replace("\n", "\r\n") + sys.stderr.write(tb) + sys.stderr.flush() + + +def get_free_port(): + sock = socket.socket() + sock.bind(("127.0.0.1", 0)) + port = sock.getsockname()[1] + sock.close() + return port + + +def main(directory, name, port): + if name is None: + name = f"os-rstudio-{directory.name}" + + if port is None: + # this is a race condition, as something else could consume the socket + # before docker binds to it, but the chance of that on a user's + # personal machine is very small. + port = str(get_free_port()) + + # if not no_browser: + # start thread to open web browser + thread = threading.Thread(target=open_browser, args=(name, port), daemon=True) + thread.name = "browser thread" + debug("starting open_browser thread") + thread.start() + + # Determine if on Linux, if so obtain user id + # And need to know in Windows win32 for text file line endings setting + if platform == "linux": + uid = os.getuid() + else: + uid = None + + docker_args = [ + "--platform=linux/amd64", + f"-p={port}:8787", + f"--name={name}", + f"--hostname={name}", + "--volume=" + + os.path.join(os.path.expanduser("~"), ".gitconfig") + + ":/home/rstudio/local-gitconfig", + f"--env=HOSTPLATFORM={platform}", + f"--env=HOSTUID={uid}", + ] + + debug("docker: " + " ".join(docker_args)) + ps = utils.run_docker( + docker_args, "rstudio", "", interactive=True, user="0:0", directory=directory + ) + # we want to exit with the same code that rstudio-server did + return ps.returncode diff --git a/tests/test_rstudio.py b/tests/test_rstudio.py new file mode 100644 index 0000000..5686b65 --- /dev/null +++ b/tests/test_rstudio.py @@ -0,0 +1,38 @@ +import os +import pathlib +from sys import platform + +from opensafely import rstudio +from tests.conftest import run_main + + +def test_rstudio(run): + if platform == "linux": + uid = os.getuid() + else: + uid = None + + run.expect( + [ + "docker", + "run", + "--rm", + "--init", + "--label=opensafely", + "--interactive", + "--user=0:0", + f"--volume={pathlib.Path.cwd()}://workspace", + "--platform=linux/amd64", + "-p=8787:8787", + "--name=test_rstudio", + "--hostname=test_rstudio", + "--volume=" + + os.path.join(os.path.expanduser("~"), ".gitconfig") + + ":/home/rstudio/local-gitconfig", + "--env=HOSTPLATFORM=" + platform, + f"--env=HOSTUID={uid}", + "ghcr.io/opensafely-core/rstudio", + ] + ) + + assert run_main(rstudio, "--port 8787 --name test_rstudio") == 0 From 4e1900e3e988f4882feeb13407e99564c442fa20 Mon Sep 17 00:00:00 2001 From: Tom Palmer Date: Wed, 16 Oct 2024 16:54:43 +0100 Subject: [PATCH 02/12] Remove default name --- opensafely/rstudio.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py index 1791bfb..24b13d7 100644 --- a/opensafely/rstudio.py +++ b/opensafely/rstudio.py @@ -38,9 +38,7 @@ def add_arguments(parser): help="Directory to run the RStudio Server session in (default is current dir)", ) parser.add_argument( - "--name", - help="Name of docker image (defaults to use directory name)", - default="rstudio", + "--name", help="Name of docker image (defaults to use directory name)" ) parser.add_argument( From 33708b313485f7fddbf909e4f96b3526e9ddeaa1 Mon Sep 17 00:00:00 2001 From: Tom Palmer Date: Wed, 16 Oct 2024 16:54:57 +0100 Subject: [PATCH 03/12] Remove default port --- opensafely/rstudio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py index 24b13d7..c5c349a 100644 --- a/opensafely/rstudio.py +++ b/opensafely/rstudio.py @@ -44,7 +44,7 @@ def add_arguments(parser): parser.add_argument( "--port", "-p", - default=8787, + default=None, help="Port to run on", ) From f037ba17e1e89a316c4519480ec4a1c556b2ded0 Mon Sep 17 00:00:00 2001 From: Tom Palmer Date: Wed, 16 Oct 2024 16:55:28 +0100 Subject: [PATCH 04/12] Add message about port --- opensafely/rstudio.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py index c5c349a..97c5863 100644 --- a/opensafely/rstudio.py +++ b/opensafely/rstudio.py @@ -128,6 +128,9 @@ def main(directory, name, port): ] debug("docker: " + " ".join(docker_args)) + print( + f"Opening an RStudio Server session at http://localhost:{port}/ , please press Ctrl+C here to end the session" + ) ps = utils.run_docker( docker_args, "rstudio", "", interactive=True, user="0:0", directory=directory ) From b4598c49969c3e2341ad8df44f474498b94c6bab Mon Sep 17 00:00:00 2001 From: Tom Palmer Date: Thu, 17 Oct 2024 08:41:50 +0100 Subject: [PATCH 05/12] Ignore .Rhistory In case when testing `opensafely rstudio` some R commands are entered into the R console, which will cause .Rhistory to be created. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 8873881..3a5e5e7 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,4 @@ venv/ .coverage tests/fixtures/projects/metadata/ tests/fixtures/projects/output/ +.Rhistory From c7231f1bbdb0b95e896210a07c03279f1c9a19f5 Mon Sep 17 00:00:00 2001 From: Tom Palmer Date: Thu, 17 Oct 2024 09:58:47 +0100 Subject: [PATCH 06/12] Edit message --- opensafely/rstudio.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py index 97c5863..f2b320a 100644 --- a/opensafely/rstudio.py +++ b/opensafely/rstudio.py @@ -129,7 +129,8 @@ def main(directory, name, port): debug("docker: " + " ".join(docker_args)) print( - f"Opening an RStudio Server session at http://localhost:{port}/ , please press Ctrl+C here to end the session" + f"Opening an RStudio Server session at http://localhost:{port}/ when " + "you are finished working please press Ctrl+C here to end the session" ) ps = utils.run_docker( docker_args, "rstudio", "", interactive=True, user="0:0", directory=directory From d9ad023a2181cbcf06c3931522840202ed9c5406 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Thu, 17 Oct 2024 13:42:08 +0100 Subject: [PATCH 07/12] Add --platform=linux/amd64 to default docker args This makes them all work macos m-series machines. --- opensafely/rstudio.py | 1 - opensafely/utils.py | 3 +++ tests/test_execute.py | 7 +++++++ tests/test_jupyter.py | 1 + tests/test_rstudio.py | 2 +- tests/test_utils.py | 4 ++++ 6 files changed, 16 insertions(+), 2 deletions(-) diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py index f2b320a..2fda8a6 100644 --- a/opensafely/rstudio.py +++ b/opensafely/rstudio.py @@ -116,7 +116,6 @@ def main(directory, name, port): uid = None docker_args = [ - "--platform=linux/amd64", f"-p={port}:8787", f"--name={name}", f"--hostname={name}", diff --git a/opensafely/utils.py b/opensafely/utils.py index b487b6c..0b2a88a 100644 --- a/opensafely/utils.py +++ b/opensafely/utils.py @@ -97,6 +97,9 @@ def run_docker( "--rm", "--init", "--label=opensafely", + # all our docker images are this platform + # helps when running on M-series macs. + "--platform=linux/amd64", ] if interactive: diff --git a/tests/test_execute.py b/tests/test_execute.py index df997fc..bb75dec 100644 --- a/tests/test_execute.py +++ b/tests/test_execute.py @@ -15,6 +15,7 @@ def test_execute_main_args(run, no_user): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", f"--volume={pathlib.Path.cwd()}://workspace", "--env", @@ -41,6 +42,7 @@ def test_execute_main_entrypoint(run, no_user): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", f"--volume={pathlib.Path.cwd()}://workspace", "--env", @@ -65,6 +67,7 @@ def test_execute_main_env(run, no_user): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", f"--volume={pathlib.Path.cwd()}://workspace", "--env", @@ -94,6 +97,7 @@ def test_execute_main_env_in_env(run, no_user): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", f"--volume={pathlib.Path.cwd()}://workspace", "--env", @@ -120,6 +124,7 @@ def test_execute_main_user_cli_arg_overrides(default, run, monkeypatch): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", "--user=1234:5678", f"--volume={pathlib.Path.cwd()}://workspace", @@ -146,6 +151,7 @@ def test_execute_main_user_linux_disble(run, monkeypatch): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", f"--volume={pathlib.Path.cwd()}://workspace", "--env", @@ -171,6 +177,7 @@ def test_execute_main_stata_license(run, monkeypatch, no_user): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", f"--volume={pathlib.Path.cwd()}://workspace", "--env", diff --git a/tests/test_jupyter.py b/tests/test_jupyter.py index b587368..fa6b180 100644 --- a/tests/test_jupyter.py +++ b/tests/test_jupyter.py @@ -12,6 +12,7 @@ def test_jupyter(run, no_user): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", f"--volume={pathlib.Path.cwd()}://workspace", "-p=1234:1234", diff --git a/tests/test_rstudio.py b/tests/test_rstudio.py index 5686b65..d421789 100644 --- a/tests/test_rstudio.py +++ b/tests/test_rstudio.py @@ -19,10 +19,10 @@ def test_rstudio(run): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", "--user=0:0", f"--volume={pathlib.Path.cwd()}://workspace", - "--platform=linux/amd64", "-p=8787:8787", "--name=test_rstudio", "--hostname=test_rstudio", diff --git a/tests/test_utils.py b/tests/test_utils.py index 78a4845..4c3ee9d 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -63,6 +63,7 @@ def test_run_docker_user_default(run, monkeypatch): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", f"--volume={pathlib.Path.cwd()}://workspace", "ghcr.io/opensafely-core/ehrql:v1", ], @@ -80,6 +81,7 @@ def test_run_docker_user_linux(run, monkeypatch): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--user=uid:gid", f"--volume={pathlib.Path.cwd()}://workspace", "ghcr.io/opensafely-core/ehrql:v1", @@ -96,6 +98,7 @@ def test_run_docker_interactive(run, no_user): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", f"--volume={pathlib.Path.cwd()}://workspace", "ghcr.io/opensafely-core/ehrql:v1", @@ -114,6 +117,7 @@ def test_run_docker_interactive_tty(run, no_user, monkeypatch): "--rm", "--init", "--label=opensafely", + "--platform=linux/amd64", "--interactive", "--tty", f"--volume={pathlib.Path.cwd()}://workspace", From 3917df794df6cb2174da2ed9f47b4d37a18a287c Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Thu, 17 Oct 2024 13:54:27 +0100 Subject: [PATCH 08/12] Move get_free_port and debug functions into utils.py --- opensafely/jupyter.py | 43 ++++++++++--------------------------------- opensafely/rstudio.py | 41 +++++++---------------------------------- opensafely/utils.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 67 deletions(-) diff --git a/opensafely/jupyter.py b/opensafely/jupyter.py index 3c806f6..386efd6 100644 --- a/opensafely/jupyter.py +++ b/opensafely/jupyter.py @@ -1,7 +1,6 @@ import argparse import json import os -import socket import subprocess import sys import threading @@ -16,21 +15,6 @@ DESCRIPTION = "Run a jupyter lab notebook using the OpenSAFELY environment" -# poor mans debugging because debugging threads on windows is hard -if os.environ.get("DEBUG", False): - - def debug(msg): - # threaded output for some reason needs the carriage return or else - # it doesn't reset the cursor. - sys.stderr.write("DEBUG: " + msg.replace("\n", "\r\n") + "\r\n") - sys.stderr.flush() - -else: - - def debug(msg): - pass - - def add_arguments(parser): parser.add_argument( "--directory", @@ -80,20 +64,20 @@ def open_browser(name, port): capture_output=True, ) if ps.returncode == 0: - debug(ps.stdout) + utils.debug(ps.stdout) metadata = json.loads(ps.stdout) else: time.sleep(1) if metadata is None: - debug("open_browser: Could not get metadata") + utils.debug("get_metadata: Could not get metadata") return url = f"http://localhost:{port}/?token={metadata['token']}" - debug(f"open_browser: url={url}") + utils.debug(f"open_browser: url={url}") # wait for port to be open - debug("open_browser: waiting for port") + utils.debug("open_browser: waiting for port") start = time.time() while time.time() - start < 60.0: try: @@ -104,11 +88,11 @@ def open_browser(name, port): break if not response: - debug("open_browser: open_browser: could not get response") + utils.debug("open_browser: open_browser: could not get response") return # open a webbrowser pointing to the docker container - debug("open_browser: open_browser: opening browser window") + utils.debug("open_browser: open_browser: opening browser window") webbrowser.open(url, new=2) except Exception: @@ -121,14 +105,6 @@ def open_browser(name, port): sys.stderr.flush() -def get_free_port(): - sock = socket.socket() - sock.bind(("127.0.0.1", 0)) - port = sock.getsockname()[1] - sock.close() - return port - - def main(directory, name, port, no_browser, jupyter_args): if name is None: name = f"os-jupyter-{directory.name}" @@ -137,7 +113,7 @@ def main(directory, name, port, no_browser, jupyter_args): # this is a race condition, as something else could consume the socket # before docker binds to it, but the chance of that on a user's # personal machine is very small. - port = str(get_free_port()) + port = str(utils.get_free_port()) jupyter_cmd = [ "jupyter", @@ -158,7 +134,7 @@ def main(directory, name, port, no_browser, jupyter_args): # start thread to open web browser thread = threading.Thread(target=open_browser, args=(name, port), daemon=True) thread.name = "browser thread" - debug("starting open_browser thread") + utils.debug("starting open_browser thread") thread.start() docker_args = [ @@ -174,7 +150,8 @@ def main(directory, name, port, no_browser, jupyter_args): "PYTHONPATH=/workspace", ] - debug("docker: " + " ".join(docker_args)) + utils.debug("docker: " + " ".join(docker_args)) + ps = utils.run_docker(docker_args, "python", jupyter_cmd, interactive=True) ps = utils.run_docker( docker_args, "python", jupyter_cmd, interactive=True, directory=directory ) diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py index 2fda8a6..2e69de2 100644 --- a/opensafely/rstudio.py +++ b/opensafely/rstudio.py @@ -1,5 +1,4 @@ import os -import socket import sys import threading import time @@ -14,21 +13,6 @@ DESCRIPTION = "Run an RStudio Server session using the OpenSAFELY environment" -# poor mans debugging because debugging threads on windows is hard -if os.environ.get("DEBUG", False): - - def debug(msg): - # threaded output for some reason needs the carriage return or else - # it doesn't reset the cursor. - sys.stderr.write("DEBUG: " + msg.replace("\n", "\r\n") + "\r\n") - sys.stderr.flush() - -else: - - def debug(msg): - pass - - def add_arguments(parser): parser.add_argument( "--directory", @@ -52,10 +36,10 @@ def add_arguments(parser): def open_browser(name, port): try: url = f"http://localhost:{port}" - debug(f"open_browser: url={url}") + utils.debug(f"open_browser: url={url}") # wait for port to be open - debug("open_browser: waiting for port") + utils.debug("open_browser: waiting for port") start = time.time() while time.time() - start < 60.0: try: @@ -66,11 +50,11 @@ def open_browser(name, port): break if not response: - debug("open_browser: open_browser: could not get response") + utils.debug("open_browser: open_browser: could not get response") return # open a webbrowser pointing to the docker container - debug("open_browser: open_browser: opening browser window") + utils.debug("open_browser: open_browser: opening browser window") webbrowser.open(url, new=2) except Exception: @@ -83,29 +67,18 @@ def open_browser(name, port): sys.stderr.flush() -def get_free_port(): - sock = socket.socket() - sock.bind(("127.0.0.1", 0)) - port = sock.getsockname()[1] - sock.close() - return port - - def main(directory, name, port): if name is None: name = f"os-rstudio-{directory.name}" if port is None: - # this is a race condition, as something else could consume the socket - # before docker binds to it, but the chance of that on a user's - # personal machine is very small. - port = str(get_free_port()) + port = str(utils.get_free_port()) # if not no_browser: # start thread to open web browser thread = threading.Thread(target=open_browser, args=(name, port), daemon=True) thread.name = "browser thread" - debug("starting open_browser thread") + utils.debug("starting open_browser thread") thread.start() # Determine if on Linux, if so obtain user id @@ -126,7 +99,7 @@ def main(directory, name, port): f"--env=HOSTUID={uid}", ] - debug("docker: " + " ".join(docker_args)) + utils.debug("docker: " + " ".join(docker_args)) print( f"Opening an RStudio Server session at http://localhost:{port}/ when " "you are finished working please press Ctrl+C here to end the session" diff --git a/opensafely/utils.py b/opensafely/utils.py index 0b2a88a..542c0fa 100644 --- a/opensafely/utils.py +++ b/opensafely/utils.py @@ -3,12 +3,28 @@ import os import pathlib import shutil +import socket import subprocess import sys from opensafely._vendor.jobrunner import config +# poor mans debugging because debugging threads on windows is hard +if os.environ.get("DEBUG", False): + + def debug(msg): + # threaded output for some reason needs the carriage return or else + # it doesn't reset the cursor. + sys.stderr.write("DEBUG: " + msg.replace("\n", "\r\n") + "\r\n") + sys.stderr.flush() + +else: + + def debug(msg): + pass + + def get_default_user(): try: # On ~unix, in order for any files that get created to have the @@ -131,3 +147,15 @@ def run_docker( print(" ".join(docker_cmd)) return subprocess.run(docker_cmd, *args, **kwargs) + + +def get_free_port(): + """Get a port that is free on the users host machine""" + # this is a race condition, as something else could consume the socket + # before docker binds to it, but the chance of that on a user's + # personal machine is very small. + sock = socket.socket() + sock.bind(("127.0.0.1", 0)) + port = sock.getsockname()[1] + sock.close() + return port From 6a40f5c0184454847fa3732cffae5879d60be176 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Thu, 17 Oct 2024 14:16:29 +0100 Subject: [PATCH 09/12] Factor the open browser logic into utils. This vastly simplifies the implementation of jupyter and rstudio commands, and avoids repetition --- opensafely/jupyter.py | 104 ++++++++++++++++-------------------------- opensafely/rstudio.py | 50 ++------------------ opensafely/utils.py | 48 +++++++++++++++++++ 3 files changed, 93 insertions(+), 109 deletions(-) diff --git a/opensafely/jupyter.py b/opensafely/jupyter.py index 386efd6..e694c21 100644 --- a/opensafely/jupyter.py +++ b/opensafely/jupyter.py @@ -2,12 +2,8 @@ import json import os import subprocess -import sys -import threading import time -import webbrowser from pathlib import Path -from urllib import request from opensafely import utils @@ -50,59 +46,42 @@ def add_arguments(parser): ) -def open_browser(name, port): +def get_metadata(name): + """Read the login token from the generated json file in the container""" + metadata = None + metadata_path = "/tmp/.local/share/jupyter/runtime/nbserver-*.json" + + # wait for jupyter to be set up + start = time.time() + while metadata is None and time.time() - start < 120.0: + ps = subprocess.run( + ["docker", "exec", name, "bash", "-c", f"cat {metadata_path}"], + text=True, + capture_output=True, + ) + if ps.returncode == 0: + utils.debug(ps.stdout) + metadata = json.loads(ps.stdout) + else: + time.sleep(1) + + if metadata is None: + utils.debug("get_metadata: Could not get metadata") + return None + + return metadata + + +def read_metadata_and_open(name, port): try: - metadata = None - metadata_path = "/tmp/.local/share/jupyter/runtime/nbserver-*.json" - - # wait for jupyter to be set up - start = time.time() - while metadata is None and time.time() - start < 120.0: - ps = subprocess.run( - ["docker", "exec", name, "bash", "-c", f"cat {metadata_path}"], - text=True, - capture_output=True, - ) - if ps.returncode == 0: - utils.debug(ps.stdout) - metadata = json.loads(ps.stdout) - else: - time.sleep(1) - - if metadata is None: - utils.debug("get_metadata: Could not get metadata") - return - - url = f"http://localhost:{port}/?token={metadata['token']}" - utils.debug(f"open_browser: url={url}") - - # wait for port to be open - utils.debug("open_browser: waiting for port") - start = time.time() - while time.time() - start < 60.0: - try: - response = request.urlopen(url, timeout=1) - except (request.URLError, OSError): - pass - else: - break - - if not response: - utils.debug("open_browser: open_browser: could not get response") - return - - # open a webbrowser pointing to the docker container - utils.debug("open_browser: open_browser: opening browser window") - webbrowser.open(url, new=2) - - except Exception: - # reformat exception printing to work from thread - import traceback - - sys.stderr.write("Error in open browser thread:\r\n") - tb = traceback.format_exc().replace("\n", "\r\n") - sys.stderr.write(tb) - sys.stderr.flush() + metadata = get_metadata(name) + if metadata: + url = f"http://localhost:{port}/?token={metadata['token']}" + utils.open_browser(url) + else: + utils.debug("could not retrieve login token from jupyter container") + except Exception as exc: + utils.print_exception_from_thread(exc) def main(directory, name, port, no_browser, jupyter_args): @@ -130,13 +109,6 @@ def main(directory, name, port, no_browser, jupyter_args): print(f"Running following jupyter cmd in OpenSAFELY docker container {name}...") print(" ".join(jupyter_cmd)) - if not no_browser: - # start thread to open web browser - thread = threading.Thread(target=open_browser, args=(name, port), daemon=True) - thread.name = "browser thread" - utils.debug("starting open_browser thread") - thread.start() - docker_args = [ # we use our port on both sides of the docker port mapping so that # jupyter's logging uses the correct port from the user's perspective @@ -150,10 +122,14 @@ def main(directory, name, port, no_browser, jupyter_args): "PYTHONPATH=/workspace", ] + if not no_browser: + utils.open_in_thread(read_metadata_and_open, (name, port)) + utils.debug("docker: " + " ".join(docker_args)) - ps = utils.run_docker(docker_args, "python", jupyter_cmd, interactive=True) + ps = utils.run_docker( docker_args, "python", jupyter_cmd, interactive=True, directory=directory ) + # we want to exit with the same code that jupyter did return ps.returncode diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py index 2e69de2..87bdcdc 100644 --- a/opensafely/rstudio.py +++ b/opensafely/rstudio.py @@ -1,11 +1,6 @@ import os -import sys -import threading -import time -import webbrowser from pathlib import Path from sys import platform -from urllib import request from opensafely import utils @@ -33,40 +28,6 @@ def add_arguments(parser): ) -def open_browser(name, port): - try: - url = f"http://localhost:{port}" - utils.debug(f"open_browser: url={url}") - - # wait for port to be open - utils.debug("open_browser: waiting for port") - start = time.time() - while time.time() - start < 60.0: - try: - response = request.urlopen(url, timeout=1) - except (request.URLError, OSError): - pass - else: - break - - if not response: - utils.debug("open_browser: open_browser: could not get response") - return - - # open a webbrowser pointing to the docker container - utils.debug("open_browser: open_browser: opening browser window") - webbrowser.open(url, new=2) - - except Exception: - # reformat exception printing to work from thread - import traceback - - sys.stderr.write("Error in open browser thread:\r\n") - tb = traceback.format_exc().replace("\n", "\r\n") - sys.stderr.write(tb) - sys.stderr.flush() - - def main(directory, name, port): if name is None: name = f"os-rstudio-{directory.name}" @@ -74,12 +35,7 @@ def main(directory, name, port): if port is None: port = str(utils.get_free_port()) - # if not no_browser: - # start thread to open web browser - thread = threading.Thread(target=open_browser, args=(name, port), daemon=True) - thread.name = "browser thread" - utils.debug("starting open_browser thread") - thread.start() + url = f"http://localhost:{port}" # Determine if on Linux, if so obtain user id # And need to know in Windows win32 for text file line endings setting @@ -104,8 +60,12 @@ def main(directory, name, port): f"Opening an RStudio Server session at http://localhost:{port}/ when " "you are finished working please press Ctrl+C here to end the session" ) + + utils.open_in_thread(utils.open_browser, (url,)) + ps = utils.run_docker( docker_args, "rstudio", "", interactive=True, user="0:0", directory=directory ) + # we want to exit with the same code that rstudio-server did return ps.returncode diff --git a/opensafely/utils.py b/opensafely/utils.py index 542c0fa..308b6c3 100644 --- a/opensafely/utils.py +++ b/opensafely/utils.py @@ -6,6 +6,10 @@ import socket import subprocess import sys +import threading +import time +import webbrowser +from urllib import request from opensafely._vendor.jobrunner import config @@ -159,3 +163,47 @@ def get_free_port(): port = sock.getsockname()[1] sock.close() return port + + +def print_exception_from_thread(exc): + # reformat exception printing to work from thread + import traceback + + sys.stderr.write("Error in background thread:\r\n") + tb = traceback.format_exc(exc).replace("\n", "\r\n") + sys.stderr.write(tb) + sys.stderr.flush() + + +def open_browser(url): + try: + debug(f"open_browser: url={url}") + + # wait for port to be open + debug("open_browser: waiting for port") + start = time.time() + while time.time() - start < 60.0: + try: + response = request.urlopen(url, timeout=1) + except (request.URLError, OSError): + pass + else: + break + + if not response: + debug("open_browser: open_browser: could not get response") + return + + # open a webbrowser pointing to the docker container + debug("open_browser: open_browser: opening browser window") + webbrowser.open(url, new=2) + + except Exception as exc: + print_exception_from_thread(exc) + + +def open_in_thread(target, args): + thread = threading.Thread(target=target, args=args, daemon=True) + thread.name = "browser thread" + debug("starting browser thread") + thread.start() From 7fc7240198efadabb2ce412e50815fff697caaaf Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Thu, 17 Oct 2024 14:30:00 +0100 Subject: [PATCH 10/12] some language tweaks and cleanups --- opensafely/jupyter.py | 5 +---- opensafely/rstudio.py | 16 +++++++++------- opensafely/utils.py | 2 +- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/opensafely/jupyter.py b/opensafely/jupyter.py index e694c21..3121c38 100644 --- a/opensafely/jupyter.py +++ b/opensafely/jupyter.py @@ -36,7 +36,7 @@ def add_arguments(parser): "--port", "-p", default=None, - help="Port to run on", + help="Port to run on (random by default)", ) parser.add_argument( "jupyter_args", @@ -89,9 +89,6 @@ def main(directory, name, port, no_browser, jupyter_args): name = f"os-jupyter-{directory.name}" if port is None: - # this is a race condition, as something else could consume the socket - # before docker binds to it, but the chance of that on a user's - # personal machine is very small. port = str(utils.get_free_port()) jupyter_cmd = [ diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py index 87bdcdc..5e7f5ed 100644 --- a/opensafely/rstudio.py +++ b/opensafely/rstudio.py @@ -19,12 +19,11 @@ def add_arguments(parser): parser.add_argument( "--name", help="Name of docker image (defaults to use directory name)" ) - parser.add_argument( "--port", "-p", default=None, - help="Port to run on", + help="Port to run on (random by default)", ) @@ -37,8 +36,6 @@ def main(directory, name, port): url = f"http://localhost:{port}" - # Determine if on Linux, if so obtain user id - # And need to know in Windows win32 for text file line endings setting if platform == "linux": uid = os.getuid() else: @@ -57,14 +54,19 @@ def main(directory, name, port): utils.debug("docker: " + " ".join(docker_args)) print( - f"Opening an RStudio Server session at http://localhost:{port}/ when " - "you are finished working please press Ctrl+C here to end the session" + f"Opening an RStudio Server session at {url}. " + "When you are finished working please press Ctrl+C here to end the session" ) utils.open_in_thread(utils.open_browser, (url,)) ps = utils.run_docker( - docker_args, "rstudio", "", interactive=True, user="0:0", directory=directory + docker_args, + image="rstudio", + interactive=True, + # rstudio needs to start as root, but drops privileges to uid later + user="0:0", + directory=directory, ) # we want to exit with the same code that rstudio-server did diff --git a/opensafely/utils.py b/opensafely/utils.py index 308b6c3..aa28ff0 100644 --- a/opensafely/utils.py +++ b/opensafely/utils.py @@ -92,7 +92,7 @@ def git_bash_tty_wrapper(): def run_docker( docker_args, image, - cmd, + cmd=(), directory=None, interactive=False, user=None, From 9ae4bcfcbcb008cc201686770aed06eb4469a734 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Thu, 17 Oct 2024 14:48:08 +0100 Subject: [PATCH 11/12] Do not mount .gitconfig unless it exists. Or else, docker will create and empty version on the host, that will be owned by root in linux. --- opensafely/rstudio.py | 7 +++--- tests/test_rstudio.py | 57 ++++++++++++++++++++++++++++--------------- 2 files changed, 41 insertions(+), 23 deletions(-) diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py index 5e7f5ed..a2af93a 100644 --- a/opensafely/rstudio.py +++ b/opensafely/rstudio.py @@ -45,13 +45,14 @@ def main(directory, name, port): f"-p={port}:8787", f"--name={name}", f"--hostname={name}", - "--volume=" - + os.path.join(os.path.expanduser("~"), ".gitconfig") - + ":/home/rstudio/local-gitconfig", f"--env=HOSTPLATFORM={platform}", f"--env=HOSTUID={uid}", ] + gitconfig = Path.home() / ".gitconfig" + if gitconfig.exists(): + docker_args.append(f"--volume={gitconfig}:/home/rstudio/local-gitconfig") + utils.debug("docker: " + " ".join(docker_args)) print( f"Opening an RStudio Server session at {url}. " diff --git a/tests/test_rstudio.py b/tests/test_rstudio.py index d421789..6053aab 100644 --- a/tests/test_rstudio.py +++ b/tests/test_rstudio.py @@ -2,37 +2,54 @@ import pathlib from sys import platform +import pytest + from opensafely import rstudio from tests.conftest import run_main -def test_rstudio(run): +@pytest.mark.parametrize("gitconfig_exists", [True, False]) +def test_rstudio(run, tmp_path, monkeypatch, gitconfig_exists): + + home = tmp_path / "home" + home.mkdir() + # linux/macos + monkeypatch.setitem(os.environ, "HOME", str(home)) + # windows + monkeypatch.setitem(os.environ, "USERPROFILE", str(home)) + + if gitconfig_exists: + (home / ".gitconfig").touch() + if platform == "linux": uid = os.getuid() else: uid = None - run.expect( - [ - "docker", - "run", - "--rm", - "--init", - "--label=opensafely", - "--platform=linux/amd64", - "--interactive", - "--user=0:0", - f"--volume={pathlib.Path.cwd()}://workspace", - "-p=8787:8787", - "--name=test_rstudio", - "--hostname=test_rstudio", + expected = [ + "docker", + "run", + "--rm", + "--init", + "--label=opensafely", + "--platform=linux/amd64", + "--interactive", + "--user=0:0", + f"--volume={pathlib.Path.cwd()}://workspace", + "-p=8787:8787", + "--name=test_rstudio", + "--hostname=test_rstudio", + "--env=HOSTPLATFORM=" + platform, + f"--env=HOSTUID={uid}", + ] + + if gitconfig_exists: + expected.append( "--volume=" + os.path.join(os.path.expanduser("~"), ".gitconfig") + ":/home/rstudio/local-gitconfig", - "--env=HOSTPLATFORM=" + platform, - f"--env=HOSTUID={uid}", - "ghcr.io/opensafely-core/rstudio", - ] - ) + ) + + run.expect(expected + ["ghcr.io/opensafely-core/rstudio"]) assert run_main(rstudio, "--port 8787 --name test_rstudio") == 0 From a275f67882b6fb5962895a5595f2a8bf6d2d4155 Mon Sep 17 00:00:00 2001 From: Tom Palmer Date: Sun, 20 Oct 2024 14:01:42 +0100 Subject: [PATCH 12/12] Check for rstudio image - if not present pull it --- opensafely/rstudio.py | 17 +++++++++++++++++ tests/test_rstudio.py | 2 ++ 2 files changed, 19 insertions(+) diff --git a/opensafely/rstudio.py b/opensafely/rstudio.py index a2af93a..decc7ca 100644 --- a/opensafely/rstudio.py +++ b/opensafely/rstudio.py @@ -1,4 +1,5 @@ import os +import subprocess from pathlib import Path from sys import platform @@ -41,6 +42,22 @@ def main(directory, name, port): else: uid = None + # check for rstudio image, if not present pull image + imgchk = subprocess.run( + ["docker", "image", "inspect", "ghcr.io/opensafely-core/rstudio:latest"], + capture_output=True, + ) + if imgchk.returncode == 1: + subprocess.run( + [ + "docker", + "pull", + "--platform=linux/amd64", + "ghcr.io/opensafely-core/rstudio:latest", + ], + check=True, + ) + docker_args = [ f"-p={port}:8787", f"--name={name}", diff --git a/tests/test_rstudio.py b/tests/test_rstudio.py index 6053aab..58b2fbf 100644 --- a/tests/test_rstudio.py +++ b/tests/test_rstudio.py @@ -26,6 +26,8 @@ def test_rstudio(run, tmp_path, monkeypatch, gitconfig_exists): else: uid = None + run.expect(["docker", "image", "inspect", "ghcr.io/opensafely-core/rstudio:latest"]) + expected = [ "docker", "run",