From 335be7dae8ead864b95845de05a2f9e5e55197dc Mon Sep 17 00:00:00 2001 From: Ravi Kumar Pilla Date: Mon, 11 Sep 2023 14:17:02 -0500 Subject: [PATCH] Fix for Kedro Viz Connection Error (#1507) * initial draft for resolving connection error * refactor launchers and test code * modify unit tests * fix lint errors * fix run_viz tests * update unit test for coverage * update unit tests * Refactor visualize dataset stats from DataNodeMetadata to DataNode (#1499) * add stats to data node * lint and format check fix * fix pytests * fix layout issue * fix transcoded data stats Signed-off-by: ravi-kumar-pilla * initial draft for resolving connection error Signed-off-by: ravi-kumar-pilla * Support for Python 3.11 (#1502) * initial draft for python 3.11 support * update release doc * add python warnings for e2e tests * modify e2e test * modify e2e test * test by removing lower req scenario * skip e2e tests for lower bound requirement on python 3.11 * skip e2e tests for lower bound requirement on python 3.11 * remove print statements --------- Co-authored-by: Nok Lam Chan Signed-off-by: ravi-kumar-pilla * Remove Python Upper Bound Requirements (#1506) * initial draft for python 3.11 support * update release doc * add python warnings for e2e tests * modify e2e test * modify e2e test * test by removing lower req scenario * skip e2e tests for lower bound requirement on python 3.11 * skip e2e tests for lower bound requirement on python 3.11 * remove python upperbounds initial draft * fix lint and format errors * test remove upperbound warning * test lowerbound pandas install * revert back pandas requirement * bump lower requirements for pandas * remove upper bound clean up * update release notes * fix PR comments --------- Co-authored-by: Nok Lam Chan Signed-off-by: ravi-kumar-pilla * refactor launchers and test code Signed-off-by: ravi-kumar-pilla * modify unit tests Signed-off-by: ravi-kumar-pilla * fix lint errors Signed-off-by: ravi-kumar-pilla * Fix: Adding favicon to Kedro Demo (#1509) * Fix: Adding favicon to Kedro Demo * Fix: Change in approach for serving favicon * Lint error fix * Lint error fix * Favicon endpoint test added * Favicon endpoint test added * Lint error fixed * Fix: Adding favicon to Kedro Demo Signed-off-by: Jitendra Gundaniya * Fix: Change in approach for serving favicon Signed-off-by: Jitendra Gundaniya * Lint error fix Signed-off-by: Jitendra Gundaniya * Lint error fix Signed-off-by: Jitendra Gundaniya * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya * Lint error fixed Signed-off-by: Jitendra Gundaniya * Fixed favicon endpoint test * Release doc updated * Update RELEASE.md Co-authored-by: rashidakanchwala <37628668+rashidakanchwala@users.noreply.github.com> * Removed pytest.fixture as per review comment --------- Signed-off-by: Jitendra Gundaniya Co-authored-by: rashidakanchwala <37628668+rashidakanchwala@users.noreply.github.com> Signed-off-by: ravi-kumar-pilla * fix run_viz tests Signed-off-by: ravi-kumar-pilla * update unit test for coverage Signed-off-by: ravi-kumar-pilla * Release v6.5.0 (#1513) * v6.5.0 * release * update-reminder-content * update reminder Signed-off-by: ravi-kumar-pilla * remove branch condition for automate release version check (#1514) Signed-off-by: ravi-kumar-pilla * update unit tests Signed-off-by: ravi-kumar-pilla * add release record * modify comment * fix PR comments * DCO fix * fixing dco Signed-off-by: ravi-kumar-pilla * update pytest Signed-off-by: ravi-kumar-pilla --------- Signed-off-by: ravi-kumar-pilla Signed-off-by: Jitendra Gundaniya Co-authored-by: Rashida Kanchwala Co-authored-by: Nok Lam Chan Co-authored-by: Jitendra Gundaniya <38945204+jitu5@users.noreply.github.com> Co-authored-by: rashidakanchwala <37628668+rashidakanchwala@users.noreply.github.com> --- RELEASE.md | 5 ++ package/kedro_viz/launchers/cli.py | 50 +++++++---- package/kedro_viz/launchers/jupyter.py | 63 +------------- package/kedro_viz/launchers/utils.py | 92 ++++++++++++++++++++ package/kedro_viz/server.py | 11 +-- package/tests/test_launchers/test_cli.py | 75 ++++++++++++---- package/tests/test_launchers/test_jupyter.py | 3 +- package/tests/test_launchers/test_utils.py | 48 ++++++++++ package/tests/test_server.py | 26 ------ 9 files changed, 241 insertions(+), 132 deletions(-) create mode 100644 package/kedro_viz/launchers/utils.py create mode 100644 package/tests/test_launchers/test_utils.py diff --git a/RELEASE.md b/RELEASE.md index 46cf41c54a..6af517974a 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -5,6 +5,11 @@ Please follow the established format: - Use present tense (e.g. 'Add new feature') - Include the ID number for the related PR (or PRs) in parentheses --> +# Release 6.5.1 + +## Bug fixes and other changes + +- Fix for Kedro Viz Connection Error (#1507) # Release 6.5.0 diff --git a/package/kedro_viz/launchers/cli.py b/package/kedro_viz/launchers/cli.py index a4ca7c503a..40cd2b1bf4 100644 --- a/package/kedro_viz/launchers/cli.py +++ b/package/kedro_viz/launchers/cli.py @@ -1,8 +1,9 @@ """`kedro_viz.launchers.cli` launches the viz server as a CLI app.""" +import multiprocessing import traceback -import webbrowser from pathlib import Path +from typing import Dict import click from kedro.framework.cli.project import PARAMS_ARG_HELP @@ -13,6 +14,9 @@ from kedro_viz import __version__ from kedro_viz.constants import DEFAULT_HOST, DEFAULT_PORT from kedro_viz.integrations.pypi import get_latest_version, is_running_outdated_version +from kedro_viz.launchers.utils import _check_viz_up, _start_browser, _wait_for + +_VIZ_PROCESSES: Dict[str, int] = {} @click.group(name="Kedro-Viz") @@ -83,11 +87,10 @@ def commands(): # pylint: disable=missing-function-docstring # pylint: disable=import-outside-toplevel, too-many-locals def viz(host, port, browser, load_file, save_file, pipeline, env, autoreload, params): """Visualise a Kedro pipeline using Kedro viz.""" - from kedro_viz.server import is_localhost, run_server + from kedro_viz.server import run_server installed_version = VersionInfo.parse(__version__) latest_version = get_latest_version() - if is_running_outdated_version(installed_version, latest_version): click.echo( click.style( @@ -102,6 +105,9 @@ def viz(host, port, browser, load_file, save_file, pipeline, env, autoreload, pa ) try: + if port in _VIZ_PROCESSES and _VIZ_PROCESSES[port].is_alive(): + _VIZ_PROCESSES[port].terminate() + run_server_kwargs = { "host": host, "port": port, @@ -109,29 +115,37 @@ def viz(host, port, browser, load_file, save_file, pipeline, env, autoreload, pa "save_file": save_file, "pipeline_name": pipeline, "env": env, - "browser": browser, "autoreload": autoreload, "extra_params": params, } if autoreload: - if browser and is_localhost(host): - webbrowser.open_new(f"http://{host}:{port}/") - project_path = Path.cwd() run_server_kwargs["project_path"] = project_path - # we don't want to launch a new browser tab on reload - run_server_kwargs["browser"] = False - - run_process( - path=project_path, - target=run_server, - kwargs=run_server_kwargs, - watcher_cls=RegExpWatcher, - watcher_kwargs={"re_files": r"^.*(\.yml|\.yaml|\.py|\.json)$"}, + run_process_kwargs = { + "path": project_path, + "target": run_server, + "kwargs": run_server_kwargs, + "watcher_cls": RegExpWatcher, + "watcher_kwargs": {"re_files": r"^.*(\.yml|\.yaml|\.py|\.json)$"}, + } + viz_process = multiprocessing.Process( + target=run_process, daemon=False, kwargs={**run_process_kwargs} ) - else: - run_server(**run_server_kwargs) + viz_process = multiprocessing.Process( + target=run_server, daemon=False, kwargs={**run_server_kwargs} + ) + + viz_process.start() + _VIZ_PROCESSES[port] = viz_process + + _wait_for(func=_check_viz_up, host=host, port=port) + + print("Kedro Viz Backend Server started successfully...") + + if browser: + _start_browser(host, port) + except Exception as ex: # pragma: no cover traceback.print_exc() raise KedroCliError(str(ex)) from ex diff --git a/package/kedro_viz/launchers/jupyter.py b/package/kedro_viz/launchers/jupyter.py index 821a3d9216..c63057249f 100644 --- a/package/kedro_viz/launchers/jupyter.py +++ b/package/kedro_viz/launchers/jupyter.py @@ -7,13 +7,12 @@ import os import socket from contextlib import closing -from time import sleep, time -from typing import Any, Callable, Dict +from typing import Any, Dict import IPython -import requests from IPython.display import HTML, display +from kedro_viz.launchers.utils import _check_viz_up, _wait_for from kedro_viz.server import DEFAULT_HOST, DEFAULT_PORT, run_server _VIZ_PROCESSES: Dict[str, int] = {} @@ -22,64 +21,6 @@ logger = logging.getLogger(__name__) -class WaitForException(Exception): - """WaitForException: if func doesn't return expected result within the specified time""" - - -def _wait_for( - func: Callable, - expected_result: Any = True, - timeout: int = 60, - print_error: bool = True, - sleep_for: int = 1, - **kwargs, -) -> None: - """ - Run specified function until it returns expected result until timeout. - - Args: - func (Callable): Specified function - expected_result (Any): result that is expected. Defaults to None. - timeout (int): Time out in seconds. Defaults to 10. - print_error (boolean): whether any exceptions raised should be printed. - Defaults to False. - sleep_for (int): Execute func every specified number of seconds. - Defaults to 1. - **kwargs: Arguments to be passed to func - - Raises: - WaitForException: if func doesn't return expected result within the - specified time - - """ - end = time() + timeout - - while time() <= end: - try: - retval = func(**kwargs) - except Exception as err: # pylint: disable=broad-except - if print_error: - logger.error(err) - else: - if retval == expected_result: - return None - sleep(sleep_for) - - raise WaitForException( - f"func: {func}, didn't return {expected_result} within specified timeout: {timeout}" - ) - - -def _check_viz_up(host: str, port: int): # pragma: no cover - url = f"http://{host}:{port}" - try: - response = requests.get(url, timeout=10) - except requests.ConnectionError: - return False - - return response.status_code == 200 - - def _allocate_port(host: str, start_at: int, end_at: int = 65535) -> int: acceptable_ports = range(start_at, end_at + 1) diff --git a/package/kedro_viz/launchers/utils.py b/package/kedro_viz/launchers/utils.py new file mode 100644 index 0000000000..37cb5d2839 --- /dev/null +++ b/package/kedro_viz/launchers/utils.py @@ -0,0 +1,92 @@ +"""`kedro_viz.launchers.utils` contains utility functions +used in the `kedro_viz.launchers` package.""" +import logging +import webbrowser +from time import sleep, time +from typing import Any, Callable + +import requests + +logger = logging.getLogger(__name__) + + +class WaitForException(Exception): + """WaitForException: if func doesn't return expected result within the specified time""" + + +def _wait_for( + func: Callable, + expected_result: Any = True, + timeout: int = 60, + print_error: bool = True, + sleep_for: int = 1, + **kwargs, +) -> None: + """ + Run specified function until it returns expected result until timeout. + + Args: + func (Callable): Specified function to call + expected_result (Any): result that is expected. Defaults to None. + timeout (int): Time out in seconds. Defaults to 10. + print_error (boolean): whether any exceptions raised should be printed. + Defaults to False. + sleep_for (int): Execute func every specified number of seconds. + Defaults to 1. + **kwargs: Arguments to be passed to func + + Raises: + WaitForException: if func doesn't return expected result within the + specified time + + """ + end = time() + timeout + + while time() <= end: + try: + retval = func(**kwargs) + except Exception as err: # pylint: disable=broad-except + if print_error: + logger.error(err) + else: + if retval == expected_result: + return None + sleep(sleep_for) + + raise WaitForException( + f"func: {func}, didn't return {expected_result} within specified timeout: {timeout}" + ) + + +def _check_viz_up(host: str, port: int): + """Checks if Kedro Viz Server has started and is responding to requests + + Args: + host: the host that launched the webserver + port: the port the webserver is listening + """ + + url = f"http://{host}:{port}" + try: + response = requests.get(url, timeout=10) + except requests.ConnectionError: + return False + + return response.status_code == 200 + + +def _is_localhost(host: str) -> bool: + """Check whether a host is a localhost""" + return host in ("127.0.0.1", "localhost", "0.0.0.0") + + +def _start_browser(host: str, port: int): + """Starts a new browser window only on a local interface + + Args: + host: browser url host + port: browser url port + """ + + if _is_localhost(host): + webbrowser.open_new(f"http://{host}:{port}/") diff --git a/package/kedro_viz/server.py b/package/kedro_viz/server.py index 2321cb4097..78f2bf4158 100644 --- a/package/kedro_viz/server.py +++ b/package/kedro_viz/server.py @@ -1,6 +1,5 @@ """`kedro_viz.server` provides utilities to launch a webserver for Kedro pipeline visualisation.""" -import webbrowser from pathlib import Path from typing import Any, Dict, Optional @@ -22,11 +21,6 @@ DEV_PORT = 4142 -def is_localhost(host) -> bool: - """Check whether a host is a localhost""" - return host in ("127.0.0.1", "localhost", "0.0.0.0") - - def populate_data( data_access_manager: DataAccessManager, catalog: DataCatalog, @@ -54,7 +48,6 @@ def populate_data( def run_server( host: str = DEFAULT_HOST, port: int = DEFAULT_PORT, - browser: Optional[bool] = None, load_file: Optional[str] = None, save_file: Optional[str] = None, pipeline_name: Optional[str] = None, @@ -69,7 +62,6 @@ def run_server( Args: host: the host to launch the webserver port: the port to launch the webserver - browser: whether to open the default browser automatically load_file: if a valid JSON containing API response data is provided, the API of the server is created from the JSON. save_file: if provided, the data returned by the API will be saved to a file. @@ -84,6 +76,7 @@ def run_server( take precedence over) the parameters retrieved from the project configuration. """ + print("Starting Kedro Viz Backend Server...") if load_file is None: path = Path(project_path) if project_path else Path.cwd() catalog, pipelines, session_store, stats_dict = kedro_data_loader.load_data( @@ -108,8 +101,6 @@ def run_server( else: app = apps.create_api_app_from_file(load_file) - if browser and is_localhost(host): - webbrowser.open_new(f"http://{host}:{port}/") uvicorn.run(app, host=host, port=port, log_config=None) diff --git a/package/tests/test_launchers/test_cli.py b/package/tests/test_launchers/test_cli.py index 03161f0c0b..8c9a2aa0ef 100644 --- a/package/tests/test_launchers/test_cli.py +++ b/package/tests/test_launchers/test_cli.py @@ -2,13 +2,23 @@ import requests from click.testing import CliRunner from semver import VersionInfo -from watchgod import RegExpWatcher +from watchgod import RegExpWatcher, run_process from kedro_viz import __version__ from kedro_viz.launchers import cli from kedro_viz.server import run_server +@pytest.fixture +def patched_check_viz_up(mocker): + mocker.patch("kedro_viz.launchers.cli._check_viz_up", return_value=True) + + +@pytest.fixture +def patched_start_browser(mocker): + mocker.patch("kedro_viz.launchers.cli._start_browser") + + @pytest.mark.parametrize( "command_options,run_server_args", [ @@ -17,7 +27,23 @@ { "host": "127.0.0.1", "port": 4141, - "browser": True, + "load_file": None, + "save_file": None, + "pipeline_name": None, + "env": None, + "autoreload": False, + "extra_params": {}, + }, + ), + ( + [ + "viz", + "--host", + "localhost", + ], + { + "host": "localhost", + "port": 4141, "load_file": None, "save_file": None, "pipeline_name": None, @@ -46,7 +72,6 @@ { "host": "8.8.8.8", "port": 4142, - "browser": False, "load_file": None, "save_file": "save.json", "pipeline_name": "data_science", @@ -57,13 +82,24 @@ ), ], ) -def test_kedro_viz_command_run_server(command_options, run_server_args, mocker): - run_server = mocker.patch("kedro_viz.server.run_server") +def test_kedro_viz_command_run_server( + command_options, + run_server_args, + mocker, + patched_check_viz_up, + patched_start_browser, +): + process_init = mocker.patch("multiprocessing.Process") runner = CliRunner() + # Reduce the timeout argument from 60 to 1 to make test run faster. + mocker.patch("kedro_viz.launchers.cli._wait_for.__defaults__", (True, 1, True, 1)) with runner.isolated_filesystem(): runner.invoke(cli.commands, command_options) - run_server.assert_called_once_with(**run_server_args) + process_init.assert_called_once_with( + target=run_server, daemon=False, kwargs={**run_server_args} + ) + assert run_server_args["port"] in cli._VIZ_PROCESSES def test_kedro_viz_command_should_log_outdated_version(mocker, mock_http_response): @@ -118,19 +154,22 @@ def test_kedro_viz_command_should_not_log_if_pypi_is_down(mocker, mock_http_resp mock_click_echo.assert_not_called() -def test_kedro_viz_command_with_autoreload(mocker): - mocker.patch("webbrowser.open_new") +def test_kedro_viz_command_with_autoreload( + mocker, patched_check_viz_up, patched_start_browser +): + process_init = mocker.patch("multiprocessing.Process") mock_project_path = "/tmp/project_path" mocker.patch("pathlib.Path.cwd", return_value=mock_project_path) - run_process = mocker.patch("kedro_viz.launchers.cli.run_process") + # Reduce the timeout argument from 60 to 1 to make test run faster. + mocker.patch("kedro_viz.launchers.cli._wait_for.__defaults__", (True, 1, True, 1)) runner = CliRunner() with runner.isolated_filesystem(): runner.invoke(cli.commands, ["viz", "--autoreload"]) - run_process.assert_called_once_with( - path=mock_project_path, - target=run_server, - kwargs={ + run_process_kwargs = { + "path": mock_project_path, + "target": run_server, + "kwargs": { "host": "127.0.0.1", "port": 4141, "load_file": None, @@ -138,10 +177,14 @@ def test_kedro_viz_command_with_autoreload(mocker): "pipeline_name": None, "env": None, "autoreload": True, - "browser": False, "project_path": mock_project_path, "extra_params": {}, }, - watcher_cls=RegExpWatcher, - watcher_kwargs={"re_files": "^.*(\\.yml|\\.yaml|\\.py|\\.json)$"}, + "watcher_cls": RegExpWatcher, + "watcher_kwargs": {"re_files": "^.*(\\.yml|\\.yaml|\\.py|\\.json)$"}, + } + + process_init.assert_called_once_with( + target=run_process, daemon=False, kwargs={**run_process_kwargs} ) + assert run_process_kwargs["kwargs"]["port"] in cli._VIZ_PROCESSES diff --git a/package/tests/test_launchers/test_jupyter.py b/package/tests/test_launchers/test_jupyter.py index cfc984e814..9890d13360 100644 --- a/package/tests/test_launchers/test_jupyter.py +++ b/package/tests/test_launchers/test_jupyter.py @@ -1,6 +1,7 @@ import pytest -from kedro_viz.launchers.jupyter import _VIZ_PROCESSES, WaitForException, run_viz +from kedro_viz.launchers.jupyter import _VIZ_PROCESSES, run_viz +from kedro_viz.launchers.utils import WaitForException from kedro_viz.server import run_server diff --git a/package/tests/test_launchers/test_utils.py b/package/tests/test_launchers/test_utils.py new file mode 100644 index 0000000000..32c29e4982 --- /dev/null +++ b/package/tests/test_launchers/test_utils.py @@ -0,0 +1,48 @@ +from unittest import mock +from unittest.mock import Mock + +import pytest +import requests + +from kedro_viz.launchers.utils import _check_viz_up, _start_browser + + +@pytest.mark.parametrize( + "ip,should_browser_open", + [ + ("0.0.0.0", True), + ("127.0.0.1", True), + ("localhost", True), + ("8.8.8.8", False), + ], +) +@mock.patch("kedro_viz.launchers.utils.webbrowser") +def test_browser_open( + webbrowser, + ip, + should_browser_open, + mocker, +): + _start_browser(ip, port=4141) + if should_browser_open: + webbrowser.open_new.assert_called_once() + else: + webbrowser.open_new.assert_not_called() + + +@pytest.mark.parametrize( + "host, port, status_code, expected_result", + [ + ("localhost", 8080, 200, True), # Successful response + ("localhost", 8080, 500, False), # Non-200 response + ("localhost", 8080, None, False), # Connection error + ], +) +def test_check_viz_up(host, port, status_code, expected_result, mocker): + if status_code is not None: + mocker.patch("requests.get", return_value=Mock(status_code=status_code)) + else: + mocker.patch("requests.get", side_effect=requests.ConnectionError()) + + result = _check_viz_up(host, port) + assert result == expected_result diff --git a/package/tests/test_server.py b/package/tests/test_server.py index c9fcdead88..21cd9159ff 100644 --- a/package/tests/test_server.py +++ b/package/tests/test_server.py @@ -1,5 +1,4 @@ import json -from unittest import mock import pytest from pydantic import BaseModel @@ -133,28 +132,3 @@ def test_save_file(self, tmp_path, mocker): run_server(save_file=save_file) with open(save_file, "r", encoding="utf8") as f: assert json.load(f) == {"content": "test"} - - @pytest.mark.parametrize( - "browser,ip,should_browser_open", - [ - (True, "0.0.0.0", True), - (True, "127.0.0.1", True), - (True, "localhost", True), - (False, "127.0.0.1", False), - (True, "8.8.8.8", False), - ], - ) - @mock.patch("kedro_viz.server.webbrowser") - def test_browser_open( - self, - webbrowser, - browser, - ip, - should_browser_open, - mocker, - ): - run_server(browser=browser, host=ip) - if should_browser_open: - webbrowser.open_new.assert_called_once() - else: - webbrowser.open_new.assert_not_called()