Skip to content
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.

Sign up for GitHub

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

Fix Kedro Viz stays waiting even if no valid project was found #1793

Closed
wants to merge 9 commits into from
Closed

Fix Kedro Viz stays waiting even if no valid project was found #1793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f7f652e
santanas-pr
rashidakanchwala Mar 4, 2024
f55291c
fix tests
rashidakanchwala Mar 5, 2024
6294144
Merge branch 'main' of https://github.com/kedro-org/kedro-viz
rashidakanchwala Mar 5, 2024
aa09b85
fixed
rashidakanchwala Mar 5, 2024
2135271
remove unwanted PR changes
rashidakanchwala Mar 5, 2024
95f3dc5
remove unwanted PR changes
rashidakanchwala Mar 5, 2024
80f4d87
fix tests and lint
rashidakanchwala Mar 5, 2024
6a7e36b
fix tests
rashidakanchwala Mar 5, 2024
1f0af43
fix lint
rashidakanchwala Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion package/kedro_viz/launchers/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,14 @@ def run(
params,
):
"""Launch local Kedro Viz instance"""
from kedro.framework.startup import bootstrap_project

from kedro_viz.server import run_server

project_path = Path.cwd()

bootstrap_project(project_path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine but instead of showing an error stack trace, it would be nice to show a custom error message with the exception, something like -

# check if the project_path points to a valid Kedro Project
    try:
        bootstrap_project(project_path)
    except Exception as exc:
        display_cli_message(f"ERROR: Unable to start Kedro-Viz : {exc} ", "red")
        return


installed_version = parse(__version__)
latest_version = get_latest_version()
if is_running_outdated_version(installed_version, latest_version):
Expand Down Expand Up @@ -158,7 +164,6 @@ def run(
"extra_params": params,
}
if autoreload:
project_path = Path.cwd()
run_server_kwargs["project_path"] = project_path
run_process_kwargs = {
"path": project_path,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above process calls run_server -> load_and_populate_data -> load_data . In load_data we again call bootstrap_project (line 91) which is a duplicate call. Though I like the idea of having bootstrap_project inside data_loader which makes sense considering it loads metadata, we need to remove this if we go by the approach in this PR. For the dev mode we already have bootstrap_project(project_path) in server.py -> main at line 143

Expand Down
9 changes: 9 additions & 0 deletions package/tests/test_launchers/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ def test_kedro_viz_command_run_server(
runner = CliRunner()

mocker.patch("kedro_viz.launchers.cli._wait_for.__defaults__", (True, 1, True, 1))
mocker.patch("kedro.framework.startup.bootstrap_project")

with runner.isolated_filesystem():
runner.invoke(cli.viz_cli, command_options)
Expand All @@ -187,6 +188,8 @@ def test_kedro_viz_command_should_log_outdated_version(
)

mocker.patch("kedro_viz.server.run_server")
mocker.patch("kedro.framework.startup.bootstrap_project")

runner = CliRunner()
with runner.isolated_filesystem():
runner.invoke(cli.viz_cli, ["viz", "run"])
Expand Down Expand Up @@ -214,6 +217,8 @@ def test_kedro_viz_command_should_not_log_latest_version(
)

mocker.patch("kedro_viz.server.run_server")
mocker.patch("kedro.framework.startup.bootstrap_project")

runner = CliRunner()
with runner.isolated_filesystem():
runner.invoke(cli.viz_cli, ["viz", "run"])
Expand All @@ -230,6 +235,8 @@ def test_kedro_viz_command_should_not_log_if_pypi_is_down(
requests_get.side_effect = requests.exceptions.RequestException("PyPI is down")

mocker.patch("kedro_viz.server.run_server")
mocker.patch("kedro.framework.startup.bootstrap_project")

runner = CliRunner()
with runner.isolated_filesystem():
runner.invoke(cli.viz_cli, ["viz", "run"])
Expand All @@ -246,6 +253,8 @@ def test_kedro_viz_command_with_autoreload(

# 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))
mocker.patch("kedro.framework.startup.bootstrap_project")

runner = CliRunner()
with runner.isolated_filesystem():
runner.invoke(cli.viz_cli, ["viz", "run", "--autoreload"])
Expand Down