From 14ff3ae0901a93bd45cc2da545a2447d544e84f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Dock=C3=A8s?= Date: Wed, 3 Jan 2024 11:00:28 +0100 Subject: [PATCH] [FIX] use a local server rather than temp file to open plots in a browser (#4180) * use a local server rather than temp file to open plots in a browser * add deprecation warning & whatsnew * update tests * add tests + use a DeprecationWarning * Update doc/changes/latest.rst --------- Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com> --- doc/changes/latest.rst | 1 + nilearn/plotting/html_document.py | 103 ++++++++++-------- nilearn/plotting/rm_file.py | 24 ---- nilearn/plotting/tests/test_html_document.py | 86 ++++++++------- .../plotting/tests/test_js_plotting_utils.py | 28 ----- 5 files changed, 108 insertions(+), 134 deletions(-) delete mode 100644 nilearn/plotting/rm_file.py diff --git a/doc/changes/latest.rst b/doc/changes/latest.rst index a3f014e4d0..77abe5c0a0 100644 --- a/doc/changes/latest.rst +++ b/doc/changes/latest.rst @@ -10,6 +10,7 @@ HIGHLIGHTS NEW --- +- :bdg-info:`Plotting` The ``temp_file_lifetime`` parameter of interactive plots' ``open_in_browser`` method is deprecated and has no effect (:gh:`4180` by `Jerome Dockes`_).. Fixes ----- diff --git a/nilearn/plotting/html_document.py b/nilearn/plotting/html_document.py index 0fd57b5241..e555adf1b3 100644 --- a/nilearn/plotting/html_document.py +++ b/nilearn/plotting/html_document.py @@ -1,15 +1,17 @@ """Handle HTML plotting.""" -import os -import subprocess -import sys -import tempfile import warnings import weakref import webbrowser from html import escape +from http import HTTPStatus +from http.server import BaseHTTPRequestHandler +from queue import Empty, Queue +from socketserver import TCPServer +from threading import Thread MAX_IMG_VIEWS_BEFORE_WARNING = 10 +BROWSER_TIMEOUT_SECONDS = 3.0 def set_max_img_views_before_warning(new_value): @@ -21,12 +23,45 @@ def set_max_img_views_before_warning(new_value): MAX_IMG_VIEWS_BEFORE_WARNING = new_value -def _remove_after_n_seconds(file_name, n_seconds): - script = os.path.join(os.path.dirname(__file__), "rm_file.py") - proc = subprocess.Popen( - [sys.executable, script, file_name, str(n_seconds)] - ) - return proc +def _open_in_browser(content): + """Open a page in the user's web browser. + + This function starts a local server in a separate thread, opens the page + with webbrowser, and shuts down the server once it has served one request. + """ + queue = Queue() + + class Handler(BaseHTTPRequestHandler): + def log_message(self, *args): + del args + + def do_GET(self): + if not self.path.endswith("index.html"): + self.send_error(HTTPStatus.NOT_FOUND, "File not found") + return + self.send_response(HTTPStatus.OK) + self.send_header("Content-type", "text/html") + self.send_header("Content-Length", str(len(content))) + self.end_headers() + self.wfile.write(content) + queue.put("done") + + server = TCPServer(("", 0), Handler) + _, port = server.server_address + + server_thread = Thread(target=server.serve_forever, daemon=True) + server_thread.start() + + url = f"http://localhost:{port}/index.html" + webbrowser.open(url) + try: + queue.get(timeout=BROWSER_TIMEOUT_SECONDS) + except Empty: + raise RuntimeError( + "Failed to open nilearn plot or report in a web browser." + ) + server.shutdown() + server_thread.join() class HTMLDocument: @@ -146,7 +181,7 @@ def save_as_html(self, file_name): with open(file_name, "wb") as f: f.write(self.get_standalone().encode("utf-8")) - def open_in_browser(self, file_name=None, temp_file_lifetime=30): + def open_in_browser(self, file_name=None, temp_file_lifetime="deprecated"): """Save the plot to a temporary HTML file and open it in a browser. Parameters @@ -155,41 +190,21 @@ def open_in_browser(self, file_name=None, temp_file_lifetime=30): HTML file to use as a temporary file. temp_file_lifetime : :obj:`float`, default=30 - Time, in seconds, after which the temporary file is removed. - If None, it is never removed. + .. deprecated:: 0.11 + The parameter is kept for backward compatibility and will be + removed in a future version. It has no effect. """ + if temp_file_lifetime != "deprecated": + warnings.warn( + "temp_file_lifetime is deprecated. It has no " + "effect and passing a value for this parameter " + "will result in an error starting with nilearn version 0.13", + DeprecationWarning, + ) if file_name is None: - fd, file_name = tempfile.mkstemp(".html", "nilearn_plot_") - os.close(fd) - named_file = False - else: - named_file = True - self.save_as_html(file_name) - self._temp_file = file_name - file_size = os.path.getsize(file_name) / 1e6 - if temp_file_lifetime is None: - if not named_file: - warnings.warn( - f"Saved HTML in temporary file: {file_name}\n" - f"file size is {file_size:.1f}M, " - "delete it when you're done, " - "for example by calling this.remove_temp_file" - ) + _open_in_browser(self.get_standalone().encode("utf-8")) else: - self._temp_file_removing_proc = _remove_after_n_seconds( - self._temp_file, temp_file_lifetime - ) - webbrowser.open(f"file://{file_name}") - - def remove_temp_file(self): - """Remove the temporary file created by \ - ``open_in_browser``, if necessary.""" - if self._temp_file is None: - return - if not os.path.isfile(self._temp_file): - return - os.remove(self._temp_file) - print(f"removed {self._temp_file}") - self._temp_file = None + self.save_as_html(file_name) + webbrowser.open(f"file://{file_name}") diff --git a/nilearn/plotting/rm_file.py b/nilearn/plotting/rm_file.py deleted file mode 100644 index 9331fe8a40..0000000000 --- a/nilearn/plotting/rm_file.py +++ /dev/null @@ -1,24 +0,0 @@ -"""Remove a file after a certain time. - -This is run in a subprocess -by nilearn.plotting.html_surface.SurfaceView to remove the temporary -file it uses to open a plot in a web browser. - -""" -import argparse -import os -import time -import warnings - -if __name__ == "__main__": - parser = argparse.ArgumentParser() - parser.add_argument("file_name", type=str) - parser.add_argument("n_seconds", type=float) - args = parser.parse_args() - - time.sleep(args.n_seconds) - if os.path.isfile(args.file_name): - try: - os.remove(args.file_name) - except Exception as e: - warnings.warn(f"failed to remove {args.file_name}:\n{e}") diff --git a/nilearn/plotting/tests/test_html_document.py b/nilearn/plotting/tests/test_html_document.py index 7fa3ef9acf..b616445b65 100644 --- a/nilearn/plotting/tests/test_html_document.py +++ b/nilearn/plotting/tests/test_html_document.py @@ -1,10 +1,9 @@ -import os -import tempfile import time -import warnings import webbrowser +from unittest.mock import Mock import pytest +import requests from numpy.testing import assert_no_warnings from nilearn.plotting import html_document @@ -14,41 +13,52 @@ # warnings -def _open_mock(f): - print(f"opened {f}") - - -def test_temp_file_removing(): - html = html_document.HTMLDocument("hello") - wb_open = webbrowser.open - webbrowser.open = _open_mock - fd, tmpfile = tempfile.mkstemp() - try: - os.close(fd) - with warnings.catch_warnings(record=True) as record: - html.open_in_browser(file_name=tmpfile, temp_file_lifetime=None) - for warning in record: - assert "Saved HTML in temporary file" not in str(warning.message) - html.open_in_browser(temp_file_lifetime=0.5) - assert os.path.isfile(html._temp_file) - html._temp_file_removing_proc.wait() - assert not os.path.isfile(html._temp_file) - with pytest.warns(UserWarning, match="Saved HTML in temporary file"): - html.open_in_browser(temp_file_lifetime=None) - html.open_in_browser(temp_file_lifetime=None) - assert os.path.isfile(html._temp_file) - time.sleep(1.5) - assert os.path.isfile(html._temp_file) - finally: - webbrowser.open = wb_open - try: - os.remove(html._temp_file) - except Exception: - pass - try: - os.remove(tmpfile) - except Exception: - pass +class Get: + def __init__(self, delay=0.0): + self.delay = delay + + def __call__(self, url): + time.sleep(self.delay) + self.url = url + requests.get(url.replace("index.html", "favicon.ico")) + self.content = requests.get(url).content + + +# disable request mocking for this test -- note we are accessing localhost only +@pytest.mark.parametrize("request_mocker", [None]) +def test_open_in_browser(monkeypatch): + opener = Get() + monkeypatch.setattr(webbrowser, "open", opener) + doc = html_document.HTMLDocument("hello") + doc.open_in_browser() + assert opener.content == b"hello" + + +def test_open_in_browser_timeout(monkeypatch): + opener = Get(delay=1.0) + monkeypatch.setattr(webbrowser, "open", opener) + monkeypatch.setattr(html_document, "BROWSER_TIMEOUT_SECONDS", 0.01) + doc = html_document.HTMLDocument("hello") + with pytest.raises(RuntimeError, match="Failed to open"): + doc.open_in_browser() + + +@pytest.mark.parametrize("request_mocker", [None]) +def test_open_in_browser_deprecation_warning(monkeypatch): + monkeypatch.setattr(webbrowser, "open", Get()) + doc = html_document.HTMLDocument("hello") + with pytest.warns(DeprecationWarning, match="temp_file_lifetime"): + doc.open_in_browser(temp_file_lifetime=30.0) + + +def test_open_in_browser_file(tmp_path, monkeypatch): + opener = Mock() + monkeypatch.setattr(webbrowser, "open", opener) + file_path = tmp_path / "doc.html" + doc = html_document.HTMLDocument("hello") + doc.open_in_browser(file_name=str(file_path)) + assert file_path.read_text("utf-8") == "hello" + opener.assert_called_once_with(f"file://{file_path}") def _open_views(): diff --git a/nilearn/plotting/tests/test_js_plotting_utils.py b/nilearn/plotting/tests/test_js_plotting_utils.py index 9cfd3022d3..15847a6324 100644 --- a/nilearn/plotting/tests/test_js_plotting_utils.py +++ b/nilearn/plotting/tests/test_js_plotting_utils.py @@ -2,7 +2,6 @@ import os import re import tempfile -import webbrowser import numpy as np import pytest @@ -208,7 +207,6 @@ def check_html( assert html._repr_html_() == html.get_iframe() assert str(html) == html.get_standalone() assert '' in str(html) - _check_open_in_browser(html) resized = html.resize(3, 17) assert resized is html assert (html.width, html.height) == (3, 17) @@ -241,32 +239,6 @@ def check_html( assert len(view.findall("option")) == 7 -def _open_mock(f): - print(f"opened {f}") - - -def _check_open_in_browser(html): - wb_open = webbrowser.open - webbrowser.open = _open_mock - try: - html.open_in_browser(temp_file_lifetime=None) - temp_file = html._temp_file - assert html._temp_file is not None - assert os.path.isfile(temp_file) - html.remove_temp_file() - assert html._temp_file is None - assert not os.path.isfile(temp_file) - html.remove_temp_file() - html._temp_file = "aaaaaaaaaaaaaaaaaaaaaa" - html.remove_temp_file() - finally: - webbrowser.open = wb_open - try: - os.remove(temp_file) - except Exception: - pass - - @pytest.mark.parametrize( "colors", [