From 38c9d0a360fa13aa1b530944016839e45ad8ed0e Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sun, 19 Jan 2025 18:38:14 +0100 Subject: [PATCH 01/57] feat: Expose open as an alternative to quickstart to should spin-off server --- skore/src/skore/cli/cli.py | 103 ++++++----- skore/src/skore/cli/color_format.py | 2 +- skore/src/skore/cli/launch_dashboard.py | 57 ------ skore/src/skore/cli/quickstart_command.py | 56 ------ skore/src/skore/exceptions.py | 4 + skore/src/skore/project/__init__.py | 2 +- .../skore/project/{create.py => _manage.py} | 169 +++++++++++++++++- skore/src/skore/project/{open.py => _open.py} | 31 ++-- skore/src/skore/project/load.py | 51 ------ skore/src/skore/project/project.py | 2 + skore/src/skore/ui/app.py | 6 +- skore/src/skore/ui/project_routes.py | 2 +- skore/tests/conftest.py | 1 + skore/tests/unit/cli/test_cli.py | 2 +- skore/tests/unit/project/test_load.py | 2 +- skore/tests/unit/project/test_project.py | 2 +- 16 files changed, 253 insertions(+), 239 deletions(-) delete mode 100644 skore/src/skore/cli/launch_dashboard.py delete mode 100644 skore/src/skore/cli/quickstart_command.py rename skore/src/skore/project/{create.py => _manage.py} (51%) rename skore/src/skore/project/{open.py => _open.py} (58%) delete mode 100644 skore/src/skore/project/load.py diff --git a/skore/src/skore/cli/cli.py b/skore/src/skore/cli/cli.py index 0a7631bf8..0a35578fb 100644 --- a/skore/src/skore/cli/cli.py +++ b/skore/src/skore/cli/cli.py @@ -4,9 +4,8 @@ from importlib.metadata import version from skore.cli.color_format import ColorArgumentParser -from skore.cli.launch_dashboard import __launch -from skore.cli.quickstart_command import __quickstart -from skore.project.create import _create +from skore.project import open +from skore.project._manage import _create, _launch, _load def cli(args: list[str]): @@ -19,6 +18,26 @@ def cli(args: list[str]): subparsers = parser.add_subparsers(dest="subcommand") + # create a skore project + parser_create = subparsers.add_parser("create", help="Create a project") + parser_create.add_argument( + "project_name", + nargs="?", + help="the name or path of the project to create (default: %(default)s)", + default="project", + ) + parser_create.add_argument( + "--overwrite", + action="store_true", + help="overwrite an existing project with the same name", + ) + parser_create.add_argument( + "--verbose", + action="store_true", + help="increase logging verbosity", + ) + + # launch the skore UI parser_launch = subparsers.add_parser("launch", help="Launch the web UI") parser_launch.add_argument( "project_name", @@ -28,7 +47,7 @@ def cli(args: list[str]): "--port", type=int, help="the port at which to bind the UI server (default: %(default)s)", - default=22140, + default=None, ) parser_launch.add_argument( "--open-browser", @@ -45,54 +64,39 @@ def cli(args: list[str]): help="increase logging verbosity", ) - parser_create = subparsers.add_parser("create", help="Create a project") - parser_create.add_argument( + # open a skore project + parser_open = subparsers.add_parser( + "open", help='Create a "project.skore" file and start the UI' + ) + parser_open.add_argument( "project_name", nargs="?", help="the name or path of the project to create (default: %(default)s)", default="project", ) - parser_create.add_argument( - "--overwrite", - action="store_true", - help="overwrite an existing project with the same name", - ) - parser_create.add_argument( - "--verbose", + parser_open.add_argument( + "--create", action="store_true", - help="increase logging verbosity", + help="create a new project if it does not exist", ) - - parser_quickstart = subparsers.add_parser( - "quickstart", help='Create a "project.skore" file and start the UI' - ) - parser_quickstart.add_argument( - "project_name", - nargs="?", - help="the name or path of the project to create (default: %(default)s)", - default="project", - ) - parser_quickstart.add_argument( + parser_open.add_argument( "--overwrite", action="store_true", help="overwrite an existing project with the same name", ) - parser_quickstart.add_argument( + parser_open.add_argument( + "--serve", + action=argparse.BooleanOptionalAction, + help=("whether to serve the project (default: %(default)s)"), + default=True, + ) + parser_open.add_argument( "--port", type=int, help="the port at which to bind the UI server (default: %(default)s)", - default=22140, + default=None, ) - parser_quickstart.add_argument( - "--open-browser", - action=argparse.BooleanOptionalAction, - help=( - "whether to automatically open a browser tab showing the web UI " - "(default: %(default)s)" - ), - default=True, - ) - parser_quickstart.add_argument( + parser_open.add_argument( "--verbose", action="store_true", help="increase logging verbosity", @@ -100,26 +104,27 @@ def cli(args: list[str]): parsed_args: argparse.Namespace = parser.parse_args(args) - if parsed_args.subcommand == "launch": - __launch( - project_name=parsed_args.project_name, - port=parsed_args.port, - open_browser=parsed_args.open_browser, - verbose=parsed_args.verbose, - ) - elif parsed_args.subcommand == "create": + if parsed_args.subcommand == "create": _create( project_name=parsed_args.project_name, overwrite=parsed_args.overwrite, verbose=parsed_args.verbose, ) - elif parsed_args.subcommand == "quickstart": - __quickstart( - project_name=parsed_args.project_name, - overwrite=parsed_args.overwrite, + elif parsed_args.subcommand == "launch": + _launch( + project=_load(project_name=parsed_args.project_name), port=parsed_args.port, open_browser=parsed_args.open_browser, verbose=parsed_args.verbose, ) + elif parsed_args.subcommand == "open": + open( + project_path=parsed_args.project_name, + create=parsed_args.create, + overwrite=parsed_args.overwrite, + serve=parsed_args.serve, + port=parsed_args.port, + verbose=parsed_args.verbose, + ) else: parser.print_help() diff --git a/skore/src/skore/cli/color_format.py b/skore/src/skore/cli/color_format.py index db510d649..6a3e3a960 100644 --- a/skore/src/skore/cli/color_format.py +++ b/skore/src/skore/cli/color_format.py @@ -79,7 +79,7 @@ def format_help(self): # Color the subcommands in cyan help_text = re.sub( - r"(?<=\s)(launch|create|quickstart)(?=\s+)", + r"(?<=\s)(launch|create|open)(?=\s+)", r"[cyan bold]\1[/cyan bold]", help_text, ) diff --git a/skore/src/skore/cli/launch_dashboard.py b/skore/src/skore/cli/launch_dashboard.py deleted file mode 100644 index 368838c81..000000000 --- a/skore/src/skore/cli/launch_dashboard.py +++ /dev/null @@ -1,57 +0,0 @@ -"""Implement the "launch" command.""" - -import webbrowser -from contextlib import asynccontextmanager -from pathlib import Path -from typing import Union - -import uvicorn -from fastapi import FastAPI - -from skore.cli import logger -from skore.project import open -from skore.ui.app import create_app -from skore.utils._logger import logger_context - - -def __launch( - project_name: Union[str, Path], - port: int, - open_browser: bool, - verbose: bool = False, -): - """Launch the UI to visualize a project. - - Parameters - ---------- - project_name : Path-like - Name of the project to be created, or a relative or absolute path. - port : int - Port at which to bind the UI server. - open_browser: bool - Whether to automatically open a browser tab showing the UI. - verbose: bool - Whether to display info logs to the user. - """ - from skore import console # avoid circular import - - with logger_context(logger, verbose): - project = open(project_name) - - @asynccontextmanager - async def lifespan(app: FastAPI): - if open_browser: - webbrowser.open(f"http://localhost:{port}") - yield - - app = create_app(project=project, lifespan=lifespan) - - try: - # TODO: check port is free - console.rule("[bold cyan]skore-UI[/bold cyan]") - console.print( - f"Running skore UI from '{project_name}' at URL http://localhost:{port}" - ) - uvicorn.run(app, port=port, log_level="error") - except KeyboardInterrupt: - console.print("Closing skore UI") diff --git a/skore/src/skore/cli/quickstart_command.py b/skore/src/skore/cli/quickstart_command.py deleted file mode 100644 index e2aa0c394..000000000 --- a/skore/src/skore/cli/quickstart_command.py +++ /dev/null @@ -1,56 +0,0 @@ -"""Implement the "quickstart" command.""" - -from pathlib import Path -from typing import Union - -from skore.cli import logger -from skore.cli.launch_dashboard import __launch -from skore.project.create import _create -from skore.utils._logger import logger_context - - -def __quickstart( - project_name: Union[str, Path], - overwrite: bool, - port: int, - open_browser: bool, - verbose: bool = False, -): - """Quickstart a Skore project. - - Create it if it does not exist, then launch the web UI. - - Parameters - ---------- - project_name : Path-like - Name of the project to be created, or a relative or absolute path. - overwrite : bool - If ``True``, overwrite an existing project with the same name. - If ``False``, simply warn that a project already exists. - port : int - Port at which to bind the UI server. - open_browser : bool - Whether to automatically open a browser tab showing the UI. - verbose : bool - Whether to increase logging verbosity. - """ - with logger_context(logger, verbose): - try: - _create( - project_name=project_name, - overwrite=overwrite, - verbose=verbose, - ) - except FileExistsError: - logger.info( - f"Project file '{project_name}' already exists. Skipping creation step." - ) - - path = Path(project_name) - - __launch( - project_name=path, - port=port, - open_browser=open_browser, - verbose=verbose, - ) diff --git a/skore/src/skore/exceptions.py b/skore/src/skore/exceptions.py index 8df4456f1..138d6508c 100644 --- a/skore/src/skore/exceptions.py +++ b/skore/src/skore/exceptions.py @@ -18,3 +18,7 @@ class ProjectCreationError(Exception): class ProjectPermissionError(Exception): """Permissions in the directory do not allow creating a file.""" + + +class ProjectLoadError(Exception): + """Failed to load project.""" diff --git a/skore/src/skore/project/__init__.py b/skore/src/skore/project/__init__.py index be1978581..e02110f5b 100644 --- a/skore/src/skore/project/__init__.py +++ b/skore/src/skore/project/__init__.py @@ -1,6 +1,6 @@ """Alias top level function and class of the project submodule.""" -from .open import open +from ._open import open from .project import Project __all__ = [ diff --git a/skore/src/skore/project/create.py b/skore/src/skore/project/_manage.py similarity index 51% rename from skore/src/skore/project/create.py rename to skore/src/skore/project/_manage.py index 43fab5672..c92836119 100644 --- a/skore/src/skore/project/create.py +++ b/skore/src/skore/project/_manage.py @@ -1,19 +1,80 @@ -"""Create project helper.""" +"""Load project helper.""" +import asyncio import re import shutil +import socket +import webbrowser +from concurrent.futures import ThreadPoolExecutor +from contextlib import asynccontextmanager from pathlib import Path from typing import Optional, Union +import uvicorn +from fastapi import FastAPI +from rich.console import Console + from skore.exceptions import ( InvalidProjectNameError, ProjectCreationError, + ProjectLoadError, ProjectPermissionError, ) -from skore.project.load import _load +from skore.item import ItemRepository +from skore.persistence.disk_cache_storage import DirectoryDoesNotExist, DiskCacheStorage from skore.project.project import Project, logger from skore.utils._logger import logger_context from skore.view.view import View +from skore.view.view_repository import ViewRepository + + +def _load(project_name: Union[str, Path]) -> Project: + """Load an existing Project given a project name or path. + + Transforms a project name to a directory path as follows: + - Resolves relative path to current working directory, + - Checks that the file ends with the ".skore" extension, + - If not provided, it will be automatically appended, + - If project name is an absolute path, keeps that path. + + Parameters + ---------- + project_name : Path-like + Name of the project to be created, or a relative or absolute path. + + Returns + ------- + Project + The loaded Project instance. + """ + path = Path(project_name).resolve() + + if path.suffix != ".skore": + path = path.parent / (path.name + ".skore") + + if not Path(path).exists(): + raise FileNotFoundError(f"Project '{path}' does not exist: did you create it?") + + try: + # FIXME: Should those hardcoded strings be factorized somewhere ? + item_storage = DiskCacheStorage(directory=Path(path) / "items") + item_repository = ItemRepository(storage=item_storage) + view_storage = DiskCacheStorage(directory=Path(path) / "views") + view_repository = ViewRepository(storage=view_storage) + project = Project( + name=path.name, + item_repository=item_repository, + view_repository=view_repository, + ) + except DirectoryDoesNotExist as e: + missing_directory = e.args[0].split()[1] + raise ProjectLoadError( + f"Project '{path}' is corrupted: " + f"directory '{missing_directory}' should exist. " + "Consider re-creating the project." + ) from e + + return project def _validate_project_name(project_name: str) -> tuple[bool, Optional[Exception]]: @@ -69,15 +130,16 @@ def _create( project_name : Path-like Name of the project to be created, or a relative or absolute path. If relative, will be interpreted as relative to the current working directory. - overwrite : bool + overwrite : bool, default=False If ``True``, overwrite an existing project with the same name. If ``False``, raise an error if a project with the same name already exists. - verbose : bool + verbose : bool, default=False Whether or not to display info logs to the user. Returns ------- - The created project + Project + The created project """ from skore import console # avoid circular import @@ -148,3 +210,100 @@ def _create( console.rule("[bold cyan]skore[/bold cyan]") console.print(f"Project file '{project_directory}' was successfully created.") return p + + +def find_free_port() -> int: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.bind(("", 0)) + s.listen(1) + return s.getsockname()[1] + + +class ServerManager: + _instance = None + _executor = None + _port = None + + @classmethod + def get_instance(cls): + if cls._instance is None: + cls._instance = cls() + return cls._instance + + def start_server( + self, + project: Project, + port: int | None = None, + open_browser: bool = True, + ): + from skore import console + + console.rule("[bold cyan]skore-UI[/bold cyan]") + if self._executor is not None: + console.print(f"Server is already running at http://localhost:{self._port}") + return + + def run_in_thread(): + loop = asyncio.new_event_loop() + asyncio.set_event_loop(loop) + try: + self._port = port + loop.run_until_complete( + run_server(project, port, open_browser, console) + ) + except KeyboardInterrupt: + console.print("Closing skore UI") + finally: + loop.close() + + self._executor = ThreadPoolExecutor(max_workers=1) + self._executor.submit(run_in_thread) + self._executor.shutdown(wait=False) + + +async def run_server(project: Project, port: int, open_browser: bool, console: Console): + @asynccontextmanager + async def lifespan(app: FastAPI): + if open_browser: # This was previously hardcoded in _launch + webbrowser.open(f"http://localhost:{port}") + yield + + from skore.ui.app import create_app + + app = create_app(project=project, lifespan=lifespan) + server_config = uvicorn.Config(app, port=port, log_level="error") + server = uvicorn.Server(server_config) + + console.print( + f"Running skore UI from '{project.name}' at URL http://localhost:{port}" + ) + + await server.serve() + + +def _launch( + project: Project, + port: int | None = None, + open_browser: bool = True, + verbose: bool = False, +): + """Launch the UI to visualize a project. + + Parameters + ---------- + project : Project + The project to be launched. + port : int, default=None + Port at which to bind the UI server. If ``None``, the server will be bound to + an available port. + open_browser: bool, default=True + Whether to automatically open a browser tab showing the UI. + verbose: bool, default=False + Whether to display info logs to the user. + """ + if port is None: + port = find_free_port() + + with logger_context(logger, verbose): + server_manager = ServerManager.get_instance() + server_manager.start_server(project, port, open_browser) diff --git a/skore/src/skore/project/open.py b/skore/src/skore/project/_open.py similarity index 58% rename from skore/src/skore/project/open.py rename to skore/src/skore/project/_open.py index ad658b5e8..4dca700df 100644 --- a/skore/src/skore/project/open.py +++ b/skore/src/skore/project/_open.py @@ -1,10 +1,7 @@ -"""Command to open a Project.""" - from pathlib import Path from typing import Union -from skore.project.create import _create -from skore.project.load import _load +from skore.project._manage import _create, _launch, _load from skore.project.project import Project @@ -13,8 +10,11 @@ def open( *, create: bool = True, overwrite: bool = False, + serve: bool = True, + port: int | None = None, + verbose: bool = False, ) -> Project: - """Open a project given a project name or path. + """Open a project given a project name or path and launch skore UI. This function : - opens the project if it already exists, @@ -31,6 +31,13 @@ def open( overwrite: bool, default=False Overwrite the project file if it already exists and ``create`` is ``True``. Has no effect otherwise. + serve: bool, default=True + Whether to launch the skore UI. + port: int | None, default=None + Port at which to bind the UI server. If ``None``, the server will be bound to + an available port. + verbose : bool, default=False + Whether or not to display info logs to the user. Returns ------- @@ -46,11 +53,15 @@ def open( """ if create and not overwrite: try: - return _load(project_path) + project = _load(project_path) except FileNotFoundError: - return _create(project_path, overwrite=overwrite) + project = _create(project_path, overwrite=overwrite, verbose=verbose) + elif not create: + project = _load(project_path) + else: + project = _create(project_path, overwrite=overwrite, verbose=verbose) - if not create: - return _load(project_path) + if serve: + _launch(project, port=port, verbose=verbose) - return _create(project_path, overwrite=overwrite) + return project diff --git a/skore/src/skore/project/load.py b/skore/src/skore/project/load.py deleted file mode 100644 index 80a9bc0dd..000000000 --- a/skore/src/skore/project/load.py +++ /dev/null @@ -1,51 +0,0 @@ -"""Load project helper.""" - -from pathlib import Path -from typing import Union - -from skore.item import ItemRepository -from skore.persistence.disk_cache_storage import DirectoryDoesNotExist, DiskCacheStorage -from skore.project.project import Project -from skore.view.view_repository import ViewRepository - - -class ProjectLoadError(Exception): - """Failed to load project.""" - - -def _load(project_name: Union[str, Path]) -> Project: - """Load an existing Project given a project name or path. - - Transforms a project name to a directory path as follows: - - Resolves relative path to current working directory, - - Checks that the file ends with the ".skore" extension, - - If not provided, it will be automatically appended, - - If project name is an absolute path, keeps that path. - """ - path = Path(project_name).resolve() - - if path.suffix != ".skore": - path = path.parent / (path.name + ".skore") - - if not Path(path).exists(): - raise FileNotFoundError(f"Project '{path}' does not exist: did you create it?") - - try: - # FIXME: Should those hardcoded strings be factorized somewhere ? - item_storage = DiskCacheStorage(directory=Path(path) / "items") - item_repository = ItemRepository(storage=item_storage) - view_storage = DiskCacheStorage(directory=Path(path) / "views") - view_repository = ViewRepository(storage=view_storage) - project = Project( - item_repository=item_repository, - view_repository=view_repository, - ) - except DirectoryDoesNotExist as e: - missing_directory = e.args[0].split()[1] - raise ProjectLoadError( - f"Project '{path}' is corrupted: " - f"directory '{missing_directory}' should exist. " - "Consider re-creating the project." - ) from e - - return project diff --git a/skore/src/skore/project/project.py b/skore/src/skore/project/project.py index df65bb279..c7edcee47 100644 --- a/skore/src/skore/project/project.py +++ b/skore/src/skore/project/project.py @@ -71,9 +71,11 @@ class A: def __init__( self, + name: str, item_repository: ItemRepository, view_repository: ViewRepository, ): + self.name = name self.item_repository = item_repository self.view_repository = view_repository diff --git a/skore/src/skore/ui/app.py b/skore/src/skore/ui/app.py index df7ef64c3..699b59923 100644 --- a/skore/src/skore/ui/app.py +++ b/skore/src/skore/ui/app.py @@ -10,7 +10,7 @@ from fastapi.staticfiles import StaticFiles from starlette.types import Lifespan -from skore.project import Project, open +from skore.project import Project from skore.ui.dependencies import get_static_path from skore.ui.project_routes import router as project_router @@ -21,10 +21,6 @@ def create_app( """FastAPI factory used to create the API to interact with `stores`.""" app = FastAPI(lifespan=lifespan) - # Give the app access to the project - if not project: - project = open("project.skore") - app.state.project = project # Enable CORS support on all routes, for all origins and methods. diff --git a/skore/src/skore/ui/project_routes.py b/skore/src/skore/ui/project_routes.py index 94de0bbae..736e95aa7 100644 --- a/skore/src/skore/ui/project_routes.py +++ b/skore/src/skore/ui/project_routes.py @@ -110,7 +110,7 @@ async def get_activity( The activity is composed of all the items and their versions created after the datetime `after`, sorted from newest to oldest. """ - project = request.app.state.project + project: Project = request.app.state.project return sorted( ( __item_as_serializable(key, version) diff --git a/skore/tests/conftest.py b/skore/tests/conftest.py index 3260b03bb..bcbd9b1ea 100644 --- a/skore/tests/conftest.py +++ b/skore/tests/conftest.py @@ -42,6 +42,7 @@ def in_memory_project(): item_repository = ItemRepository(storage=InMemoryStorage()) view_repository = ViewRepository(storage=InMemoryStorage()) return Project( + name="in_memory_project.skore", item_repository=item_repository, view_repository=view_repository, ) diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index de72e4f01..11f32fb84 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -21,7 +21,7 @@ def fake_launch(project_name, port, open_browser, verbose): launch_open_browser = open_browser launch_verbose = verbose - monkeypatch.setattr("skore.cli.cli.__launch", fake_launch) + monkeypatch.setattr("skore.cli.cli._launch", fake_launch) cli(["launch", "project.skore", "--port", "0", "--no-open-browser", "--verbose"]) assert launch_project_name == "project.skore" diff --git a/skore/tests/unit/project/test_load.py b/skore/tests/unit/project/test_load.py index 06dd4b602..4ec290888 100644 --- a/skore/tests/unit/project/test_load.py +++ b/skore/tests/unit/project/test_load.py @@ -2,7 +2,7 @@ import pytest from skore.project import Project -from skore.project.load import _load +from skore.project._manage import _load @pytest.fixture diff --git a/skore/tests/unit/project/test_project.py b/skore/tests/unit/project/test_project.py index 866db9cf9..33862da3e 100644 --- a/skore/tests/unit/project/test_project.py +++ b/skore/tests/unit/project/test_project.py @@ -15,7 +15,7 @@ InvalidProjectNameError, ProjectCreationError, ) -from skore.project.create import _create, _validate_project_name +from skore.project._manage import _create, _validate_project_name from skore.view.view import View From 62ed5379a1b3e91ddaab75f356a692ae4cc4971b Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sun, 19 Jan 2025 19:46:55 +0100 Subject: [PATCH 02/57] iter --- skore/src/skore/cli/cli.py | 5 +- skore/src/skore/project/_manage.py | 4 + skore/src/skore/project/_open.py | 17 ++-- skore/tests/conftest.py | 2 +- .../tests/integration/cli/test_quickstart.py | 25 ----- skore/tests/unit/cli/test_cli.py | 63 +++++++++---- skore/tests/unit/cli/test_open.py | 93 +++++++++++++++++++ skore/tests/unit/cli/test_quickstart.py | 59 ------------ skore/tests/unit/project/test_open.py | 22 ++--- 9 files changed, 164 insertions(+), 126 deletions(-) delete mode 100644 skore/tests/integration/cli/test_quickstart.py create mode 100644 skore/tests/unit/cli/test_open.py delete mode 100644 skore/tests/unit/cli/test_quickstart.py diff --git a/skore/src/skore/cli/cli.py b/skore/src/skore/cli/cli.py index 0a35578fb..04061e1bd 100644 --- a/skore/src/skore/cli/cli.py +++ b/skore/src/skore/cli/cli.py @@ -76,8 +76,9 @@ def cli(args: list[str]): ) parser_open.add_argument( "--create", - action="store_true", - help="create a new project if it does not exist", + action=argparse.BooleanOptionalAction, + help=("create a new project if it does not exist " "(default: %(default)s)"), + default=True, ) parser_open.add_argument( "--overwrite", diff --git a/skore/src/skore/project/_manage.py b/skore/src/skore/project/_manage.py index c92836119..06bd22c7b 100644 --- a/skore/src/skore/project/_manage.py +++ b/skore/src/skore/project/_manage.py @@ -255,6 +255,10 @@ def run_in_thread(): console.print("Closing skore UI") finally: loop.close() + console.print( + f"Server that was running at http://localhost:{self._port} has " + "been closed" + ) self._executor = ThreadPoolExecutor(max_workers=1) self._executor.submit(run_in_thread) diff --git a/skore/src/skore/project/_open.py b/skore/src/skore/project/_open.py index 4dca700df..82d613ada 100644 --- a/skore/src/skore/project/_open.py +++ b/skore/src/skore/project/_open.py @@ -51,15 +51,16 @@ def open( ProjectCreationError If project creation fails for some reason (e.g. if the project path is invalid) """ - if create and not overwrite: - try: - project = _load(project_path) - except FileNotFoundError: - project = _create(project_path, overwrite=overwrite, verbose=verbose) - elif not create: + try: project = _load(project_path) - else: - project = _create(project_path, overwrite=overwrite, verbose=verbose) + if overwrite: + # let _create handle the overwrite flag + project = _create(project_path, overwrite=overwrite, verbose=verbose) + except FileNotFoundError: + if create: + project = _create(project_path, overwrite=overwrite, verbose=verbose) + else: + raise if serve: _launch(project, port=port, verbose=verbose) diff --git a/skore/tests/conftest.py b/skore/tests/conftest.py index bcbd9b1ea..f5e230eb3 100644 --- a/skore/tests/conftest.py +++ b/skore/tests/conftest.py @@ -50,7 +50,7 @@ def in_memory_project(): @pytest.fixture def on_disk_project(tmp_path): - project = skore.open(tmp_path / "project") + project = skore.open(tmp_path / "project", serve=False) return project diff --git a/skore/tests/integration/cli/test_quickstart.py b/skore/tests/integration/cli/test_quickstart.py deleted file mode 100644 index ee2eb26a3..000000000 --- a/skore/tests/integration/cli/test_quickstart.py +++ /dev/null @@ -1,25 +0,0 @@ -import os - -import pytest -from skore.cli.cli import cli - - -@pytest.fixture -def fake_launch(monkeypatch): - def _fake_launch(project_name, port, open_browser, verbose): - pass - - monkeypatch.setattr("skore.cli.quickstart_command.__launch", _fake_launch) - - -def test_quickstart(tmp_path, fake_launch): - os.chdir(tmp_path) - cli("quickstart".split()) - assert (tmp_path / "project.skore").exists() - - # calling the same command without overwriting should succeed - # (as the creation step is skipped) - cli("quickstart".split()) - - # calling the same command with overwriting should succeed - cli("quickstart --overwrite".split()) diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 11f32fb84..b064bbb0a 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -1,33 +1,56 @@ """Test CLI properly calls the app.""" +import os + import pytest from skore.cli.cli import cli -def test_cli_launch(monkeypatch): - launch_project_name = None - launch_port = None - launch_open_browser = None - launch_verbose = None +@pytest.fixture +def tmp_project_path(tmp_path): + """Create a project at `tmp_path` and return its absolute path.""" + # Project path must end with ".skore" + project_path = tmp_path.parent / (tmp_path.name + ".skore") + os.mkdir(project_path) + os.mkdir(project_path / "items") + os.mkdir(project_path / "views") + return project_path + + +@pytest.fixture +def mock_server_manager(monkeypatch): + """Mock ServerManager to verify server startup parameters.""" + + class MockServerManager: + def __init__(self): + self.start_params = None + + def get_instance(self): + return self + + def start_server(self, project, port=None, open_browser=True): + self.start_params = {"port": port, "open_browser": open_browser} - def fake_launch(project_name, port, open_browser, verbose): - nonlocal launch_project_name - nonlocal launch_port - nonlocal launch_open_browser - nonlocal launch_verbose + mock_manager = MockServerManager() + monkeypatch.setattr("skore.project._manage.ServerManager", mock_manager) + return mock_manager - launch_project_name = project_name - launch_port = port - launch_open_browser = open_browser - launch_verbose = verbose - monkeypatch.setattr("skore.cli.cli._launch", fake_launch) - cli(["launch", "project.skore", "--port", "0", "--no-open-browser", "--verbose"]) +def test_cli_launch(tmp_project_path, mock_server_manager): + """Test that CLI launch starts server with correct parameters.""" + cli( + [ + "launch", + str(tmp_project_path), + "--port", + "8000", + "--no-open-browser", + "--verbose", + ] + ) - assert launch_project_name == "project.skore" - assert launch_port == 0 - assert not launch_open_browser - assert launch_verbose + assert mock_server_manager.start_params["port"] == 8000 + assert mock_server_manager.start_params["open_browser"] is False def test_cli_launch_no_project_name(): diff --git a/skore/tests/unit/cli/test_open.py b/skore/tests/unit/cli/test_open.py new file mode 100644 index 000000000..abed58125 --- /dev/null +++ b/skore/tests/unit/cli/test_open.py @@ -0,0 +1,93 @@ +import os + +import pytest +from skore.cli.cli import cli + + +@pytest.fixture +def tmp_project_path(tmp_path): + """Create a project at `tmp_path` and return its absolute path.""" + # Project path must end with ".skore" + project_path = tmp_path.parent / (tmp_path.name + ".skore") + os.mkdir(project_path) + os.mkdir(project_path / "items") + os.mkdir(project_path / "views") + return project_path + + +@pytest.fixture +def mock_server_manager(monkeypatch): + """Mock ServerManager to verify server startup parameters.""" + + class MockServerManager: + def __init__(self): + self.start_params = None + + def get_instance(self): + return self + + def start_server(self, project, port=None, open_browser=True): + # Always set open_browser to False to prevent browser opening + self.start_params = {"port": port, "open_browser": False} + + mock_manager = MockServerManager() + monkeypatch.setattr("skore.project._manage.ServerManager", mock_manager) + return mock_manager + + +def test_cli_open(tmp_project_path, mock_server_manager): + """Test that CLI open creates a project and starts server with correct + parameters.""" + cli( + [ + "open", + str(tmp_project_path), + "--overwrite", + "--port", + "8000", + "--serve", + "--verbose", + ] + ) + + assert mock_server_manager.start_params["port"] == 8000 + assert mock_server_manager.start_params["open_browser"] is False + + +def test_cli_open_creates_project(tmp_path, mock_server_manager): + """Test that CLI open creates a project when it doesn't exist.""" + project_path = tmp_path / "new_project.skore" + assert not project_path.exists() + + cli(["open", str(project_path), "--create"]) + assert project_path.exists() + + +def test_cli_open_no_create_fails(tmp_path, mock_server_manager): + """Test that CLI open fails when project doesn't exist and create=False.""" + project_path = tmp_path / "nonexistent.skore" + + with pytest.raises(FileNotFoundError): + cli(["open", str(project_path), "--no-create"]) + + +def test_cli_open_overwrite(tmp_path, mock_server_manager): + """Test that CLI open can overwrite existing project.""" + project_path = tmp_path / "overwrite_test.skore" + + cli(["open", str(project_path), "--create"]) + initial_time = os.path.getmtime(project_path) + + cli(["open", str(project_path), "--create", "--overwrite"]) + new_time = os.path.getmtime(project_path) + assert new_time > initial_time + + +def test_cli_open_no_serve(tmp_path, mock_server_manager): + """Test that server is not started when --no-serve flag is passed.""" + project_path = tmp_path / "no_serve.skore" + + cli(["open", str(project_path), "--create", "--no-serve"]) + + # Verify server was not started + assert mock_server_manager.start_params is None diff --git a/skore/tests/unit/cli/test_quickstart.py b/skore/tests/unit/cli/test_quickstart.py deleted file mode 100644 index 40c65f0cb..000000000 --- a/skore/tests/unit/cli/test_quickstart.py +++ /dev/null @@ -1,59 +0,0 @@ -from skore.cli.cli import cli - - -def test_quickstart(monkeypatch, tmp_path): - """`quickstart` passes its arguments down to `create` and `launch`.""" - - create_project_name = None - create_overwrite = None - create_verbose = None - - def fake_create(project_name, overwrite, verbose): - nonlocal create_project_name - nonlocal create_overwrite - nonlocal create_verbose - - create_project_name = project_name - create_overwrite = overwrite - create_verbose = verbose - - monkeypatch.setattr("skore.cli.quickstart_command._create", fake_create) - - launch_project_name = None - launch_port = None - launch_open_browser = None - launch_verbose = None - - def fake_launch(project_name, port, open_browser, verbose): - nonlocal launch_project_name - nonlocal launch_port - nonlocal launch_open_browser - nonlocal launch_verbose - - launch_project_name = project_name - launch_port = port - launch_open_browser = open_browser - launch_verbose = verbose - - monkeypatch.setattr("skore.cli.quickstart_command.__launch", fake_launch) - - cli( - [ - "quickstart", - str(tmp_path / "my_project.skore"), - "--verbose", - "--overwrite", - "--port", - "888", - "--no-open-browser", - ] - ) - - assert create_project_name == str(tmp_path / "my_project.skore") - assert create_overwrite is True - assert create_verbose is True - - assert launch_project_name == tmp_path / "my_project.skore" - assert launch_port == 888 - assert launch_open_browser is False - assert launch_verbose is True diff --git a/skore/tests/unit/project/test_open.py b/skore/tests/unit/project/test_open.py index 9f5bc0d71..446ffb431 100644 --- a/skore/tests/unit/project/test_open.py +++ b/skore/tests/unit/project/test_open.py @@ -33,21 +33,21 @@ def project_changed(project_path, modified=True): def test_open_relative_path(tmp_project_path): """If passed a relative path, `open` operates in the current working directory.""" os.chdir(tmp_project_path.parent) - p = open(tmp_project_path.name, create=False) + p = open(tmp_project_path.name, create=False, serve=False) assert isinstance(p, Project) def test_open_default(tmp_project_path): """If a project already exists, `open` loads it.""" with project_changed(tmp_project_path, modified=False): - p = open(tmp_project_path) + p = open(tmp_project_path, serve=False) assert isinstance(p, Project) def test_open_default_no_project(tmp_path): """If no project exists, `open` creates it.""" with project_changed(tmp_path, modified=False): - p = open(tmp_path) + p = open(tmp_path, serve=False) assert isinstance(p, Project) @@ -55,39 +55,39 @@ def test_open_project_exists_create_true_overwrite_true(tmp_project_path): """If the project exists, and `create` and `overwrite` are set to `True`, `open` overwrites it with a new one.""" with project_changed(tmp_project_path): - open(tmp_project_path, create=True, overwrite=True) + open(tmp_project_path, create=True, overwrite=True, serve=False) def test_open_project_exists_create_true_overwrite_false(tmp_project_path): with project_changed(tmp_project_path, modified=False): - open(tmp_project_path, create=True, overwrite=False) + open(tmp_project_path, create=True, overwrite=False, serve=False) def test_open_project_exists_create_false_overwrite_true(tmp_project_path): - p = open(tmp_project_path, create=False, overwrite=True) + p = open(tmp_project_path, create=False, overwrite=True, serve=False) assert isinstance(p, Project) def test_open_project_exists_create_false_overwrite_false(tmp_project_path): - p = open(tmp_project_path, create=False, overwrite=False) + p = open(tmp_project_path, create=False, overwrite=False, serve=False) assert isinstance(p, Project) def test_open_no_project_create_true_overwrite_true(tmp_path): - p = open(tmp_path, create=True, overwrite=True) + p = open(tmp_path, create=True, overwrite=True, serve=False) assert isinstance(p, Project) def test_open_no_project_create_true_overwrite_false(tmp_path): - p = open(tmp_path, create=True, overwrite=False) + p = open(tmp_path, create=True, overwrite=False, serve=False) assert isinstance(p, Project) def test_open_no_project_create_false_overwrite_true(tmp_path): with pytest.raises(FileNotFoundError): - open(tmp_path, create=False, overwrite=True) + open(tmp_path, create=False, overwrite=True, serve=False) def test_open_no_project_create_false_overwrite_false(tmp_path): with pytest.raises(FileNotFoundError): - open(tmp_path, create=False, overwrite=False) + open(tmp_path, create=False, overwrite=False, serve=False) From adb1f6b440aeb8a8c5193567784db2a6990b717e Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sun, 19 Jan 2025 19:48:15 +0100 Subject: [PATCH 03/57] iter --- skore/src/skore/project/_manage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skore/src/skore/project/_manage.py b/skore/src/skore/project/_manage.py index 06bd22c7b..0525d52ed 100644 --- a/skore/src/skore/project/_manage.py +++ b/skore/src/skore/project/_manage.py @@ -1,4 +1,4 @@ -"""Load project helper.""" +"""Helpers to create, load, and launch projects.""" import asyncio import re From a435720bbfdac3b0ea2280dc123eb9dfcf7b15a8 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sun, 19 Jan 2025 20:12:13 +0100 Subject: [PATCH 04/57] iter --- skore/src/skore/cli/cli.py | 10 +- .../skore/project/{_manage.py => _create.py} | 170 +----------------- skore/src/skore/project/_launch.py | 115 ++++++++++++ skore/src/skore/project/_load.py | 59 ++++++ skore/src/skore/project/_open.py | 13 +- skore/tests/unit/cli/test_cli.py | 2 +- skore/tests/unit/cli/test_open.py | 2 +- skore/tests/unit/project/test_load.py | 8 +- skore/tests/unit/project/test_project.py | 14 +- 9 files changed, 205 insertions(+), 188 deletions(-) rename skore/src/skore/project/{_manage.py => _create.py} (50%) create mode 100644 skore/src/skore/project/_launch.py create mode 100644 skore/src/skore/project/_load.py diff --git a/skore/src/skore/cli/cli.py b/skore/src/skore/cli/cli.py index 04061e1bd..cda64b58c 100644 --- a/skore/src/skore/cli/cli.py +++ b/skore/src/skore/cli/cli.py @@ -5,7 +5,9 @@ from skore.cli.color_format import ColorArgumentParser from skore.project import open -from skore.project._manage import _create, _launch, _load +from skore.project._create import create +from skore.project._launch import launch +from skore.project._load import load def cli(args: list[str]): @@ -106,14 +108,14 @@ def cli(args: list[str]): parsed_args: argparse.Namespace = parser.parse_args(args) if parsed_args.subcommand == "create": - _create( + create( project_name=parsed_args.project_name, overwrite=parsed_args.overwrite, verbose=parsed_args.verbose, ) elif parsed_args.subcommand == "launch": - _launch( - project=_load(project_name=parsed_args.project_name), + launch( + project=load(project_name=parsed_args.project_name), port=parsed_args.port, open_browser=parsed_args.open_browser, verbose=parsed_args.verbose, diff --git a/skore/src/skore/project/_manage.py b/skore/src/skore/project/_create.py similarity index 50% rename from skore/src/skore/project/_manage.py rename to skore/src/skore/project/_create.py index 0525d52ed..d67692ecf 100644 --- a/skore/src/skore/project/_manage.py +++ b/skore/src/skore/project/_create.py @@ -1,80 +1,19 @@ -"""Helpers to create, load, and launch projects.""" +"""Helper to create a project.""" -import asyncio import re import shutil -import socket -import webbrowser -from concurrent.futures import ThreadPoolExecutor -from contextlib import asynccontextmanager from pathlib import Path from typing import Optional, Union -import uvicorn -from fastapi import FastAPI -from rich.console import Console - from skore.exceptions import ( InvalidProjectNameError, ProjectCreationError, - ProjectLoadError, ProjectPermissionError, ) -from skore.item import ItemRepository -from skore.persistence.disk_cache_storage import DirectoryDoesNotExist, DiskCacheStorage +from skore.project._load import load from skore.project.project import Project, logger from skore.utils._logger import logger_context from skore.view.view import View -from skore.view.view_repository import ViewRepository - - -def _load(project_name: Union[str, Path]) -> Project: - """Load an existing Project given a project name or path. - - Transforms a project name to a directory path as follows: - - Resolves relative path to current working directory, - - Checks that the file ends with the ".skore" extension, - - If not provided, it will be automatically appended, - - If project name is an absolute path, keeps that path. - - Parameters - ---------- - project_name : Path-like - Name of the project to be created, or a relative or absolute path. - - Returns - ------- - Project - The loaded Project instance. - """ - path = Path(project_name).resolve() - - if path.suffix != ".skore": - path = path.parent / (path.name + ".skore") - - if not Path(path).exists(): - raise FileNotFoundError(f"Project '{path}' does not exist: did you create it?") - - try: - # FIXME: Should those hardcoded strings be factorized somewhere ? - item_storage = DiskCacheStorage(directory=Path(path) / "items") - item_repository = ItemRepository(storage=item_storage) - view_storage = DiskCacheStorage(directory=Path(path) / "views") - view_repository = ViewRepository(storage=view_storage) - project = Project( - name=path.name, - item_repository=item_repository, - view_repository=view_repository, - ) - except DirectoryDoesNotExist as e: - missing_directory = e.args[0].split()[1] - raise ProjectLoadError( - f"Project '{path}' is corrupted: " - f"directory '{missing_directory}' should exist. " - "Consider re-creating the project." - ) from e - - return project def _validate_project_name(project_name: str) -> tuple[bool, Optional[Exception]]: @@ -118,7 +57,7 @@ def _validate_project_name(project_name: str) -> tuple[bool, Optional[Exception] return True, None -def _create( +def create( project_name: Union[str, Path], overwrite: bool = False, verbose: bool = False, @@ -204,110 +143,9 @@ def _create( f"Unable to create project file '{views_dir}'." ) from e - p = _load(project_directory) + p = load(project_directory) p.put_view("default", View(layout=[])) console.rule("[bold cyan]skore[/bold cyan]") console.print(f"Project file '{project_directory}' was successfully created.") return p - - -def find_free_port() -> int: - with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - s.bind(("", 0)) - s.listen(1) - return s.getsockname()[1] - - -class ServerManager: - _instance = None - _executor = None - _port = None - - @classmethod - def get_instance(cls): - if cls._instance is None: - cls._instance = cls() - return cls._instance - - def start_server( - self, - project: Project, - port: int | None = None, - open_browser: bool = True, - ): - from skore import console - - console.rule("[bold cyan]skore-UI[/bold cyan]") - if self._executor is not None: - console.print(f"Server is already running at http://localhost:{self._port}") - return - - def run_in_thread(): - loop = asyncio.new_event_loop() - asyncio.set_event_loop(loop) - try: - self._port = port - loop.run_until_complete( - run_server(project, port, open_browser, console) - ) - except KeyboardInterrupt: - console.print("Closing skore UI") - finally: - loop.close() - console.print( - f"Server that was running at http://localhost:{self._port} has " - "been closed" - ) - - self._executor = ThreadPoolExecutor(max_workers=1) - self._executor.submit(run_in_thread) - self._executor.shutdown(wait=False) - - -async def run_server(project: Project, port: int, open_browser: bool, console: Console): - @asynccontextmanager - async def lifespan(app: FastAPI): - if open_browser: # This was previously hardcoded in _launch - webbrowser.open(f"http://localhost:{port}") - yield - - from skore.ui.app import create_app - - app = create_app(project=project, lifespan=lifespan) - server_config = uvicorn.Config(app, port=port, log_level="error") - server = uvicorn.Server(server_config) - - console.print( - f"Running skore UI from '{project.name}' at URL http://localhost:{port}" - ) - - await server.serve() - - -def _launch( - project: Project, - port: int | None = None, - open_browser: bool = True, - verbose: bool = False, -): - """Launch the UI to visualize a project. - - Parameters - ---------- - project : Project - The project to be launched. - port : int, default=None - Port at which to bind the UI server. If ``None``, the server will be bound to - an available port. - open_browser: bool, default=True - Whether to automatically open a browser tab showing the UI. - verbose: bool, default=False - Whether to display info logs to the user. - """ - if port is None: - port = find_free_port() - - with logger_context(logger, verbose): - server_manager = ServerManager.get_instance() - server_manager.start_server(project, port, open_browser) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py new file mode 100644 index 000000000..0de2826da --- /dev/null +++ b/skore/src/skore/project/_launch.py @@ -0,0 +1,115 @@ +"""Helpers to create, load, and launch projects.""" + +import asyncio +import socket +import webbrowser +from concurrent.futures import ThreadPoolExecutor +from contextlib import asynccontextmanager + +import uvicorn +from fastapi import FastAPI +from rich.console import Console + +from skore.project.project import Project, logger +from skore.utils._logger import logger_context + + +def find_free_port() -> int: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.bind(("", 0)) + s.listen(1) + return s.getsockname()[1] + + +class ServerManager: + _instance = None + _executor = None + _port = None + + @classmethod + def get_instance(cls): + if cls._instance is None: + cls._instance = cls() + return cls._instance + + def start_server( + self, + project: Project, + port: int | None = None, + open_browser: bool = True, + ): + from skore import console + + console.rule("[bold cyan]skore-UI[/bold cyan]") + if self._executor is not None: + console.print(f"Server is already running at http://localhost:{self._port}") + return + + def run_in_thread(): + loop = asyncio.new_event_loop() + asyncio.set_event_loop(loop) + try: + self._port = port + loop.run_until_complete( + run_server(project, port, open_browser, console) + ) + except KeyboardInterrupt: + console.print("Closing skore UI") + finally: + loop.close() + console.print( + f"Server that was running at http://localhost:{self._port} has " + "been closed" + ) + + self._executor = ThreadPoolExecutor(max_workers=1) + self._executor.submit(run_in_thread) + self._executor.shutdown(wait=False) + + +async def run_server(project: Project, port: int, open_browser: bool, console: Console): + @asynccontextmanager + async def lifespan(app: FastAPI): + if open_browser: # This was previously hardcoded in _launch + webbrowser.open(f"http://localhost:{port}") + yield + + from skore.ui.app import create_app + + app = create_app(project=project, lifespan=lifespan) + server_config = uvicorn.Config(app, port=port, log_level="error") + server = uvicorn.Server(server_config) + + console.print( + f"Running skore UI from '{project.name}' at URL http://localhost:{port}" + ) + + await server.serve() + + +def launch( + project: Project, + port: int | None = None, + open_browser: bool = True, + verbose: bool = False, +): + """Launch the UI to visualize a project. + + Parameters + ---------- + project : Project + The project to be launched. + port : int, default=None + Port at which to bind the UI server. If ``None``, the server will be bound to + an available port. + open_browser: bool, default=True + Whether to automatically open a browser tab showing the UI. + verbose: bool, default=False + Whether to display info logs to the user. + """ + if port is None: + port = find_free_port() + + with logger_context(logger, verbose): + server_manager = ServerManager.get_instance() + server_manager.start_server(project, port, open_browser) diff --git a/skore/src/skore/project/_load.py b/skore/src/skore/project/_load.py new file mode 100644 index 000000000..fd8a947a6 --- /dev/null +++ b/skore/src/skore/project/_load.py @@ -0,0 +1,59 @@ +"""Helper to load a project.""" + +from pathlib import Path +from typing import Union + +from skore.exceptions import ProjectLoadError +from skore.item import ItemRepository +from skore.persistence.disk_cache_storage import DirectoryDoesNotExist, DiskCacheStorage +from skore.project.project import Project +from skore.view.view_repository import ViewRepository + + +def load(project_name: Union[str, Path]) -> Project: + """Load an existing Project given a project name or path. + + Transforms a project name to a directory path as follows: + - Resolves relative path to current working directory, + - Checks that the file ends with the ".skore" extension, + - If not provided, it will be automatically appended, + - If project name is an absolute path, keeps that path. + + Parameters + ---------- + project_name : Path-like + Name of the project to be created, or a relative or absolute path. + + Returns + ------- + Project + The loaded Project instance. + """ + path = Path(project_name).resolve() + + if path.suffix != ".skore": + path = path.parent / (path.name + ".skore") + + if not Path(path).exists(): + raise FileNotFoundError(f"Project '{path}' does not exist: did you create it?") + + try: + # FIXME: Should those hardcoded strings be factorized somewhere ? + item_storage = DiskCacheStorage(directory=Path(path) / "items") + item_repository = ItemRepository(storage=item_storage) + view_storage = DiskCacheStorage(directory=Path(path) / "views") + view_repository = ViewRepository(storage=view_storage) + project = Project( + name=path.name, + item_repository=item_repository, + view_repository=view_repository, + ) + except DirectoryDoesNotExist as e: + missing_directory = e.args[0].split()[1] + raise ProjectLoadError( + f"Project '{path}' is corrupted: " + f"directory '{missing_directory}' should exist. " + "Consider re-creating the project." + ) from e + + return project diff --git a/skore/src/skore/project/_open.py b/skore/src/skore/project/_open.py index 82d613ada..af9ecb04f 100644 --- a/skore/src/skore/project/_open.py +++ b/skore/src/skore/project/_open.py @@ -1,7 +1,6 @@ from pathlib import Path from typing import Union -from skore.project._manage import _create, _launch, _load from skore.project.project import Project @@ -51,18 +50,22 @@ def open( ProjectCreationError If project creation fails for some reason (e.g. if the project path is invalid) """ + from skore.project._create import create + from skore.project._launch import launch + from skore.project._load import load + try: - project = _load(project_path) + project = load(project_path) if overwrite: # let _create handle the overwrite flag - project = _create(project_path, overwrite=overwrite, verbose=verbose) + project = create(project_path, overwrite=overwrite, verbose=verbose) except FileNotFoundError: if create: - project = _create(project_path, overwrite=overwrite, verbose=verbose) + project = create(project_path, overwrite=overwrite, verbose=verbose) else: raise if serve: - _launch(project, port=port, verbose=verbose) + launch(project, port=port, verbose=verbose) return project diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index b064bbb0a..467f50637 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -32,7 +32,7 @@ def start_server(self, project, port=None, open_browser=True): self.start_params = {"port": port, "open_browser": open_browser} mock_manager = MockServerManager() - monkeypatch.setattr("skore.project._manage.ServerManager", mock_manager) + monkeypatch.setattr("skore.project._launch.ServerManager", mock_manager) return mock_manager diff --git a/skore/tests/unit/cli/test_open.py b/skore/tests/unit/cli/test_open.py index abed58125..9cb1781a3 100644 --- a/skore/tests/unit/cli/test_open.py +++ b/skore/tests/unit/cli/test_open.py @@ -31,7 +31,7 @@ def start_server(self, project, port=None, open_browser=True): self.start_params = {"port": port, "open_browser": False} mock_manager = MockServerManager() - monkeypatch.setattr("skore.project._manage.ServerManager", mock_manager) + monkeypatch.setattr("skore.project._launch.ServerManager", mock_manager) return mock_manager diff --git a/skore/tests/unit/project/test_load.py b/skore/tests/unit/project/test_load.py index 4ec290888..a199f8d91 100644 --- a/skore/tests/unit/project/test_load.py +++ b/skore/tests/unit/project/test_load.py @@ -2,7 +2,7 @@ import pytest from skore.project import Project -from skore.project._manage import _load +from skore.project._load import load @pytest.fixture @@ -18,15 +18,15 @@ def tmp_project_path(tmp_path): def test_load_no_project(): with pytest.raises(FileNotFoundError): - _load("/empty") + load("/empty") def test_load_absolute_path(tmp_project_path): - p = _load(tmp_project_path) + p = load(tmp_project_path) assert isinstance(p, Project) def test_load_relative_path(tmp_project_path): os.chdir(tmp_project_path.parent) - p = _load(tmp_project_path.name) + p = load(tmp_project_path.name) assert isinstance(p, Project) diff --git a/skore/tests/unit/project/test_project.py b/skore/tests/unit/project/test_project.py index 33862da3e..3266dfee0 100644 --- a/skore/tests/unit/project/test_project.py +++ b/skore/tests/unit/project/test_project.py @@ -15,7 +15,7 @@ InvalidProjectNameError, ProjectCreationError, ) -from skore.project._manage import _create, _validate_project_name +from skore.project._create import _validate_project_name, create from skore.view.view import View @@ -266,30 +266,30 @@ def test_validate_project_name(project_name, expected): @pytest.mark.parametrize("project_name", ["hello", "hello.skore"]) def test_create_project(project_name, tmp_path): - _create(tmp_path / project_name) + create(tmp_path / project_name) assert (tmp_path / "hello.skore").exists() # TODO: If using fixtures in test cases is possible, join this with # `test_create_project` def test_create_project_absolute_path(tmp_path): - _create(tmp_path / "hello") + create(tmp_path / "hello") assert (tmp_path / "hello.skore").exists() def test_create_project_fails_if_file_exists(tmp_path): - _create(tmp_path / "hello") + create(tmp_path / "hello") assert (tmp_path / "hello.skore").exists() with pytest.raises(FileExistsError): - _create(tmp_path / "hello") + create(tmp_path / "hello") def test_create_project_fails_if_permission_denied(tmp_path): with pytest.raises(ProjectCreationError): - _create("/") + create("/") @pytest.mark.parametrize("project_name", ["hello.txt", "%%%", "COM1"]) def test_create_project_fails_if_invalid_name(project_name, tmp_path): with pytest.raises(ProjectCreationError): - _create(tmp_path / project_name) + create(tmp_path / project_name) From 091b4aef400697dbceb4b28837ac606f487a9240 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sun, 19 Jan 2025 20:13:39 +0100 Subject: [PATCH 05/57] iter --- skore/src/skore/project/_open.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/skore/src/skore/project/_open.py b/skore/src/skore/project/_open.py index af9ecb04f..f33ed4fee 100644 --- a/skore/src/skore/project/_open.py +++ b/skore/src/skore/project/_open.py @@ -1,3 +1,5 @@ +"""Helper to open a project.""" + from pathlib import Path from typing import Union From 01a6bb4fb29ad5bcdc3307c54fe67bdb424bcec3 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sun, 19 Jan 2025 20:28:17 +0100 Subject: [PATCH 06/57] iter --- skore/src/skore/cli/cli.py | 12 ++++++------ skore/src/skore/project/_create.py | 6 +++--- skore/src/skore/project/_launch.py | 2 +- skore/src/skore/project/_load.py | 2 +- skore/src/skore/project/_open.py | 14 +++++++------- skore/tests/unit/project/test_load.py | 8 ++++---- skore/tests/unit/project/test_project.py | 14 +++++++------- 7 files changed, 29 insertions(+), 29 deletions(-) diff --git a/skore/src/skore/cli/cli.py b/skore/src/skore/cli/cli.py index cda64b58c..3d59bc05a 100644 --- a/skore/src/skore/cli/cli.py +++ b/skore/src/skore/cli/cli.py @@ -5,9 +5,9 @@ from skore.cli.color_format import ColorArgumentParser from skore.project import open -from skore.project._create import create -from skore.project._launch import launch -from skore.project._load import load +from skore.project._create import _create +from skore.project._launch import _launch +from skore.project._load import _load def cli(args: list[str]): @@ -108,14 +108,14 @@ def cli(args: list[str]): parsed_args: argparse.Namespace = parser.parse_args(args) if parsed_args.subcommand == "create": - create( + _create( project_name=parsed_args.project_name, overwrite=parsed_args.overwrite, verbose=parsed_args.verbose, ) elif parsed_args.subcommand == "launch": - launch( - project=load(project_name=parsed_args.project_name), + _launch( + project=_load(project_name=parsed_args.project_name), port=parsed_args.port, open_browser=parsed_args.open_browser, verbose=parsed_args.verbose, diff --git a/skore/src/skore/project/_create.py b/skore/src/skore/project/_create.py index d67692ecf..39d67be0b 100644 --- a/skore/src/skore/project/_create.py +++ b/skore/src/skore/project/_create.py @@ -10,7 +10,7 @@ ProjectCreationError, ProjectPermissionError, ) -from skore.project._load import load +from skore.project._load import _load from skore.project.project import Project, logger from skore.utils._logger import logger_context from skore.view.view import View @@ -57,7 +57,7 @@ def _validate_project_name(project_name: str) -> tuple[bool, Optional[Exception] return True, None -def create( +def _create( project_name: Union[str, Path], overwrite: bool = False, verbose: bool = False, @@ -143,7 +143,7 @@ def create( f"Unable to create project file '{views_dir}'." ) from e - p = load(project_directory) + p = _load(project_directory) p.put_view("default", View(layout=[])) console.rule("[bold cyan]skore[/bold cyan]") diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 0de2826da..69dc55c68 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -87,7 +87,7 @@ async def lifespan(app: FastAPI): await server.serve() -def launch( +def _launch( project: Project, port: int | None = None, open_browser: bool = True, diff --git a/skore/src/skore/project/_load.py b/skore/src/skore/project/_load.py index fd8a947a6..6e8f321cb 100644 --- a/skore/src/skore/project/_load.py +++ b/skore/src/skore/project/_load.py @@ -10,7 +10,7 @@ from skore.view.view_repository import ViewRepository -def load(project_name: Union[str, Path]) -> Project: +def _load(project_name: Union[str, Path]) -> Project: """Load an existing Project given a project name or path. Transforms a project name to a directory path as follows: diff --git a/skore/src/skore/project/_open.py b/skore/src/skore/project/_open.py index f33ed4fee..f1ee343bb 100644 --- a/skore/src/skore/project/_open.py +++ b/skore/src/skore/project/_open.py @@ -52,22 +52,22 @@ def open( ProjectCreationError If project creation fails for some reason (e.g. if the project path is invalid) """ - from skore.project._create import create - from skore.project._launch import launch - from skore.project._load import load + from skore.project._create import _create + from skore.project._launch import _launch + from skore.project._load import _load try: - project = load(project_path) + project = _load(project_path) if overwrite: # let _create handle the overwrite flag - project = create(project_path, overwrite=overwrite, verbose=verbose) + project = _create(project_path, overwrite=overwrite, verbose=verbose) except FileNotFoundError: if create: - project = create(project_path, overwrite=overwrite, verbose=verbose) + project = _create(project_path, overwrite=overwrite, verbose=verbose) else: raise if serve: - launch(project, port=port, verbose=verbose) + _launch(project, port=port, verbose=verbose) return project diff --git a/skore/tests/unit/project/test_load.py b/skore/tests/unit/project/test_load.py index a199f8d91..6bd07fbab 100644 --- a/skore/tests/unit/project/test_load.py +++ b/skore/tests/unit/project/test_load.py @@ -2,7 +2,7 @@ import pytest from skore.project import Project -from skore.project._load import load +from skore.project._load import _load @pytest.fixture @@ -18,15 +18,15 @@ def tmp_project_path(tmp_path): def test_load_no_project(): with pytest.raises(FileNotFoundError): - load("/empty") + _load("/empty") def test_load_absolute_path(tmp_project_path): - p = load(tmp_project_path) + p = _load(tmp_project_path) assert isinstance(p, Project) def test_load_relative_path(tmp_project_path): os.chdir(tmp_project_path.parent) - p = load(tmp_project_path.name) + p = _load(tmp_project_path.name) assert isinstance(p, Project) diff --git a/skore/tests/unit/project/test_project.py b/skore/tests/unit/project/test_project.py index 3266dfee0..7f93cd2e9 100644 --- a/skore/tests/unit/project/test_project.py +++ b/skore/tests/unit/project/test_project.py @@ -15,7 +15,7 @@ InvalidProjectNameError, ProjectCreationError, ) -from skore.project._create import _validate_project_name, create +from skore.project._create import _create, _validate_project_name from skore.view.view import View @@ -266,30 +266,30 @@ def test_validate_project_name(project_name, expected): @pytest.mark.parametrize("project_name", ["hello", "hello.skore"]) def test_create_project(project_name, tmp_path): - create(tmp_path / project_name) + _create(tmp_path / project_name) assert (tmp_path / "hello.skore").exists() # TODO: If using fixtures in test cases is possible, join this with # `test_create_project` def test_create_project_absolute_path(tmp_path): - create(tmp_path / "hello") + _create(tmp_path / "hello") assert (tmp_path / "hello.skore").exists() def test_create_project_fails_if_file_exists(tmp_path): - create(tmp_path / "hello") + _create(tmp_path / "hello") assert (tmp_path / "hello.skore").exists() with pytest.raises(FileExistsError): - create(tmp_path / "hello") + _create(tmp_path / "hello") def test_create_project_fails_if_permission_denied(tmp_path): with pytest.raises(ProjectCreationError): - create("/") + _create("/") @pytest.mark.parametrize("project_name", ["hello.txt", "%%%", "COM1"]) def test_create_project_fails_if_invalid_name(project_name, tmp_path): with pytest.raises(ProjectCreationError): - create(tmp_path / project_name) + _create(tmp_path / project_name) From 8a75f102276c19a6b5acd8e0bde6090795784531 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sun, 19 Jan 2025 20:32:34 +0100 Subject: [PATCH 07/57] iter --- skore/src/skore/project/_launch.py | 5 +++-- skore/src/skore/project/_open.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 69dc55c68..ee23404e1 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -5,6 +5,7 @@ import webbrowser from concurrent.futures import ThreadPoolExecutor from contextlib import asynccontextmanager +from typing import Union import uvicorn from fastapi import FastAPI @@ -35,7 +36,7 @@ def get_instance(cls): def start_server( self, project: Project, - port: int | None = None, + port: Union[int, None] = None, open_browser: bool = True, ): from skore import console @@ -89,7 +90,7 @@ async def lifespan(app: FastAPI): def _launch( project: Project, - port: int | None = None, + port: Union[int, None] = None, open_browser: bool = True, verbose: bool = False, ): diff --git a/skore/src/skore/project/_open.py b/skore/src/skore/project/_open.py index f1ee343bb..931fff794 100644 --- a/skore/src/skore/project/_open.py +++ b/skore/src/skore/project/_open.py @@ -12,7 +12,7 @@ def open( create: bool = True, overwrite: bool = False, serve: bool = True, - port: int | None = None, + port: Union[int, None] = None, verbose: bool = False, ) -> Project: """Open a project given a project name or path and launch skore UI. @@ -34,7 +34,7 @@ def open( Has no effect otherwise. serve: bool, default=True Whether to launch the skore UI. - port: int | None, default=None + port: int, default=None Port at which to bind the UI server. If ``None``, the server will be bound to an available port. verbose : bool, default=False From 92b70f606094bb78261b0763949793e9aab4fc4c Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sun, 19 Jan 2025 21:07:11 +0100 Subject: [PATCH 08/57] iter --- skore/tests/unit/cli/test_cli.py | 39 +++++++++++++++++++++++++++++++ skore/tests/unit/cli/test_open.py | 1 - 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 467f50637..35bc52f18 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -56,3 +56,42 @@ def test_cli_launch(tmp_project_path, mock_server_manager): def test_cli_launch_no_project_name(): with pytest.raises(SystemExit): cli(["launch", "--port", 0, "--no-open-browser", "--verbose"]) + + +def test_cli_create(tmp_path, monkeypatch): + """Test that CLI create command creates the expected directory structure.""" + os.chdir(tmp_path) # Change to temp directory for relative path testing + + cli(["create", "test_project", "--verbose"]) + + project_path = tmp_path / "test_project.skore" + assert project_path.exists() + assert (project_path / "items").exists() + assert (project_path / "views").exists() + + with pytest.raises(FileExistsError): + cli(["create", "test_project"]) + + cli(["create", "test_project", "--overwrite"]) + assert project_path.exists() + + +def test_cli_open(tmp_path, mock_server_manager): + """Test that CLI open command works with different scenarios.""" + os.chdir(tmp_path) + + cli(["open", "test_project", "--no-serve"]) + project_path = tmp_path / "test_project.skore" + assert project_path.exists() + assert (project_path / "items").exists() + assert (project_path / "views").exists() + assert mock_server_manager.start_params is None + + cli(["open", "test_project", "--port", "8000", "--verbose"]) + assert mock_server_manager.start_params["port"] == 8000 + + cli(["open", "test_project", "--no-create", "--port", "8001"]) + assert mock_server_manager.start_params["port"] == 8001 + + with pytest.raises(FileNotFoundError): + cli(["open", "nonexistent_project", "--no-create"]) diff --git a/skore/tests/unit/cli/test_open.py b/skore/tests/unit/cli/test_open.py index 9cb1781a3..62012d154 100644 --- a/skore/tests/unit/cli/test_open.py +++ b/skore/tests/unit/cli/test_open.py @@ -89,5 +89,4 @@ def test_cli_open_no_serve(tmp_path, mock_server_manager): cli(["open", str(project_path), "--create", "--no-serve"]) - # Verify server was not started assert mock_server_manager.start_params is None From c3deb5c9fb6205751770e7720bde96a160e4c207 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sun, 19 Jan 2025 21:40:46 +0100 Subject: [PATCH 09/57] iter --- skore/tests/unit/project/test_create.py | 152 +++++++++++++++++++++++ skore/tests/unit/project/test_project.py | 60 --------- 2 files changed, 152 insertions(+), 60 deletions(-) create mode 100644 skore/tests/unit/project/test_create.py diff --git a/skore/tests/unit/project/test_create.py b/skore/tests/unit/project/test_create.py new file mode 100644 index 000000000..60658f8eb --- /dev/null +++ b/skore/tests/unit/project/test_create.py @@ -0,0 +1,152 @@ +from pathlib import Path + +import pytest +from skore.exceptions import ( + InvalidProjectNameError, + ProjectCreationError, + ProjectPermissionError, +) +from skore.project._create import _create, _validate_project_name + + +@pytest.mark.parametrize( + "project_name,expected", + [ + ( + "a" * 250, + (False, InvalidProjectNameError()), + ), + ( + "%", + (False, InvalidProjectNameError()), + ), + ( + "hello world", + (False, InvalidProjectNameError()), + ), + ], +) +def test_validate_project_name(project_name, expected): + result, exception = _validate_project_name(project_name) + expected_result, expected_exception = expected + assert result == expected_result + assert type(exception) is type(expected_exception) + + +@pytest.mark.parametrize("project_name", ["hello", "hello.skore"]) +def test_create_project(project_name, tmp_path): + _create(tmp_path / project_name) + assert (tmp_path / "hello.skore").exists() + + +# TODO: If using fixtures in test cases is possible, join this with +# `test_create_project` +def test_create_project_absolute_path(tmp_path): + _create(tmp_path / "hello") + assert (tmp_path / "hello.skore").exists() + + +def test_create_project_fails_if_file_exists(tmp_path): + _create(tmp_path / "hello") + assert (tmp_path / "hello.skore").exists() + with pytest.raises(FileExistsError): + _create(tmp_path / "hello") + + +def test_create_project_fails_if_permission_denied(tmp_path): + with pytest.raises(ProjectCreationError): + _create("/") + + +@pytest.mark.parametrize("project_name", ["hello.txt", "%%%", "COM1"]) +def test_create_project_fails_if_invalid_name(project_name, tmp_path): + with pytest.raises(ProjectCreationError): + _create(tmp_path / project_name) + + +def test_create_project_invalid_names(): + """Test project creation with invalid names.""" + invalid_names = [ + "CON", # Reserved name + "COM1", # Reserved name + "LPT1", # Reserved name + "-invalid", # Starts with hyphen + "_invalid", # Starts with underscore + "invalid@project", # Invalid character + "a" * 251, # Too long (255 - len('.skore')) + ] + + for name in invalid_names: + with pytest.raises(ProjectCreationError): + _create(name) + + +def test_create_project_existing_no_overwrite(tmp_path): + """Test creating project with existing name without overwrite flag.""" + project_path = tmp_path / "existing_project" + + _create(project_path) + with pytest.raises(FileExistsError): + _create(project_path) + + +def test_create_project_permission_error(tmp_path, monkeypatch): + """Test project creation with permission error.""" + + def mock_mkdir(*args, **kwargs): + raise PermissionError("Permission denied") + + # Patch Path.mkdir to raise PermissionError + monkeypatch.setattr(Path, "mkdir", mock_mkdir) + + with pytest.raises(ProjectPermissionError): + _create(tmp_path / "permission_denied") + + +def test_create_project_general_error(tmp_path, monkeypatch): + """Test project creation with general error.""" + + def mock_mkdir(*args, **kwargs): + raise RuntimeError("Unknown error") + + monkeypatch.setattr(Path, "mkdir", mock_mkdir) + + with pytest.raises(ProjectCreationError): + _create(tmp_path / "error_project") + + +def test_create_project_subdirectory_errors(tmp_path, monkeypatch): + """Test project creation fails when unable to create subdirectories.""" + + original_mkdir = Path.mkdir + failed_path = None + + def mock_mkdir(*args, **kwargs): + nonlocal failed_path + self = args[0] if args else kwargs.get("self") + + # Let the project directory creation succeed + if self.name.endswith(".skore"): + return original_mkdir(*args, **kwargs) + + # Fail if this is the path we want to fail + if failed_path and self.name == failed_path: + raise RuntimeError(f"Failed to create {failed_path} directory") + + return original_mkdir(*args, **kwargs) + + monkeypatch.setattr(Path, "mkdir", mock_mkdir) + + # Test items directory creation failure + failed_path = "items" + with pytest.raises(ProjectCreationError) as exc_info: + _create(tmp_path / "failed_items") + assert "Unable to create project file" in str(exc_info.value) + assert (tmp_path / "failed_items.skore").exists() + + # Test views directory creation failure + failed_path = "views" + with pytest.raises(ProjectCreationError) as exc_info: + _create(tmp_path / "failed_views") + assert "Unable to create project file" in str(exc_info.value) + assert (tmp_path / "failed_views.skore").exists() diff --git a/skore/tests/unit/project/test_project.py b/skore/tests/unit/project/test_project.py index 7f93cd2e9..540bc1714 100644 --- a/skore/tests/unit/project/test_project.py +++ b/skore/tests/unit/project/test_project.py @@ -11,11 +11,6 @@ from matplotlib import pyplot as plt from PIL import Image from sklearn.ensemble import RandomForestClassifier -from skore.exceptions import ( - InvalidProjectNameError, - ProjectCreationError, -) -from skore.project._create import _create, _validate_project_name from skore.view.view import View @@ -238,58 +233,3 @@ def test_put_wrong_key_and_value_raise(in_memory_project): """When `on_error` is "raise", raise the first error that occurs.""" with pytest.raises(TypeError): in_memory_project.put(0, (lambda: "unsupported object")) - - -test_cases = [ - ( - "a" * 250, - (False, InvalidProjectNameError()), - ), - ( - "%", - (False, InvalidProjectNameError()), - ), - ( - "hello world", - (False, InvalidProjectNameError()), - ), -] - - -@pytest.mark.parametrize("project_name,expected", test_cases) -def test_validate_project_name(project_name, expected): - result, exception = _validate_project_name(project_name) - expected_result, expected_exception = expected - assert result == expected_result - assert type(exception) is type(expected_exception) - - -@pytest.mark.parametrize("project_name", ["hello", "hello.skore"]) -def test_create_project(project_name, tmp_path): - _create(tmp_path / project_name) - assert (tmp_path / "hello.skore").exists() - - -# TODO: If using fixtures in test cases is possible, join this with -# `test_create_project` -def test_create_project_absolute_path(tmp_path): - _create(tmp_path / "hello") - assert (tmp_path / "hello.skore").exists() - - -def test_create_project_fails_if_file_exists(tmp_path): - _create(tmp_path / "hello") - assert (tmp_path / "hello.skore").exists() - with pytest.raises(FileExistsError): - _create(tmp_path / "hello") - - -def test_create_project_fails_if_permission_denied(tmp_path): - with pytest.raises(ProjectCreationError): - _create("/") - - -@pytest.mark.parametrize("project_name", ["hello.txt", "%%%", "COM1"]) -def test_create_project_fails_if_invalid_name(project_name, tmp_path): - with pytest.raises(ProjectCreationError): - _create(tmp_path / project_name) From b4560665e0764d2ec7f6df845b58b18ab952ebab Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Mon, 20 Jan 2025 00:23:22 +0100 Subject: [PATCH 10/57] iter --- skore/src/skore/project/_launch.py | 93 +++++++++++++++++++++---- skore/src/skore/project/project.py | 9 +++ skore/tests/unit/project/test_launch.py | 53 ++++++++++++++ 3 files changed, 142 insertions(+), 13 deletions(-) create mode 100644 skore/tests/unit/project/test_launch.py diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index ee23404e1..638e7941e 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -1,7 +1,9 @@ """Helpers to create, load, and launch projects.""" import asyncio +import contextlib import socket +import time import webbrowser from concurrent.futures import ThreadPoolExecutor from contextlib import asynccontextmanager @@ -26,6 +28,12 @@ class ServerManager: _instance = None _executor = None _port = None + _server_running = False + + def __init__(self): + self._timestamp = time.time() + self._loop = None + self._server_task = None @classmethod def get_instance(cls): @@ -42,22 +50,34 @@ def start_server( from skore import console console.rule("[bold cyan]skore-UI[/bold cyan]") - if self._executor is not None: - console.print(f"Server is already running at http://localhost:{self._port}") + if self._executor is not None and self._server_running: + console.print(f"Server is already running at http://localhost:{port}") return def run_in_thread(): - loop = asyncio.new_event_loop() - asyncio.set_event_loop(loop) + self._loop = asyncio.new_event_loop() + asyncio.set_event_loop(self._loop) try: self._port = port - loop.run_until_complete( + self._server_running = True + self._loop.run_until_complete( run_server(project, port, open_browser, console) ) - except KeyboardInterrupt: - console.print("Closing skore UI") + except (KeyboardInterrupt, asyncio.CancelledError): + pass finally: - loop.close() + self._server_running = False + try: + tasks = asyncio.all_tasks(self._loop) + for task in tasks: + task.cancel() + self._loop.run_until_complete( + asyncio.gather(*tasks, return_exceptions=True) + ) + self._loop.close() + except Exception: + pass + console.rule("[bold cyan]skore-UI[/bold cyan]") console.print( f"Server that was running at http://localhost:{self._port} has " "been closed" @@ -65,27 +85,74 @@ def run_in_thread(): self._executor = ThreadPoolExecutor(max_workers=1) self._executor.submit(run_in_thread) + project._server_manager = self self._executor.shutdown(wait=False) + def shutdown(self): + """Shutdown the server and cleanup resources.""" + if self._executor is not None and self._server_running: + if self._loop is not None: + + async def stop(): + tasks = asyncio.all_tasks(self._loop) + for task in tasks: + if not task.done(): + task.cancel() + await asyncio.gather(*tasks, return_exceptions=True) + + try: + future = asyncio.run_coroutine_threadsafe(stop(), self._loop) + future.result(timeout=0.1) + except Exception: + pass + + self._server_running = False + with contextlib.suppress(Exception): + self._executor.shutdown(wait=True, cancel_futures=True) + self._executor = None + self._loop = None + async def run_server(project: Project, port: int, open_browser: bool, console: Console): @asynccontextmanager async def lifespan(app: FastAPI): - if open_browser: # This was previously hardcoded in _launch + if open_browser: webbrowser.open(f"http://localhost:{port}") - yield + try: + yield + except asyncio.CancelledError: + project._server_manager = None from skore.ui.app import create_app app = create_app(project=project, lifespan=lifespan) - server_config = uvicorn.Config(app, port=port, log_level="error") + server_config = uvicorn.Config( + app, + port=port, + log_level="error", + log_config={ + "version": 1, + "disable_existing_loggers": True, + "handlers": { + "default": { + "class": "logging.NullHandler", + } + }, + "loggers": { + "uvicorn": {"handlers": ["default"], "level": "ERROR"}, + "uvicorn.error": {"handlers": ["default"], "level": "ERROR"}, + }, + }, + ) server = uvicorn.Server(server_config) - console.print( f"Running skore UI from '{project.name}' at URL http://localhost:{port}" ) - await server.serve() + try: + await server.serve() + except asyncio.CancelledError: + await server.shutdown() def _launch( diff --git a/skore/src/skore/project/project.py b/skore/src/skore/project/project.py index c7edcee47..c08621bab 100644 --- a/skore/src/skore/project/project.py +++ b/skore/src/skore/project/project.py @@ -69,6 +69,8 @@ class A: However some of these libraries support importing this raw data. """ + _server_manager = None + def __init__( self, name: str, @@ -361,3 +363,10 @@ def delete_note(self, key: str, *, version=-1): >>> project.delete_note("key", version=0) # doctest: +SKIP """ return self.item_repository.delete_item_note(key=key, version=version) + + def shutdown_web_ui(self): + """Shutdown the web UI server if it is running.""" + if self._server_manager is not None: + self._server_manager.shutdown() + else: + raise RuntimeError("UI server is not running") diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py new file mode 100644 index 000000000..6eeaaa31f --- /dev/null +++ b/skore/tests/unit/project/test_launch.py @@ -0,0 +1,53 @@ +import socket +import time + +from skore.project._create import _create +from skore.project._launch import ( + ServerManager, + _launch, + find_free_port, +) + + +def test_find_free_port(): + """Test that find_free_port returns a valid port number""" + port = find_free_port() + assert isinstance(port, int) + assert port > 0 + + # Verify we can bind to the port since it should be free + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + try: + sock.bind(("", port)) + sock.listen(1) + finally: + sock.close() + + +def test_server_manager_singleton(): + """Test ServerManager singleton pattern""" + server_manager = ServerManager.get_instance() + assert isinstance(server_manager, ServerManager) + assert ServerManager.get_instance() is server_manager + + +def test_launch(capsys, tmp_path): + """Check the general behaviour of the launch function.""" + skore_project = _create(tmp_path / "test_project") + try: + _launch(skore_project, port=8000, open_browser=False, verbose=True) + + time.sleep(0.1) # let the server start + assert skore_project._server_manager is not None + assert skore_project._server_manager is ServerManager.get_instance() + assert "Running skore UI" in capsys.readouterr().out + + _launch(skore_project, port=8000, open_browser=False, verbose=True) + + time.sleep(0.1) + assert skore_project._server_manager is ServerManager.get_instance() + assert "Server is already running" in capsys.readouterr().out + finally: + skore_project.shutdown_web_ui() + time.sleep(0.1) # let the server shutdown + assert "Server that was running" in capsys.readouterr().out From 37b49c3dfd1e468d8b1cc4ad5cd33605bab84141 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Mon, 20 Jan 2025 00:26:50 +0100 Subject: [PATCH 11/57] test --- skore/tests/unit/project/test_project.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/skore/tests/unit/project/test_project.py b/skore/tests/unit/project/test_project.py index 540bc1714..3465b3820 100644 --- a/skore/tests/unit/project/test_project.py +++ b/skore/tests/unit/project/test_project.py @@ -233,3 +233,8 @@ def test_put_wrong_key_and_value_raise(in_memory_project): """When `on_error` is "raise", raise the first error that occurs.""" with pytest.raises(TypeError): in_memory_project.put(0, (lambda: "unsupported object")) + + +def test_shutdown_web_ui(in_memory_project): + with pytest.raises(RuntimeError, match="UI server is not running"): + in_memory_project.shutdown_web_ui() From f1745d1cd87ea9d7381e74d5d588b45610a9d129 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Mon, 20 Jan 2025 00:29:21 +0100 Subject: [PATCH 12/57] more tests for _load --- skore/tests/unit/project/test_load.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/skore/tests/unit/project/test_load.py b/skore/tests/unit/project/test_load.py index 6bd07fbab..a6b6778f9 100644 --- a/skore/tests/unit/project/test_load.py +++ b/skore/tests/unit/project/test_load.py @@ -1,6 +1,7 @@ import os import pytest +from skore.exceptions import ProjectLoadError from skore.project import Project from skore.project._load import _load @@ -30,3 +31,20 @@ def test_load_relative_path(tmp_project_path): os.chdir(tmp_project_path.parent) p = _load(tmp_project_path.name) assert isinstance(p, Project) + + +def test_load_corrupted_project(tmp_path): + """Test loading a project with missing subdirectories raises ProjectLoadError.""" + # Create project directory without required subdirectories + project_path = tmp_path.parent / (tmp_path.name + ".skore") + os.mkdir(project_path) + # Only create 'items' directory, leaving 'views' missing + os.mkdir(project_path / "items") + + with pytest.raises(ProjectLoadError) as exc_info: + _load(project_path) + + assert "Project" in str(exc_info.value) + assert "corrupted" in str(exc_info.value) + assert "views" in str(exc_info.value) + assert "Consider re-creating the project" in str(exc_info.value) From 354cfa88363f2ca971192d9eddf5adc1f7a0bdf4 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Mon, 20 Jan 2025 00:35:22 +0100 Subject: [PATCH 13/57] coverage --- skore/pyproject.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/skore/pyproject.toml b/skore/pyproject.toml index 37deb551e..16b0b6436 100644 --- a/skore/pyproject.toml +++ b/skore/pyproject.toml @@ -116,6 +116,8 @@ doctest_optionflags = ["ELLIPSIS", "NORMALIZE_WHITESPACE"] [tool.coverage.run] branch = true source = ["skore"] +concurrency = ["thread"] +parallel = true [tool.coverage.report] omit = ["*/externals/*", "src/*", "tests/*"] From eaf8578e15d7783851092c0ae5729a43f4a38984 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Mon, 20 Jan 2025 00:40:25 +0100 Subject: [PATCH 14/57] more extensive test --- skore/tests/unit/project/test_launch.py | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 6eeaaa31f..6e48fe8e4 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -37,17 +37,31 @@ def test_launch(capsys, tmp_path): try: _launch(skore_project, port=8000, open_browser=False, verbose=True) - time.sleep(0.1) # let the server start + time.sleep(0.5) # let the server start assert skore_project._server_manager is not None - assert skore_project._server_manager is ServerManager.get_instance() + server_manager = skore_project._server_manager + assert server_manager is ServerManager.get_instance() assert "Running skore UI" in capsys.readouterr().out - _launch(skore_project, port=8000, open_browser=False, verbose=True) + # Force server shutdown + server_manager._server_running = True # ensure it's marked as running + server_manager.shutdown() + time.sleep(0.5) # let the shutdown complete + + # Check shutdown output + output = capsys.readouterr().out + assert "Server that was running at http://localhost:8000" in output + assert not server_manager._server_running + assert server_manager._loop is None - time.sleep(0.1) + # Try launching again + _launch(skore_project, port=8000, open_browser=False, verbose=True) + time.sleep(0.5) + _launch(skore_project, port=8000, open_browser=False, verbose=True) + time.sleep(0.5) assert skore_project._server_manager is ServerManager.get_instance() assert "Server is already running" in capsys.readouterr().out finally: skore_project.shutdown_web_ui() - time.sleep(0.1) # let the server shutdown + time.sleep(0.5) # let the server shutdown assert "Server that was running" in capsys.readouterr().out From c555dabb3a165a405311025136e8e6fcdc5902b1 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Thu, 23 Jan 2025 11:56:35 +0100 Subject: [PATCH 15/57] iter --- skore/src/skore/project/_launch.py | 201 +++++++++++++----------- skore/tests/unit/project/test_launch.py | 55 +++---- 2 files changed, 132 insertions(+), 124 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 638e7941e..b6eb32e49 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -1,17 +1,17 @@ """Helpers to create, load, and launch projects.""" import asyncio +import atexit import contextlib import socket +import threading import time import webbrowser -from concurrent.futures import ThreadPoolExecutor from contextlib import asynccontextmanager from typing import Union import uvicorn from fastapi import FastAPI -from rich.console import Console from skore.project.project import Project, logger from skore.utils._logger import logger_context @@ -26,14 +26,19 @@ def find_free_port() -> int: class ServerManager: _instance = None - _executor = None _port = None _server_running = False def __init__(self): self._timestamp = time.time() self._loop = None - self._server_task = None + self._server_thread = None + self._server_ready = threading.Event() + self._server = None + atexit.register(self._cleanup_server) + + def __del__(self): + atexit.unregister(self._cleanup_server) @classmethod def get_instance(cls): @@ -41,118 +46,130 @@ def get_instance(cls): cls._instance = cls() return cls._instance + def _cleanup_server(self): + """Cleanup server resources.""" + self._server_running = False + if self._loop and not self._loop.is_closed(): + with contextlib.suppress(Exception): + if self._server: + # Schedule server shutdown in the event loop + async def shutdown(): + await self._server.shutdown() + + self._loop.call_soon_threadsafe( + lambda: asyncio.create_task(shutdown()) + ) + + for task in asyncio.all_tasks(self._loop): + task.cancel() + self._loop.stop() + self._loop.close() + + from skore import console + + console.rule("[bold cyan]skore-UI[/bold cyan]") + console.print( + f"Server that was running at http://localhost:{self._port} has " + "been closed" + ) + self._loop = None + self._server = None + self._server_thread = None + self._server_ready.clear() + + async def _run_server_async(self, project, port, open_browser, console): + """Async function to start the server and signal when it's ready.""" + + @asynccontextmanager + async def lifespan(app: FastAPI): + if open_browser: + webbrowser.open(f"http://localhost:{port}") + try: + yield + except asyncio.CancelledError: + project._server_manager = None + + from skore.ui.app import create_app + + app = create_app(project=project, lifespan=lifespan) + server_config = uvicorn.Config( + app, + port=port, + log_level="error", + log_config={ + "version": 1, + "disable_existing_loggers": True, + "handlers": { + "default": { + "class": "logging.NullHandler", + } + }, + "loggers": { + "uvicorn": {"handlers": ["default"], "level": "ERROR"}, + "uvicorn.error": {"handlers": ["default"], "level": "ERROR"}, + }, + }, + ) + self._server = uvicorn.Server(server_config) + console.print( + f"Running skore UI from '{project.name}' at URL http://localhost:{port}" + ) + + # Set ready event when server is about to start + self._server_ready.set() + + try: + await self._server.serve() + except asyncio.CancelledError: + await self._server.shutdown() + finally: + self._server_ready.clear() + def start_server( self, project: Project, port: Union[int, None] = None, open_browser: bool = True, + timeout: float = 10.0, ): from skore import console console.rule("[bold cyan]skore-UI[/bold cyan]") - if self._executor is not None and self._server_running: + if self._server_running: console.print(f"Server is already running at http://localhost:{port}") return - def run_in_thread(): + def run_server_loop(): self._loop = asyncio.new_event_loop() asyncio.set_event_loop(self._loop) + self._port = port + self._server_running = True + try: - self._port = port - self._server_running = True self._loop.run_until_complete( - run_server(project, port, open_browser, console) - ) - except (KeyboardInterrupt, asyncio.CancelledError): - pass - finally: - self._server_running = False - try: - tasks = asyncio.all_tasks(self._loop) - for task in tasks: - task.cancel() - self._loop.run_until_complete( - asyncio.gather(*tasks, return_exceptions=True) - ) - self._loop.close() - except Exception: - pass - console.rule("[bold cyan]skore-UI[/bold cyan]") - console.print( - f"Server that was running at http://localhost:{self._port} has " - "been closed" + self._run_server_async(project, port, open_browser, console) ) + except Exception: + self._cleanup_server() - self._executor = ThreadPoolExecutor(max_workers=1) - self._executor.submit(run_in_thread) + self._server_thread = threading.Thread(target=run_server_loop, daemon=True) + self._server_thread.start() project._server_manager = self - self._executor.shutdown(wait=False) + + # Wait for the server to be ready + if not self._server_ready.wait(timeout): + raise TimeoutError(f"Server failed to start within {timeout} seconds") def shutdown(self): """Shutdown the server and cleanup resources.""" - if self._executor is not None and self._server_running: - if self._loop is not None: - - async def stop(): - tasks = asyncio.all_tasks(self._loop) - for task in tasks: - if not task.done(): - task.cancel() - await asyncio.gather(*tasks, return_exceptions=True) - - try: - future = asyncio.run_coroutine_threadsafe(stop(), self._loop) - future.result(timeout=0.1) - except Exception: - pass - - self._server_running = False - with contextlib.suppress(Exception): - self._executor.shutdown(wait=True, cancel_futures=True) - self._executor = None - self._loop = None + if not self._server_running: + return + if self._loop and not self._loop.is_closed(): + with contextlib.suppress(Exception): + self._loop.call_soon_threadsafe(self._loop.stop) -async def run_server(project: Project, port: int, open_browser: bool, console: Console): - @asynccontextmanager - async def lifespan(app: FastAPI): - if open_browser: - webbrowser.open(f"http://localhost:{port}") - try: - yield - except asyncio.CancelledError: - project._server_manager = None - - from skore.ui.app import create_app - - app = create_app(project=project, lifespan=lifespan) - server_config = uvicorn.Config( - app, - port=port, - log_level="error", - log_config={ - "version": 1, - "disable_existing_loggers": True, - "handlers": { - "default": { - "class": "logging.NullHandler", - } - }, - "loggers": { - "uvicorn": {"handlers": ["default"], "level": "ERROR"}, - "uvicorn.error": {"handlers": ["default"], "level": "ERROR"}, - }, - }, - ) - server = uvicorn.Server(server_config) - console.print( - f"Running skore UI from '{project.name}' at URL http://localhost:{port}" - ) - - try: - await server.serve() - except asyncio.CancelledError: - await server.shutdown() + self._cleanup_server() def _launch( diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 6e48fe8e4..03ac43c95 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -1,5 +1,4 @@ import socket -import time from skore.project._create import _create from skore.project._launch import ( @@ -34,34 +33,26 @@ def test_server_manager_singleton(): def test_launch(capsys, tmp_path): """Check the general behaviour of the launch function.""" skore_project = _create(tmp_path / "test_project") - try: - _launch(skore_project, port=8000, open_browser=False, verbose=True) - - time.sleep(0.5) # let the server start - assert skore_project._server_manager is not None - server_manager = skore_project._server_manager - assert server_manager is ServerManager.get_instance() - assert "Running skore UI" in capsys.readouterr().out - - # Force server shutdown - server_manager._server_running = True # ensure it's marked as running - server_manager.shutdown() - time.sleep(0.5) # let the shutdown complete - - # Check shutdown output - output = capsys.readouterr().out - assert "Server that was running at http://localhost:8000" in output - assert not server_manager._server_running - assert server_manager._loop is None - - # Try launching again - _launch(skore_project, port=8000, open_browser=False, verbose=True) - time.sleep(0.5) - _launch(skore_project, port=8000, open_browser=False, verbose=True) - time.sleep(0.5) - assert skore_project._server_manager is ServerManager.get_instance() - assert "Server is already running" in capsys.readouterr().out - finally: - skore_project.shutdown_web_ui() - time.sleep(0.5) # let the server shutdown - assert "Server that was running" in capsys.readouterr().out + _launch(skore_project, port=8000, open_browser=False, verbose=True) + + assert skore_project._server_manager is not None + server_manager = skore_project._server_manager + assert server_manager is ServerManager.get_instance() + assert "Running skore UI" in capsys.readouterr().out + + # Force server shutdown + server_manager._server_running = True # ensure it's marked as running + server_manager.shutdown() + assert server_manager._server_running is False + + # Check shutdown output + output = capsys.readouterr().out + assert "Server that was running" in output + assert not server_manager._server_running + assert server_manager._loop is None + + # # Try launching again + _launch(skore_project, port=8000, open_browser=False, verbose=True) + _launch(skore_project, port=8000, open_browser=False, verbose=True) + assert skore_project._server_manager is ServerManager.get_instance() + assert "Server is already running" in capsys.readouterr().out From edaa1daccf84dd0510d7ca3029bfcc16be0fd6b1 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Thu, 23 Jan 2025 14:24:48 +0100 Subject: [PATCH 16/57] iter --- skore/src/skore/project/_launch.py | 14 +++++++++----- skore/src/skore/project/project.py | 1 + 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index b6eb32e49..94cde63ca 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -34,6 +34,7 @@ def __init__(self): self._loop = None self._server_thread = None self._server_ready = threading.Event() + self._cleanup_complete = threading.Event() self._server = None atexit.register(self._cleanup_server) @@ -48,6 +49,9 @@ def get_instance(cls): def _cleanup_server(self): """Cleanup server resources.""" + if not self._server_running: + self._cleanup_complete.set() + return self._server_running = False if self._loop and not self._loop.is_closed(): with contextlib.suppress(Exception): @@ -55,6 +59,7 @@ def _cleanup_server(self): # Schedule server shutdown in the event loop async def shutdown(): await self._server.shutdown() + self._cleanup_complete.set() self._loop.call_soon_threadsafe( lambda: asyncio.create_task(shutdown()) @@ -76,6 +81,8 @@ async def shutdown(): self._server = None self._server_thread = None self._server_ready.clear() + if not self._cleanup_complete.is_set(): + self._cleanup_complete.set() async def _run_server_async(self, project, port, open_browser, console): """Async function to start the server and signal when it's ready.""" @@ -84,10 +91,7 @@ async def _run_server_async(self, project, port, open_browser, console): async def lifespan(app: FastAPI): if open_browser: webbrowser.open(f"http://localhost:{port}") - try: - yield - except asyncio.CancelledError: - project._server_manager = None + yield from skore.ui.app import create_app @@ -154,7 +158,6 @@ def run_server_loop(): self._server_thread = threading.Thread(target=run_server_loop, daemon=True) self._server_thread.start() - project._server_manager = self # Wait for the server to be ready if not self._server_ready.wait(timeout): @@ -197,4 +200,5 @@ def _launch( with logger_context(logger, verbose): server_manager = ServerManager.get_instance() + project._server_manager = server_manager server_manager.start_server(project, port, open_browser) diff --git a/skore/src/skore/project/project.py b/skore/src/skore/project/project.py index c08621bab..e678fd6f0 100644 --- a/skore/src/skore/project/project.py +++ b/skore/src/skore/project/project.py @@ -368,5 +368,6 @@ def shutdown_web_ui(self): """Shutdown the web UI server if it is running.""" if self._server_manager is not None: self._server_manager.shutdown() + self._server_manager = None else: raise RuntimeError("UI server is not running") From 637f70fbe78d3871730e6b9c83e9545bcdc57137 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Fri, 24 Jan 2025 17:42:31 +0100 Subject: [PATCH 17/57] iter --- skore/src/skore/project/_launch.py | 234 +++++++++++------------------ 1 file changed, 86 insertions(+), 148 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 94cde63ca..fab6c1f1c 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -1,13 +1,13 @@ """Helpers to create, load, and launch projects.""" -import asyncio import atexit import contextlib +import json +import multiprocessing +import os import socket -import threading -import time import webbrowser -from contextlib import asynccontextmanager +from pathlib import Path from typing import Union import uvicorn @@ -24,155 +24,70 @@ def find_free_port() -> int: return s.getsockname()[1] -class ServerManager: - _instance = None - _port = None - _server_running = False - - def __init__(self): - self._timestamp = time.time() - self._loop = None - self._server_thread = None - self._server_ready = threading.Event() - self._cleanup_complete = threading.Event() - self._server = None - atexit.register(self._cleanup_server) - - def __del__(self): - atexit.unregister(self._cleanup_server) - - @classmethod - def get_instance(cls): - if cls._instance is None: - cls._instance = cls() - return cls._instance - - def _cleanup_server(self): - """Cleanup server resources.""" - if not self._server_running: - self._cleanup_complete.set() - return - self._server_running = False - if self._loop and not self._loop.is_closed(): - with contextlib.suppress(Exception): - if self._server: - # Schedule server shutdown in the event loop - async def shutdown(): - await self._server.shutdown() - self._cleanup_complete.set() - - self._loop.call_soon_threadsafe( - lambda: asyncio.create_task(shutdown()) - ) - - for task in asyncio.all_tasks(self._loop): - task.cancel() - self._loop.stop() - self._loop.close() +def run_server(project: Project, port: int, open_browser: bool): + """Run the server in a separate process.""" + from skore import console + from skore.ui.app import create_app - from skore import console + async def lifespan(app: FastAPI): + if open_browser: + webbrowser.open(f"http://localhost:{port}") + yield - console.rule("[bold cyan]skore-UI[/bold cyan]") - console.print( - f"Server that was running at http://localhost:{self._port} has " - "been closed" - ) - self._loop = None - self._server = None - self._server_thread = None - self._server_ready.clear() - if not self._cleanup_complete.is_set(): - self._cleanup_complete.set() - - async def _run_server_async(self, project, port, open_browser, console): - """Async function to start the server and signal when it's ready.""" - - @asynccontextmanager - async def lifespan(app: FastAPI): - if open_browser: - webbrowser.open(f"http://localhost:{port}") - yield - - from skore.ui.app import create_app - - app = create_app(project=project, lifespan=lifespan) - server_config = uvicorn.Config( - app, - port=port, - log_level="error", - log_config={ - "version": 1, - "disable_existing_loggers": True, - "handlers": { - "default": { - "class": "logging.NullHandler", - } - }, - "loggers": { - "uvicorn": {"handlers": ["default"], "level": "ERROR"}, - "uvicorn.error": {"handlers": ["default"], "level": "ERROR"}, - }, - }, - ) - self._server = uvicorn.Server(server_config) - console.print( - f"Running skore UI from '{project.name}' at URL http://localhost:{port}" - ) + app = create_app(project=project, lifespan=lifespan) + console.rule("[bold cyan]skore-UI[/bold cyan]") + console.print( + f"Running skore UI from '{project.name}' at URL http://localhost:{port}" + ) - # Set ready event when server is about to start - self._server_ready.set() + uvicorn.run(app, host="0.0.0.0", port=port, log_level="error") - try: - await self._server.serve() - except asyncio.CancelledError: - await self._server.shutdown() - finally: - self._server_ready.clear() - - def start_server( - self, - project: Project, - port: Union[int, None] = None, - open_browser: bool = True, - timeout: float = 10.0, - ): - from skore import console - console.rule("[bold cyan]skore-UI[/bold cyan]") - if self._server_running: - console.print(f"Server is already running at http://localhost:{port}") - return +def get_server_info_path(project: Project) -> Path: + """Get path to the server info file for this project.""" + # return Path(project.path) / ".server_info.json" + return Path("/tmp") / "skore_server_info.json" + + +def save_server_info(project: Project, port: int, pid: int): + """Save server information to disk.""" + info = {"port": port, "pid": pid} + with open(get_server_info_path(project), "w") as f: + json.dump(info, f) + + +def load_server_info(project: Project) -> Union[dict, None]: + """Load server information from disk if it exists.""" + try: + with open(get_server_info_path(project)) as f: + return json.load(f) + except (FileNotFoundError, json.JSONDecodeError): + return None - def run_server_loop(): - self._loop = asyncio.new_event_loop() - asyncio.set_event_loop(self._loop) - self._port = port - self._server_running = True - - try: - self._loop.run_until_complete( - self._run_server_async(project, port, open_browser, console) - ) - except Exception: - self._cleanup_server() - - self._server_thread = threading.Thread(target=run_server_loop, daemon=True) - self._server_thread.start() - - # Wait for the server to be ready - if not self._server_ready.wait(timeout): - raise TimeoutError(f"Server failed to start within {timeout} seconds") - - def shutdown(self): - """Shutdown the server and cleanup resources.""" - if not self._server_running: - return - if self._loop and not self._loop.is_closed(): - with contextlib.suppress(Exception): - self._loop.call_soon_threadsafe(self._loop.stop) +def cleanup_server(project: Project): + """Cleanup server resources.""" + info = load_server_info(project) + if info is None: + return - self._cleanup_server() + try: + # Try to terminate the process + os.kill(info["pid"], 15) # SIGTERM + + from skore import console + + console.rule("[bold cyan]skore-UI[/bold cyan]") + console.print( + f"Server that was running at http://localhost:{info['port']} has " + "been closed" + ) + except ProcessLookupError: + pass # Process already terminated + finally: + # Always clean up the info file + with contextlib.suppress(FileNotFoundError): + os.remove(get_server_info_path(project)) def _launch( @@ -198,7 +113,30 @@ def _launch( if port is None: port = find_free_port() + # Check if server is already running + info = load_server_info(project) + if info is not None: + try: + # Check if process is still alive + os.kill(info["pid"], 0) + from skore import console + + console.print( + f"Server is already running at http://localhost:{info['port']}" + ) + return + except ProcessLookupError: + # Process is dead, clean up the file + cleanup_server(project) + with logger_context(logger, verbose): - server_manager = ServerManager.get_instance() - project._server_manager = server_manager - server_manager.start_server(project, port, open_browser) + process = multiprocessing.Process( + target=run_server, args=(project, port, open_browser), daemon=True + ) + process.start() + + # Save server info to disk + save_server_info(project, port, process.pid) + + # Register cleanup on exit + atexit.register(cleanup_server, project) From 19342f6d7f8a6a269ae64bfebe3704d6647163ae Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Fri, 24 Jan 2025 22:51:56 +0100 Subject: [PATCH 18/57] iter --- skore/src/skore/project/_launch.py | 123 +++++++++++++++++------- skore/src/skore/project/project.py | 7 +- skore/tests/unit/project/test_launch.py | 55 +++++------ 3 files changed, 111 insertions(+), 74 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index fab6c1f1c..86fad45ce 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -5,6 +5,7 @@ import json import multiprocessing import os +import signal import socket import webbrowser from pathlib import Path @@ -17,35 +18,74 @@ from skore.utils._logger import logger_context -def find_free_port() -> int: - with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - s.bind(("", 0)) - s.listen(1) - return s.getsockname()[1] +def find_free_port(min_port: int = 22140, max_attempts: int = 100) -> int: + """Find first available port starting from min_port. + Note: Jupyter has the same brute force way to find a free port. + see: https://github.com/jupyter/jupyter_core/blob/fa513c1550bbd1ebcc14a4a79eb8c5d95e3e23c9/tests/dotipython_empty/profile_default/ipython_notebook_config.py#L28 -def run_server(project: Project, port: int, open_browser: bool): - """Run the server in a separate process.""" - from skore import console + Parameters + ---------- + min_port : int, default=22140 + Minimum port number to start searching from. + max_attempts : int, default=100 + Maximum number of ports to try before giving up. + + Returns + ------- + int + First available port `number >= min_port`. + + Raises + ------ + RuntimeError + If no free port found after `max_attempts`. + """ + for port in range(min_port, min_port + max_attempts): + try: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.bind(("", port)) + s.listen(1) + return s.getsockname()[1] + except OSError: + continue + + raise RuntimeError( + f"Could not find free port after {max_attempts}" + f"attempts starting from {min_port}" + ) + + +def run_server( + project: Project, port: int, open_browser: bool, ready_event: multiprocessing.Event +): + """Run the server in a separate process. + + Parameters + ---------- + project : Project + The project to launch. + port : int + The port to use for the server. + open_browser : bool + Whether to open the browser. + ready_event : multiprocessing.Event + Event to signal that the server is ready. + """ from skore.ui.app import create_app async def lifespan(app: FastAPI): + ready_event.set() # Signal that the server is ready if open_browser: webbrowser.open(f"http://localhost:{port}") yield app = create_app(project=project, lifespan=lifespan) - console.rule("[bold cyan]skore-UI[/bold cyan]") - console.print( - f"Running skore UI from '{project.name}' at URL http://localhost:{port}" - ) - uvicorn.run(app, host="0.0.0.0", port=port, log_level="error") def get_server_info_path(project: Project) -> Path: """Get path to the server info file for this project.""" - # return Path(project.path) / ".server_info.json" return Path("/tmp") / "skore_server_info.json" @@ -66,14 +106,18 @@ def load_server_info(project: Project) -> Union[dict, None]: def cleanup_server(project: Project): - """Cleanup server resources.""" + """Cleanup server resources. + + Returns + ------- + True if the server was successfully terminated, False otherwise. + """ info = load_server_info(project) if info is None: - return + return False try: - # Try to terminate the process - os.kill(info["pid"], 15) # SIGTERM + os.kill(info["pid"], signal.SIGTERM) from skore import console @@ -85,10 +129,11 @@ def cleanup_server(project: Project): except ProcessLookupError: pass # Process already terminated finally: - # Always clean up the info file with contextlib.suppress(FileNotFoundError): os.remove(get_server_info_path(project)) + return True + def _launch( project: Project, @@ -100,43 +145,47 @@ def _launch( Parameters ---------- - project : Project - The project to be launched. - port : int, default=None - Port at which to bind the UI server. If ``None``, the server will be bound to - an available port. - open_browser: bool, default=True - Whether to automatically open a browser tab showing the UI. - verbose: bool, default=False - Whether to display info logs to the user. + project : Project + The project to launch. + port : int, optional + The port to use for the server, by default None. + open_browser : bool, optional + Whether to open the browser, by default True. + verbose : bool, optional + Whether to print verbose output, by default False. """ + from skore import console # avoid circular import + if port is None: port = find_free_port() - # Check if server is already running info = load_server_info(project) if info is not None: try: - # Check if process is still alive os.kill(info["pid"], 0) - from skore import console console.print( f"Server is already running at http://localhost:{info['port']}" ) return - except ProcessLookupError: - # Process is dead, clean up the file + except ProcessLookupError: # zombie process cleanup_server(project) + ready_event = multiprocessing.Event() + with logger_context(logger, verbose): process = multiprocessing.Process( - target=run_server, args=(project, port, open_browser), daemon=True + target=run_server, + args=(project, port, open_browser, ready_event), + daemon=True, ) process.start() - - # Save server info to disk save_server_info(project, port, process.pid) + ready_event.wait() # wait for server to been started + + console.rule("[bold cyan]skore-UI[/bold cyan]") + console.print( + f"Running skore UI from '{project.name}' at URL http://localhost:{port}" + ) - # Register cleanup on exit atexit.register(cleanup_server, project) diff --git a/skore/src/skore/project/project.py b/skore/src/skore/project/project.py index ce8d7f07b..543e16e64 100644 --- a/skore/src/skore/project/project.py +++ b/skore/src/skore/project/project.py @@ -280,10 +280,9 @@ def delete_note(self, key: str, *, version=-1): def shutdown_web_ui(self): """Shutdown the web UI server if it is running.""" - if self._server_manager is not None: - self._server_manager.shutdown() - self._server_manager = None - else: + from skore.project._launch import cleanup_server + + if not cleanup_server(self): raise RuntimeError("UI server is not running") def clear(self): diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 03ac43c95..9cdc2be63 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -1,11 +1,7 @@ import socket from skore.project._create import _create -from skore.project._launch import ( - ServerManager, - _launch, - find_free_port, -) +from skore.project._launch import _launch, find_free_port def test_find_free_port(): @@ -23,36 +19,29 @@ def test_find_free_port(): sock.close() -def test_server_manager_singleton(): - """Test ServerManager singleton pattern""" - server_manager = ServerManager.get_instance() - assert isinstance(server_manager, ServerManager) - assert ServerManager.get_instance() is server_manager - - def test_launch(capsys, tmp_path): """Check the general behaviour of the launch function.""" skore_project = _create(tmp_path / "test_project") _launch(skore_project, port=8000, open_browser=False, verbose=True) - assert skore_project._server_manager is not None - server_manager = skore_project._server_manager - assert server_manager is ServerManager.get_instance() - assert "Running skore UI" in capsys.readouterr().out - - # Force server shutdown - server_manager._server_running = True # ensure it's marked as running - server_manager.shutdown() - assert server_manager._server_running is False - - # Check shutdown output - output = capsys.readouterr().out - assert "Server that was running" in output - assert not server_manager._server_running - assert server_manager._loop is None - - # # Try launching again - _launch(skore_project, port=8000, open_browser=False, verbose=True) - _launch(skore_project, port=8000, open_browser=False, verbose=True) - assert skore_project._server_manager is ServerManager.get_instance() - assert "Server is already running" in capsys.readouterr().out + # assert skore_project._server_manager is not None + # server_manager = skore_project._server_manager + # assert server_manager is ServerManager.get_instance() + # assert "Running skore UI" in capsys.readouterr().out + + # # Force server shutdown + # server_manager._server_running = True # ensure it's marked as running + # server_manager.shutdown() + # assert server_manager._server_running is False + + # # Check shutdown output + # output = capsys.readouterr().out + # assert "Server that was running" in output + # assert not server_manager._server_running + # assert server_manager._loop is None + + # # # Try launching again + # _launch(skore_project, port=8000, open_browser=False, verbose=True) + # _launch(skore_project, port=8000, open_browser=False, verbose=True) + # assert skore_project._server_manager is ServerManager.get_instance() + # assert "Server is already running" in capsys.readouterr().out From bd7d926c449a54c34efe1907a6543c8b61339e76 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Fri, 24 Jan 2025 22:58:40 +0100 Subject: [PATCH 19/57] iter --- skore/src/skore/project/_launch.py | 1 + skore/tests/unit/project/test_launch.py | 29 +++++++------------------ 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 86fad45ce..008f47380 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -164,6 +164,7 @@ def _launch( try: os.kill(info["pid"], 0) + console.rule("[bold cyan]skore-UI[/bold cyan]") console.print( f"Server is already running at http://localhost:{info['port']}" ) diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 9cdc2be63..20ecd12cb 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -23,25 +23,12 @@ def test_launch(capsys, tmp_path): """Check the general behaviour of the launch function.""" skore_project = _create(tmp_path / "test_project") _launch(skore_project, port=8000, open_browser=False, verbose=True) + assert "Running skore UI" in capsys.readouterr().out - # assert skore_project._server_manager is not None - # server_manager = skore_project._server_manager - # assert server_manager is ServerManager.get_instance() - # assert "Running skore UI" in capsys.readouterr().out - - # # Force server shutdown - # server_manager._server_running = True # ensure it's marked as running - # server_manager.shutdown() - # assert server_manager._server_running is False - - # # Check shutdown output - # output = capsys.readouterr().out - # assert "Server that was running" in output - # assert not server_manager._server_running - # assert server_manager._loop is None - - # # # Try launching again - # _launch(skore_project, port=8000, open_browser=False, verbose=True) - # _launch(skore_project, port=8000, open_browser=False, verbose=True) - # assert skore_project._server_manager is ServerManager.get_instance() - # assert "Server is already running" in capsys.readouterr().out + skore_project.shutdown_web_ui() + output = capsys.readouterr().out + assert "Server that was running" in output + + _launch(skore_project, port=8000, open_browser=False, verbose=True) + _launch(skore_project, port=8000, open_browser=False, verbose=True) + assert "Server is already running" in capsys.readouterr().out From e616f7976e72f07586750b50bb7a7f82f418d886 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 00:08:24 +0100 Subject: [PATCH 20/57] iter --- skore/tests/unit/cli/test_cli.py | 32 +++++++---------------- skore/tests/unit/cli/test_open.py | 42 +++++++++++++------------------ 2 files changed, 26 insertions(+), 48 deletions(-) diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 35bc52f18..b15215532 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -18,25 +18,18 @@ def tmp_project_path(tmp_path): @pytest.fixture -def mock_server_manager(monkeypatch): - """Mock ServerManager to verify server startup parameters.""" +def mock_launch(monkeypatch): + """Fixture to patch _launch function to prevent browser opening.""" - class MockServerManager: - def __init__(self): - self.start_params = None + def mock_launch_fn(project, port=None, open_browser=True, verbose=False): + from skore.project._launch import _launch as original_launch - def get_instance(self): - return self + return original_launch(project, port=port, open_browser=False, verbose=verbose) - def start_server(self, project, port=None, open_browser=True): - self.start_params = {"port": port, "open_browser": open_browser} + monkeypatch.setattr("skore.cli.cli._launch", mock_launch_fn) - mock_manager = MockServerManager() - monkeypatch.setattr("skore.project._launch.ServerManager", mock_manager) - return mock_manager - -def test_cli_launch(tmp_project_path, mock_server_manager): +def test_cli_launch(tmp_project_path): """Test that CLI launch starts server with correct parameters.""" cli( [ @@ -49,16 +42,13 @@ def test_cli_launch(tmp_project_path, mock_server_manager): ] ) - assert mock_server_manager.start_params["port"] == 8000 - assert mock_server_manager.start_params["open_browser"] is False - def test_cli_launch_no_project_name(): with pytest.raises(SystemExit): cli(["launch", "--port", 0, "--no-open-browser", "--verbose"]) -def test_cli_create(tmp_path, monkeypatch): +def test_cli_create(tmp_path): """Test that CLI create command creates the expected directory structure.""" os.chdir(tmp_path) # Change to temp directory for relative path testing @@ -76,7 +66,7 @@ def test_cli_create(tmp_path, monkeypatch): assert project_path.exists() -def test_cli_open(tmp_path, mock_server_manager): +def test_cli_open(tmp_path, mock_launch): """Test that CLI open command works with different scenarios.""" os.chdir(tmp_path) @@ -85,13 +75,9 @@ def test_cli_open(tmp_path, mock_server_manager): assert project_path.exists() assert (project_path / "items").exists() assert (project_path / "views").exists() - assert mock_server_manager.start_params is None cli(["open", "test_project", "--port", "8000", "--verbose"]) - assert mock_server_manager.start_params["port"] == 8000 - cli(["open", "test_project", "--no-create", "--port", "8001"]) - assert mock_server_manager.start_params["port"] == 8001 with pytest.raises(FileNotFoundError): cli(["open", "nonexistent_project", "--no-create"]) diff --git a/skore/tests/unit/cli/test_open.py b/skore/tests/unit/cli/test_open.py index 62012d154..b67a4ad19 100644 --- a/skore/tests/unit/cli/test_open.py +++ b/skore/tests/unit/cli/test_open.py @@ -16,26 +16,18 @@ def tmp_project_path(tmp_path): @pytest.fixture -def mock_server_manager(monkeypatch): - """Mock ServerManager to verify server startup parameters.""" +def mock_launch(monkeypatch): + """Fixture that mocks the _launch function and tracks calls to it.""" + calls = [] - class MockServerManager: - def __init__(self): - self.start_params = None + def _mock_launch(project, port=None, open_browser=True, verbose=False): + calls.append((project, port, open_browser, verbose)) - def get_instance(self): - return self + monkeypatch.setattr("skore.project._launch._launch", _mock_launch) + return calls - def start_server(self, project, port=None, open_browser=True): - # Always set open_browser to False to prevent browser opening - self.start_params = {"port": port, "open_browser": False} - mock_manager = MockServerManager() - monkeypatch.setattr("skore.project._launch.ServerManager", mock_manager) - return mock_manager - - -def test_cli_open(tmp_project_path, mock_server_manager): +def test_cli_open(tmp_project_path, mock_launch): """Test that CLI open creates a project and starts server with correct parameters.""" cli( @@ -49,29 +41,29 @@ def test_cli_open(tmp_project_path, mock_server_manager): "--verbose", ] ) + assert len(mock_launch) == 1 - assert mock_server_manager.start_params["port"] == 8000 - assert mock_server_manager.start_params["open_browser"] is False - -def test_cli_open_creates_project(tmp_path, mock_server_manager): +def test_cli_open_creates_project(tmp_path, mock_launch): """Test that CLI open creates a project when it doesn't exist.""" project_path = tmp_path / "new_project.skore" assert not project_path.exists() cli(["open", str(project_path), "--create"]) assert project_path.exists() + assert len(mock_launch) == 1 -def test_cli_open_no_create_fails(tmp_path, mock_server_manager): +def test_cli_open_no_create_fails(tmp_path, mock_launch): """Test that CLI open fails when project doesn't exist and create=False.""" project_path = tmp_path / "nonexistent.skore" with pytest.raises(FileNotFoundError): cli(["open", str(project_path), "--no-create"]) + assert len(mock_launch) == 0 -def test_cli_open_overwrite(tmp_path, mock_server_manager): +def test_cli_open_overwrite(tmp_path, mock_launch): """Test that CLI open can overwrite existing project.""" project_path = tmp_path / "overwrite_test.skore" @@ -81,12 +73,12 @@ def test_cli_open_overwrite(tmp_path, mock_server_manager): cli(["open", str(project_path), "--create", "--overwrite"]) new_time = os.path.getmtime(project_path) assert new_time > initial_time + assert len(mock_launch) == 2 -def test_cli_open_no_serve(tmp_path, mock_server_manager): +def test_cli_open_no_serve(tmp_path, mock_launch): """Test that server is not started when --no-serve flag is passed.""" project_path = tmp_path / "no_serve.skore" cli(["open", str(project_path), "--create", "--no-serve"]) - - assert mock_server_manager.start_params is None + assert len(mock_launch) == 0 From d6e7c5b9391544a3edb5cb0adbe24903e714640e Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 00:09:37 +0100 Subject: [PATCH 21/57] iter --- skore/src/skore/project/_launch.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 008f47380..68d80de72 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -7,6 +7,7 @@ import os import signal import socket +import time import webbrowser from pathlib import Path from typing import Union @@ -118,6 +119,7 @@ def cleanup_server(project: Project): try: os.kill(info["pid"], signal.SIGTERM) + time.sleep(1) from skore import console From c76a8056c5ded35c8fe48cb8a8410f6ef81dfc90 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 12:46:15 +0100 Subject: [PATCH 22/57] iter --- skore/pyproject.toml | 1 + skore/src/skore/project/_launch.py | 75 ++++++++++++++++++------------ skore/tests/unit/cli/test_cli.py | 27 +++++++---- 3 files changed, 64 insertions(+), 39 deletions(-) diff --git a/skore/pyproject.toml b/skore/pyproject.toml index 370c31156..0c37b043d 100644 --- a/skore/pyproject.toml +++ b/skore/pyproject.toml @@ -17,6 +17,7 @@ dependencies = [ "scikit-learn", "skops", "uvicorn", + "platformdirs", ] classifiers = [ "Intended Audience :: Science/Research", diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 68d80de72..72365a7da 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -5,13 +5,12 @@ import json import multiprocessing import os -import signal import socket -import time import webbrowser from pathlib import Path from typing import Union +import psutil import uvicorn from fastapi import FastAPI @@ -57,6 +56,27 @@ def find_free_port(min_port: int = 22140, max_attempts: int = 100) -> int: ) +def get_server_info_path(project: Project) -> Path: + """Get path to the server info file for this project.""" + return Path("/tmp") / "skore_server_info.json" + + +def save_server_info(project: Project, port: int, pid: int): + """Save server information to disk.""" + info = {"port": port, "pid": pid} + with open(get_server_info_path(project), "w") as f: + json.dump(info, f) + + +def load_server_info(project: Project) -> Union[dict, None]: + """Load server information from disk if it exists.""" + try: + with open(get_server_info_path(project)) as f: + return json.load(f) + except (FileNotFoundError, json.JSONDecodeError): + return None + + def run_server( project: Project, port: int, open_browser: bool, ready_event: multiprocessing.Event ): @@ -85,50 +105,47 @@ async def lifespan(app: FastAPI): uvicorn.run(app, host="0.0.0.0", port=port, log_level="error") -def get_server_info_path(project: Project) -> Path: - """Get path to the server info file for this project.""" - return Path("/tmp") / "skore_server_info.json" +def cleanup_server(project: Project, timeout: float = 5.0) -> bool: + """Cleanup server resources and wait for termination. - -def save_server_info(project: Project, port: int, pid: int): - """Save server information to disk.""" - info = {"port": port, "pid": pid} - with open(get_server_info_path(project), "w") as f: - json.dump(info, f) - - -def load_server_info(project: Project) -> Union[dict, None]: - """Load server information from disk if it exists.""" - try: - with open(get_server_info_path(project)) as f: - return json.load(f) - except (FileNotFoundError, json.JSONDecodeError): - return None - - -def cleanup_server(project: Project): - """Cleanup server resources. + Parameters + ---------- + project : Project + The project instance. + timeout : float, default=5.0 + Maximum time to wait for the process to terminate in seconds. Returns ------- - True if the server was successfully terminated, False otherwise. + bool + True if the server was successfully terminated, False if timeout occurred + or server wasn't running. """ info = load_server_info(project) if info is None: return False try: - os.kill(info["pid"], signal.SIGTERM) - time.sleep(1) + process = psutil.Process(info["pid"]) + process.terminate() + + try: + process.wait(timeout=timeout) + success = True + except psutil.TimeoutExpired: + process.kill() + success = False from skore import console console.rule("[bold cyan]skore-UI[/bold cyan]") + status = "gracefully" if success else "forcefully" console.print( f"Server that was running at http://localhost:{info['port']} has " - "been closed" + f"been closed {status}" ) - except ProcessLookupError: + + except psutil.NoSuchProcess: pass # Process already terminated finally: with contextlib.suppress(FileNotFoundError): diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index b15215532..4392b4b6d 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -17,16 +17,12 @@ def tmp_project_path(tmp_path): return project_path -@pytest.fixture -def mock_launch(monkeypatch): - """Fixture to patch _launch function to prevent browser opening.""" - - def mock_launch_fn(project, port=None, open_browser=True, verbose=False): - from skore.project._launch import _launch as original_launch - - return original_launch(project, port=port, open_browser=False, verbose=verbose) +def close_project(path): + """Force closing the web UI.""" + from skore import open - monkeypatch.setattr("skore.cli.cli._launch", mock_launch_fn) + project = open(path, serve=False) + project.shutdown_web_ui() def test_cli_launch(tmp_project_path): @@ -41,6 +37,7 @@ def test_cli_launch(tmp_project_path): "--verbose", ] ) + close_project(tmp_project_path) def test_cli_launch_no_project_name(): @@ -66,8 +63,16 @@ def test_cli_create(tmp_path): assert project_path.exists() -def test_cli_open(tmp_path, mock_launch): +def test_cli_open(tmp_path, monkeypatch): """Test that CLI open command works with different scenarios.""" + # Force open_browser to False for all _launch calls + from skore.project._launch import _launch + + monkeypatch.setattr( + "skore.project._launch._launch", + lambda *args, **kwargs: _launch(*args, **{**kwargs, "open_browser": False}), + ) + os.chdir(tmp_path) cli(["open", "test_project", "--no-serve"]) @@ -77,7 +82,9 @@ def test_cli_open(tmp_path, mock_launch): assert (project_path / "views").exists() cli(["open", "test_project", "--port", "8000", "--verbose"]) + close_project(project_path) cli(["open", "test_project", "--no-create", "--port", "8001"]) + close_project(project_path) with pytest.raises(FileNotFoundError): cli(["open", "nonexistent_project", "--no-create"]) From ae61dd7959229660be0dee324813750a158b2533 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 15:17:52 +0100 Subject: [PATCH 23/57] iter --- skore/pyproject.toml | 2 + skore/src/skore/project/_launch.py | 116 ++++++++++++++++++++++------- skore/src/skore/project/_load.py | 6 ++ skore/src/skore/project/project.py | 13 +++- skore/tests/conftest.py | 1 + 5 files changed, 107 insertions(+), 31 deletions(-) diff --git a/skore/pyproject.toml b/skore/pyproject.toml index 0c37b043d..fcf0ae3f0 100644 --- a/skore/pyproject.toml +++ b/skore/pyproject.toml @@ -18,6 +18,7 @@ dependencies = [ "skops", "uvicorn", "platformdirs", + "psutil", ] classifiers = [ "Intended Audience :: Science/Research", @@ -113,6 +114,7 @@ addopts = [ "--ignore=doc", "--ignore=examples", "--ignore=notebooks", + "--dist=loadscope", ] doctest_optionflags = ["ELLIPSIS", "NORMALIZE_WHITESPACE"] diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 72365a7da..3ca998ace 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -6,10 +6,12 @@ import multiprocessing import os import socket +import uuid import webbrowser from pathlib import Path from typing import Union +import platformdirs import psutil import uvicorn from fastapi import FastAPI @@ -56,25 +58,85 @@ def find_free_port(min_port: int = 22140, max_attempts: int = 100) -> int: ) -def get_server_info_path(project: Project) -> Path: - """Get path to the server info file for this project.""" - return Path("/tmp") / "skore_server_info.json" +class ServerInfo: + """Server information. + Parameters + ---------- + project : Project + The project to associate with the server. + port : int + The port to use for the server. + pid : int + The PID of the server process. -def save_server_info(project: Project, port: int, pid: int): - """Save server information to disk.""" - info = {"port": port, "pid": pid} - with open(get_server_info_path(project), "w") as f: - json.dump(info, f) + Attributes + ---------- + port : int + The port to use for the server. + pid : int + The PID of the server process. + pid_file : Path + The path to the PID file. + """ + @staticmethod + def _get_pid_file_path(project: Project) -> Path: + """Get the path to the PID file.""" + if project.path is not None: + project_identifier = uuid.uuid3(uuid.NAMESPACE_DNS, str(project.path)) + else: + project_identifier = uuid.uuid3(uuid.NAMESPACE_DNS, str(project.name)) + return ( + platformdirs.user_state_path(appname="skore") + / f"skore-server-{project_identifier}.json" + ) -def load_server_info(project: Project) -> Union[dict, None]: - """Load server information from disk if it exists.""" - try: - with open(get_server_info_path(project)) as f: - return json.load(f) - except (FileNotFoundError, json.JSONDecodeError): - return None + def __init__(self, project: Project, port: int, pid: int): + self.port = port + self.pid = pid + self.pid_file = self._get_pid_file_path(project) + + @classmethod + def rejoin(cls, project: Project): + """Rejoin a project to a server. + + Parameters + ---------- + project : Project + The project to associate with the server. + + Returns + ------- + ServerInfo + The server information. + """ + pid_file = cls._get_pid_file_path(project) + if not pid_file.exists(): + return + + info = json.load(pid_file.open()) + return cls(project, info["port"], info["pid"]) + + def save_pid_file(self): + """Save server information to disk.""" + info = {"port": self.port, "pid": self.pid} + self.pid_file.parent.mkdir(parents=True, exist_ok=True) + with open(self.pid_file, "w") as f: + json.dump(info, f) + + def delete_pid_file(self): + """Delete server information from disk.""" + with contextlib.suppress(FileNotFoundError): + os.remove(self.pid_file) + + def load_pid_file(self) -> Union[dict, None]: + """Load server information from disk if it exists.""" + try: + with open(self.pid_file) as f: + return json.load(f) + except (FileNotFoundError, json.JSONDecodeError): + return None def run_server( @@ -121,12 +183,11 @@ def cleanup_server(project: Project, timeout: float = 5.0) -> bool: True if the server was successfully terminated, False if timeout occurred or server wasn't running. """ - info = load_server_info(project) - if info is None: + if project._server_info is None: return False try: - process = psutil.Process(info["pid"]) + process = psutil.Process(project._server_info.pid) process.terminate() try: @@ -141,15 +202,15 @@ def cleanup_server(project: Project, timeout: float = 5.0) -> bool: console.rule("[bold cyan]skore-UI[/bold cyan]") status = "gracefully" if success else "forcefully" console.print( - f"Server that was running at http://localhost:{info['port']} has " - f"been closed {status}" + f"Server that was running at http://localhost:{project._server_info.port} " + f"has been closed {status}" ) except psutil.NoSuchProcess: pass # Process already terminated finally: - with contextlib.suppress(FileNotFoundError): - os.remove(get_server_info_path(project)) + project._server_info.delete_pid_file() + project._server_info = None return True @@ -178,14 +239,14 @@ def _launch( if port is None: port = find_free_port() - info = load_server_info(project) - if info is not None: + if project._server_info is not None: try: - os.kill(info["pid"], 0) + os.kill(project._server_info.pid, 0) console.rule("[bold cyan]skore-UI[/bold cyan]") console.print( - f"Server is already running at http://localhost:{info['port']}" + "Server is already running at " + f"http://localhost:{project._server_info.port}" ) return except ProcessLookupError: # zombie process @@ -200,7 +261,8 @@ def _launch( daemon=True, ) process.start() - save_server_info(project, port, process.pid) + project._server_info = ServerInfo(project, port, process.pid) + project._server_info.save_pid_file() ready_event.wait() # wait for server to been started console.rule("[bold cyan]skore-UI[/bold cyan]") diff --git a/skore/src/skore/project/_load.py b/skore/src/skore/project/_load.py index d8f1c5a57..3707d17f9 100644 --- a/skore/src/skore/project/_load.py +++ b/skore/src/skore/project/_load.py @@ -9,6 +9,7 @@ DirectoryDoesNotExist, DiskCacheStorage, ) +from skore.project._launch import ServerInfo from skore.project.project import Project @@ -47,6 +48,7 @@ def _load(project_name: Union[str, Path]) -> Project: view_repository = ViewRepository(storage=view_storage) project = Project( name=path.name, + path=path, item_repository=item_repository, view_repository=view_repository, ) @@ -58,4 +60,8 @@ def _load(project_name: Union[str, Path]) -> Project: "Consider re-creating the project." ) from e + server_info = ServerInfo.rejoin(project) + if server_info is not None: + project._server_info = server_info + return project diff --git a/skore/src/skore/project/project.py b/skore/src/skore/project/project.py index 543e16e64..e9031a5af 100644 --- a/skore/src/skore/project/project.py +++ b/skore/src/skore/project/project.py @@ -4,6 +4,7 @@ import logging from collections.abc import Iterator +from pathlib import Path from typing import TYPE_CHECKING, Any, Literal, Optional, Union from skore.persistence.item import item_to_object, object_to_item @@ -35,15 +36,17 @@ class Project: :func:`~skore.Project.put`. """ - _server_manager = None + _server_info = None def __init__( self, name: str, + path: Union[Path, str, None], item_repository: ItemRepository, view_repository: ViewRepository, ): self.name = name + self.path = path self.item_repository = item_repository self.view_repository = view_repository @@ -280,11 +283,13 @@ def delete_note(self, key: str, *, version=-1): def shutdown_web_ui(self): """Shutdown the web UI server if it is running.""" - from skore.project._launch import cleanup_server - - if not cleanup_server(self): + if self._server_info is None: raise RuntimeError("UI server is not running") + from skore.project._launch import cleanup_server # avoid circular import + + cleanup_server(self) + def clear(self): """Delete all the contents of the project.""" # delete all the items diff --git a/skore/tests/conftest.py b/skore/tests/conftest.py index dd5a6df88..9867c6452 100644 --- a/skore/tests/conftest.py +++ b/skore/tests/conftest.py @@ -42,6 +42,7 @@ def in_memory_project(): view_repository = ViewRepository(storage=InMemoryStorage()) return Project( name="in_memory_project.skore", + path=None, item_repository=item_repository, view_repository=view_repository, ) From 154d342da9cd9a0be9cc8fff98dfbc8b3d1f625d Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 16:21:33 +0100 Subject: [PATCH 24/57] iter --- skore/pyproject.toml | 2 +- skore/src/skore/project/_launch.py | 4 +- skore/tests/unit/cli/test_cli.py | 4 + skore/tests/unit/project/test_launch.py | 116 +++++++++++++++++++++++- 4 files changed, 121 insertions(+), 5 deletions(-) diff --git a/skore/pyproject.toml b/skore/pyproject.toml index fcf0ae3f0..657fad7be 100644 --- a/skore/pyproject.toml +++ b/skore/pyproject.toml @@ -121,7 +121,7 @@ doctest_optionflags = ["ELLIPSIS", "NORMALIZE_WHITESPACE"] [tool.coverage.run] branch = true source = ["skore"] -concurrency = ["thread"] +concurrency = ["thread", "multiprocessing"] parallel = true [tool.coverage.report] diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 3ca998ace..946e6f6e0 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -53,7 +53,7 @@ def find_free_port(min_port: int = 22140, max_attempts: int = 100) -> int: continue raise RuntimeError( - f"Could not find free port after {max_attempts}" + f"Could not find free port after {max_attempts} " f"attempts starting from {min_port}" ) @@ -160,7 +160,7 @@ def run_server( async def lifespan(app: FastAPI): ready_event.set() # Signal that the server is ready if open_browser: - webbrowser.open(f"http://localhost:{port}") + webbrowser.open(f"http://localhost:{port}") # pragma: no cover yield app = create_app(project=project, lifespan=lifespan) diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 4392b4b6d..984786ebb 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -22,7 +22,11 @@ def close_project(path): from skore import open project = open(path, serve=False) + assert project._server_info is not None + pid_file = project._server_info.pid_file + assert pid_file.exists() project.shutdown_web_ui() + assert not pid_file.exists() def test_cli_launch(tmp_project_path): diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 20ecd12cb..6eb77cef1 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -1,7 +1,11 @@ +import os import socket +import uuid +import psutil +import pytest from skore.project._create import _create -from skore.project._launch import _launch, find_free_port +from skore.project._launch import ServerInfo, _launch, cleanup_server, find_free_port def test_find_free_port(): @@ -15,15 +19,46 @@ def test_find_free_port(): try: sock.bind(("", port)) sock.listen(1) + + err_msg = "Could not find free port after 1 attempts starting" + with pytest.raises(RuntimeError, match=err_msg): + find_free_port(max_attempts=1) finally: sock.close() +def test_server_info_in_memory_project(in_memory_project): + """Check the generation of the PID file path for an in-memory project.""" + server_info = ServerInfo(in_memory_project, port=8000, pid=1234) + assert server_info.port == 8000 + assert server_info.pid == 1234 + assert server_info.pid_file.name.startswith("skore-server-") + + +def test_server_info(on_disk_project): + """Check the ServerInfo class behaviour.""" + server_info = ServerInfo(on_disk_project, port=8000, pid=1234) + server_info.save_pid_file() + assert server_info.pid_file.exists() + assert server_info.load_pid_file() == {"port": 8000, "pid": 1234} + + server_info.delete_pid_file() + assert not server_info.pid_file.exists() + assert server_info.load_pid_file() is None + + def test_launch(capsys, tmp_path): """Check the general behaviour of the launch function.""" skore_project = _create(tmp_path / "test_project") - _launch(skore_project, port=8000, open_browser=False, verbose=True) + _launch(skore_project, open_browser=False, verbose=True) assert "Running skore UI" in capsys.readouterr().out + assert skore_project._server_info is not None + + server_info = skore_project._server_info + pid_file_content = server_info.load_pid_file() + assert server_info.port == pid_file_content["port"] + project_identifier = uuid.uuid3(uuid.NAMESPACE_DNS, str(skore_project.path)) + assert server_info.pid_file.name == f"skore-server-{project_identifier}.json" skore_project.shutdown_web_ui() output = capsys.readouterr().out @@ -32,3 +67,80 @@ def test_launch(capsys, tmp_path): _launch(skore_project, port=8000, open_browser=False, verbose=True) _launch(skore_project, port=8000, open_browser=False, verbose=True) assert "Server is already running" in capsys.readouterr().out + + +def test_cleanup_server_not_running(tmp_path): + """Check that cleanup does not fail when the server is not running.""" + skore_project = _create(tmp_path / "test_project") + cleanup_server(skore_project) + + +def test_cleanup_server_timeout(tmp_path, monkeypatch): + """Test cleanup_server when process termination times out.""" + skore_project = _create(tmp_path / "test_project") + + class MockProcess: + def __init__(self, pid): + self.terminate_called = False + self.kill_called = False + + def terminate(self): + self.terminate_called = True + + def wait(self, timeout): + raise psutil.TimeoutExpired(1, timeout) + + def kill(self): + self.kill_called = True + + mock_process = MockProcess(1234) + monkeypatch.setattr(psutil, "Process", lambda pid: mock_process) + + server_info = ServerInfo(skore_project, port=8000, pid=1234) + skore_project._server_info = server_info + server_info.save_pid_file() + + cleanup_server(skore_project) + + assert mock_process.terminate_called + assert mock_process.kill_called + assert not server_info.pid_file.exists() + assert skore_project._server_info is None + + +def test_cleanup_server_no_process(tmp_path, monkeypatch): + """Test cleanup_server when the process no longer exists.""" + skore_project = _create(tmp_path / "test_project") + + def mock_process_init(pid): + raise psutil.NoSuchProcess(pid) + + monkeypatch.setattr(psutil, "Process", mock_process_init) + + server_info = ServerInfo(skore_project, port=8000, pid=1234) + skore_project._server_info = server_info + server_info.save_pid_file() + + cleanup_server(skore_project) + + assert not server_info.pid_file.exists() + assert skore_project._server_info is None + + +def test_launch_zombie_process(tmp_path, monkeypatch): + """Test launch handling when encountering a zombie process.""" + skore_project = _create(tmp_path / "test_project") + server_info = ServerInfo(skore_project, port=8000, pid=1234) + skore_project._server_info = server_info + server_info.save_pid_file() + + def mock_kill(pid, signal): + raise ProcessLookupError() + + monkeypatch.setattr(os, "kill", mock_kill) + + _launch(skore_project, port=8001, open_browser=False, verbose=True) + + assert skore_project._server_info is not None + assert skore_project._server_info.port == 8001 + assert skore_project._server_info.pid != 1234 From b3384201a242140b44a0a2cbf1c8ea2c068385a7 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 16:48:37 +0100 Subject: [PATCH 25/57] iter --- skore/tests/unit/cli/test_cli.py | 6 ++---- skore/tests/unit/project/test_launch.py | 8 ++++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 984786ebb..404ca64fd 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -35,8 +35,6 @@ def test_cli_launch(tmp_project_path): [ "launch", str(tmp_project_path), - "--port", - "8000", "--no-open-browser", "--verbose", ] @@ -85,9 +83,9 @@ def test_cli_open(tmp_path, monkeypatch): assert (project_path / "items").exists() assert (project_path / "views").exists() - cli(["open", "test_project", "--port", "8000", "--verbose"]) + cli(["open", "test_project", "--verbose"]) close_project(project_path) - cli(["open", "test_project", "--no-create", "--port", "8001"]) + cli(["open", "test_project", "--no-create"]) close_project(project_path) with pytest.raises(FileNotFoundError): diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 6eb77cef1..a79706ed4 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -29,18 +29,18 @@ def test_find_free_port(): def test_server_info_in_memory_project(in_memory_project): """Check the generation of the PID file path for an in-memory project.""" - server_info = ServerInfo(in_memory_project, port=8000, pid=1234) - assert server_info.port == 8000 + server_info = ServerInfo(in_memory_project, port=20000, pid=1234) + assert server_info.port == 20000 assert server_info.pid == 1234 assert server_info.pid_file.name.startswith("skore-server-") def test_server_info(on_disk_project): """Check the ServerInfo class behaviour.""" - server_info = ServerInfo(on_disk_project, port=8000, pid=1234) + server_info = ServerInfo(on_disk_project, port=30000, pid=1234) server_info.save_pid_file() assert server_info.pid_file.exists() - assert server_info.load_pid_file() == {"port": 8000, "pid": 1234} + assert server_info.load_pid_file() == {"port": 30000, "pid": 1234} server_info.delete_pid_file() assert not server_info.pid_file.exists() From a47511078dc333f838c2496909bfa48b474ea7dd Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 17:23:13 +0100 Subject: [PATCH 26/57] iter --- skore/tests/unit/cli/test_cli.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 404ca64fd..1fdf08013 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -77,15 +77,15 @@ def test_cli_open(tmp_path, monkeypatch): os.chdir(tmp_path) - cli(["open", "test_project", "--no-serve"]) - project_path = tmp_path / "test_project.skore" + cli(["open", "project_cli_open", "--no-serve"]) + project_path = tmp_path / "project_cli_open.skore" assert project_path.exists() assert (project_path / "items").exists() assert (project_path / "views").exists() - cli(["open", "test_project", "--verbose"]) + cli(["open", "project_cli_open", "--verbose"]) close_project(project_path) - cli(["open", "test_project", "--no-create"]) + cli(["open", "project_cli_open", "--no-create"]) close_project(project_path) with pytest.raises(FileNotFoundError): From 06561b60ed5287301df3244b725cd2c939372b00 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 17:34:45 +0100 Subject: [PATCH 27/57] debug --- skore/src/skore/project/_launch.py | 2 ++ skore/src/skore/project/_load.py | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 946e6f6e0..440de631c 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -112,7 +112,9 @@ def rejoin(cls, project: Project): The server information. """ pid_file = cls._get_pid_file_path(project) + print(pid_file) if not pid_file.exists(): + print("XXXX rejoin failed") return info = json.load(pid_file.open()) diff --git a/skore/src/skore/project/_load.py b/skore/src/skore/project/_load.py index 3707d17f9..cffdd47b3 100644 --- a/skore/src/skore/project/_load.py +++ b/skore/src/skore/project/_load.py @@ -32,6 +32,8 @@ def _load(project_name: Union[str, Path]) -> Project: Project The loaded Project instance. """ + from skore import console # avoid circular import + path = Path(project_name).resolve() if path.suffix != ".skore": @@ -63,5 +65,9 @@ def _load(project_name: Union[str, Path]) -> Project: server_info = ServerInfo.rejoin(project) if server_info is not None: project._server_info = server_info + console.print( + f"Project '{project.name}' rejoined skore UI at URL: " + f"http://localhost:{server_info.port}" + ) return project From cd04ecf05477401e74f779bb0c65080f0c7de9a3 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 17:41:15 +0100 Subject: [PATCH 28/57] debug --- skore/src/skore/project/_launch.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 440de631c..1f8c09b64 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -11,7 +11,6 @@ from pathlib import Path from typing import Union -import platformdirs import psutil import uvicorn from fastapi import FastAPI @@ -88,8 +87,8 @@ def _get_pid_file_path(project: Project) -> Path: else: project_identifier = uuid.uuid3(uuid.NAMESPACE_DNS, str(project.name)) return ( - platformdirs.user_state_path(appname="skore") - / f"skore-server-{project_identifier}.json" + # platformdirs.user_state_path(appname="skore") + Path.home() / ".skore" / f"skore-server-{project_identifier}.json" ) def __init__(self, project: Project, port: int, pid: int): From 7c7d0fbc7200e7d43fd246c3c7756c35e8d5de8c Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 17:48:28 +0100 Subject: [PATCH 29/57] debug --- .github/workflows/backend.yml | 4 ++-- skore/src/skore/project/_launch.py | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/backend.yml b/.github/workflows/backend.yml index a683a86ad..994873671 100644 --- a/.github/workflows/backend.yml +++ b/.github/workflows/backend.yml @@ -138,7 +138,7 @@ jobs: if: ${{ ! matrix.coverage }} timeout-minutes: 10 working-directory: skore/ - run: python -m pytest -n auto src/ tests/ --no-cov + run: python -m pytest src/ tests/ --no-cov - name: Test with coverage if: ${{ matrix.coverage }} @@ -146,7 +146,7 @@ jobs: working-directory: skore/ run: | mkdir coverage - python -m pytest -n auto src/ tests/ --junitxml=coverage/coverage.xml --cov-config=pyproject.toml --cov | tee coverage/coverage.txt + python -m pytest src/ tests/ --junitxml=coverage/coverage.xml --cov-config=pyproject.toml --cov | tee coverage/coverage.txt - name: Upload coverage reports if: ${{ matrix.coverage && (github.event_name == 'pull_request') }} diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 1f8c09b64..440de631c 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -11,6 +11,7 @@ from pathlib import Path from typing import Union +import platformdirs import psutil import uvicorn from fastapi import FastAPI @@ -87,8 +88,8 @@ def _get_pid_file_path(project: Project) -> Path: else: project_identifier = uuid.uuid3(uuid.NAMESPACE_DNS, str(project.name)) return ( - # platformdirs.user_state_path(appname="skore") - Path.home() / ".skore" / f"skore-server-{project_identifier}.json" + platformdirs.user_state_path(appname="skore") + / f"skore-server-{project_identifier}.json" ) def __init__(self, project: Project, port: int, pid: int): From b861d58dc9dec4462790ffc39bc385a71426ab6b Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 17:53:05 +0100 Subject: [PATCH 30/57] debug --- .github/workflows/backend.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/backend.yml b/.github/workflows/backend.yml index 994873671..9cd9b62a5 100644 --- a/.github/workflows/backend.yml +++ b/.github/workflows/backend.yml @@ -138,7 +138,7 @@ jobs: if: ${{ ! matrix.coverage }} timeout-minutes: 10 working-directory: skore/ - run: python -m pytest src/ tests/ --no-cov + run: python -m pytest -vsl src/ tests/ --no-cov - name: Test with coverage if: ${{ matrix.coverage }} @@ -146,7 +146,7 @@ jobs: working-directory: skore/ run: | mkdir coverage - python -m pytest src/ tests/ --junitxml=coverage/coverage.xml --cov-config=pyproject.toml --cov | tee coverage/coverage.txt + python -m pytest -vsl src/ tests/ --junitxml=coverage/coverage.xml --cov-config=pyproject.toml --cov | tee coverage/coverage.txt - name: Upload coverage reports if: ${{ matrix.coverage && (github.event_name == 'pull_request') }} From 0b09039f6b7a49b1d6586d88b6d9fb2738e33005 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 18:10:26 +0100 Subject: [PATCH 31/57] debug --- skore/src/skore/project/_launch.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 440de631c..f1e494ec0 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -87,6 +87,15 @@ def _get_pid_file_path(project: Project) -> Path: project_identifier = uuid.uuid3(uuid.NAMESPACE_DNS, str(project.path)) else: project_identifier = uuid.uuid3(uuid.NAMESPACE_DNS, str(project.name)) + + # Print the content of the skore state directory + state_dir = platformdirs.user_state_path(appname="skore") + if state_dir.exists(): + print(f"Content of {state_dir}:") + for path in state_dir.iterdir(): + print(f" {path.name}") + else: + print(f"Directory {state_dir} does not exist") return ( platformdirs.user_state_path(appname="skore") / f"skore-server-{project_identifier}.json" @@ -114,7 +123,6 @@ def rejoin(cls, project: Project): pid_file = cls._get_pid_file_path(project) print(pid_file) if not pid_file.exists(): - print("XXXX rejoin failed") return info = json.load(pid_file.open()) From a6b0978073ff638334df230692025e03d5260ef5 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 18:21:24 +0100 Subject: [PATCH 32/57] debug --- skore/src/skore/project/_launch.py | 17 ++++------------- skore/tests/unit/project/test_launch.py | 4 ++-- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index f1e494ec0..ebe819da4 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -6,11 +6,11 @@ import multiprocessing import os import socket -import uuid import webbrowser from pathlib import Path from typing import Union +import joblib import platformdirs import psutil import uvicorn @@ -84,18 +84,10 @@ class ServerInfo: def _get_pid_file_path(project: Project) -> Path: """Get the path to the PID file.""" if project.path is not None: - project_identifier = uuid.uuid3(uuid.NAMESPACE_DNS, str(project.path)) + project_identifier = joblib.hash(project.path, hash_name="sha1") else: - project_identifier = uuid.uuid3(uuid.NAMESPACE_DNS, str(project.name)) - - # Print the content of the skore state directory - state_dir = platformdirs.user_state_path(appname="skore") - if state_dir.exists(): - print(f"Content of {state_dir}:") - for path in state_dir.iterdir(): - print(f" {path.name}") - else: - print(f"Directory {state_dir} does not exist") + project_identifier = joblib.hash(project.name, hash_name="sha1") + return ( platformdirs.user_state_path(appname="skore") / f"skore-server-{project_identifier}.json" @@ -121,7 +113,6 @@ def rejoin(cls, project: Project): The server information. """ pid_file = cls._get_pid_file_path(project) - print(pid_file) if not pid_file.exists(): return diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index a79706ed4..065551bff 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -1,7 +1,7 @@ import os import socket -import uuid +import joblib import psutil import pytest from skore.project._create import _create @@ -57,7 +57,7 @@ def test_launch(capsys, tmp_path): server_info = skore_project._server_info pid_file_content = server_info.load_pid_file() assert server_info.port == pid_file_content["port"] - project_identifier = uuid.uuid3(uuid.NAMESPACE_DNS, str(skore_project.path)) + project_identifier = joblib.hash(skore_project.path, hash_name="sha1") assert server_info.pid_file.name == f"skore-server-{project_identifier}.json" skore_project.shutdown_web_ui() From 4bdaf58e2dbe962eb18dc46b74c1da90c1fd35a4 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 18:22:16 +0100 Subject: [PATCH 33/57] revert ci --- .github/workflows/backend.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/backend.yml b/.github/workflows/backend.yml index 9cd9b62a5..a683a86ad 100644 --- a/.github/workflows/backend.yml +++ b/.github/workflows/backend.yml @@ -138,7 +138,7 @@ jobs: if: ${{ ! matrix.coverage }} timeout-minutes: 10 working-directory: skore/ - run: python -m pytest -vsl src/ tests/ --no-cov + run: python -m pytest -n auto src/ tests/ --no-cov - name: Test with coverage if: ${{ matrix.coverage }} @@ -146,7 +146,7 @@ jobs: working-directory: skore/ run: | mkdir coverage - python -m pytest -vsl src/ tests/ --junitxml=coverage/coverage.xml --cov-config=pyproject.toml --cov | tee coverage/coverage.txt + python -m pytest -n auto src/ tests/ --junitxml=coverage/coverage.xml --cov-config=pyproject.toml --cov | tee coverage/coverage.txt - name: Upload coverage reports if: ${{ matrix.coverage && (github.event_name == 'pull_request') }} From 3076c0bbe11dea68e0d8f9738d4d6b03f4a9fbc8 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 18:29:41 +0100 Subject: [PATCH 34/57] revert ci --- skore/src/skore/project/_launch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index ebe819da4..a509c3f6f 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -84,7 +84,7 @@ class ServerInfo: def _get_pid_file_path(project: Project) -> Path: """Get the path to the PID file.""" if project.path is not None: - project_identifier = joblib.hash(project.path, hash_name="sha1") + project_identifier = joblib.hash(str(project.path), hash_name="sha1") else: project_identifier = joblib.hash(project.name, hash_name="sha1") From c0115d5595e55e0c3e6d97f8cf4002e447433532 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 18:32:31 +0100 Subject: [PATCH 35/57] iter --- skore/tests/unit/project/test_launch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 065551bff..1e2174e40 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -57,7 +57,7 @@ def test_launch(capsys, tmp_path): server_info = skore_project._server_info pid_file_content = server_info.load_pid_file() assert server_info.port == pid_file_content["port"] - project_identifier = joblib.hash(skore_project.path, hash_name="sha1") + project_identifier = joblib.hash(str(skore_project.path), hash_name="sha1") assert server_info.pid_file.name == f"skore-server-{project_identifier}.json" skore_project.shutdown_web_ui() From 1e2d6c04418807162f69b07ba7a974a015877585 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 18:38:43 +0100 Subject: [PATCH 36/57] debug --- .github/workflows/backend.yml | 4 ++-- skore/src/skore/project/_launch.py | 14 ++++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/.github/workflows/backend.yml b/.github/workflows/backend.yml index a683a86ad..9cd9b62a5 100644 --- a/.github/workflows/backend.yml +++ b/.github/workflows/backend.yml @@ -138,7 +138,7 @@ jobs: if: ${{ ! matrix.coverage }} timeout-minutes: 10 working-directory: skore/ - run: python -m pytest -n auto src/ tests/ --no-cov + run: python -m pytest -vsl src/ tests/ --no-cov - name: Test with coverage if: ${{ matrix.coverage }} @@ -146,7 +146,7 @@ jobs: working-directory: skore/ run: | mkdir coverage - python -m pytest -n auto src/ tests/ --junitxml=coverage/coverage.xml --cov-config=pyproject.toml --cov | tee coverage/coverage.txt + python -m pytest -vsl src/ tests/ --junitxml=coverage/coverage.xml --cov-config=pyproject.toml --cov | tee coverage/coverage.txt - name: Upload coverage reports if: ${{ matrix.coverage && (github.event_name == 'pull_request') }} diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index a509c3f6f..f6482dff1 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -84,14 +84,20 @@ class ServerInfo: def _get_pid_file_path(project: Project) -> Path: """Get the path to the PID file.""" if project.path is not None: + print(str(project.path)) project_identifier = joblib.hash(str(project.path), hash_name="sha1") else: + print(project.name) project_identifier = joblib.hash(project.name, hash_name="sha1") + state_path = platformdirs.user_state_path(appname="skore") + if state_path.exists(): + print(f"Contents of {state_path}:") + for item in state_path.iterdir(): + print(f" {item.name}") + else: + print(f"Directory {state_path} does not exist") - return ( - platformdirs.user_state_path(appname="skore") - / f"skore-server-{project_identifier}.json" - ) + return state_path / f"skore-server-{project_identifier}.json" def __init__(self, project: Project, port: int, pid: int): self.port = port From 7ff4c6ec245de08dc58bd7e776c9cca501a7f393 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 18:48:41 +0100 Subject: [PATCH 37/57] debug --- skore/tests/unit/cli/test_cli.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 1fdf08013..5af315ac6 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -75,17 +75,16 @@ def test_cli_open(tmp_path, monkeypatch): lambda *args, **kwargs: _launch(*args, **{**kwargs, "open_browser": False}), ) - os.chdir(tmp_path) - - cli(["open", "project_cli_open", "--no-serve"]) project_path = tmp_path / "project_cli_open.skore" + + cli(["open", str(project_path), "--no-serve"]) assert project_path.exists() assert (project_path / "items").exists() assert (project_path / "views").exists() - cli(["open", "project_cli_open", "--verbose"]) + cli(["open", str(project_path), "--verbose"]) close_project(project_path) - cli(["open", "project_cli_open", "--no-create"]) + cli(["open", str(project_path), "--no-create"]) close_project(project_path) with pytest.raises(FileNotFoundError): From 8ac56a45776bbe9ae1a48b6eac31812b239e435e Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 18:52:09 +0100 Subject: [PATCH 38/57] iter --- .github/workflows/backend.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/backend.yml b/.github/workflows/backend.yml index 9cd9b62a5..a683a86ad 100644 --- a/.github/workflows/backend.yml +++ b/.github/workflows/backend.yml @@ -138,7 +138,7 @@ jobs: if: ${{ ! matrix.coverage }} timeout-minutes: 10 working-directory: skore/ - run: python -m pytest -vsl src/ tests/ --no-cov + run: python -m pytest -n auto src/ tests/ --no-cov - name: Test with coverage if: ${{ matrix.coverage }} @@ -146,7 +146,7 @@ jobs: working-directory: skore/ run: | mkdir coverage - python -m pytest -vsl src/ tests/ --junitxml=coverage/coverage.xml --cov-config=pyproject.toml --cov | tee coverage/coverage.txt + python -m pytest -n auto src/ tests/ --junitxml=coverage/coverage.xml --cov-config=pyproject.toml --cov | tee coverage/coverage.txt - name: Upload coverage reports if: ${{ matrix.coverage && (github.event_name == 'pull_request') }} From 15e0b21bb8972bf0e999bd6931de15f5ef59d495 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 18:52:53 +0100 Subject: [PATCH 39/57] iter --- skore/src/skore/project/_launch.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index f6482dff1..2f586eb4f 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -84,19 +84,11 @@ class ServerInfo: def _get_pid_file_path(project: Project) -> Path: """Get the path to the PID file.""" if project.path is not None: - print(str(project.path)) project_identifier = joblib.hash(str(project.path), hash_name="sha1") else: - print(project.name) project_identifier = joblib.hash(project.name, hash_name="sha1") - state_path = platformdirs.user_state_path(appname="skore") - if state_path.exists(): - print(f"Contents of {state_path}:") - for item in state_path.iterdir(): - print(f" {item.name}") - else: - print(f"Directory {state_path} does not exist") + state_path = platformdirs.user_state_path(appname="skore") return state_path / f"skore-server-{project_identifier}.json" def __init__(self, project: Project, port: int, pid: int): From f7ecb17c7837bd6516e14afd2837f818007db69e Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 19:43:30 +0100 Subject: [PATCH 40/57] iter --- skore/src/skore/project/_load.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/skore/src/skore/project/_load.py b/skore/src/skore/project/_load.py index cffdd47b3..3707d17f9 100644 --- a/skore/src/skore/project/_load.py +++ b/skore/src/skore/project/_load.py @@ -32,8 +32,6 @@ def _load(project_name: Union[str, Path]) -> Project: Project The loaded Project instance. """ - from skore import console # avoid circular import - path = Path(project_name).resolve() if path.suffix != ".skore": @@ -65,9 +63,5 @@ def _load(project_name: Union[str, Path]) -> Project: server_info = ServerInfo.rejoin(project) if server_info is not None: project._server_info = server_info - console.print( - f"Project '{project.name}' rejoined skore UI at URL: " - f"http://localhost:{server_info.port}" - ) return project From 504f1464ae675264d4c6842d6cea107552e63a96 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 23:21:46 +0100 Subject: [PATCH 41/57] iter --- skore/src/skore/cli/cli.py | 34 +++++++++++++- skore/src/skore/cli/color_format.py | 2 +- skore/src/skore/project/_launch.py | 62 +++++++++++++++++++------ skore/src/skore/project/_open.py | 9 +++- skore/src/skore/utils/_environment.py | 52 +++++++++++++++++++++ skore/tests/unit/cli/test_cli.py | 9 ++-- skore/tests/unit/cli/test_open.py | 15 +++--- skore/tests/unit/project/test_launch.py | 14 ++++-- 8 files changed, 168 insertions(+), 29 deletions(-) create mode 100644 skore/src/skore/utils/_environment.py diff --git a/skore/src/skore/cli/cli.py b/skore/src/skore/cli/cli.py index 3d59bc05a..d5bdf531e 100644 --- a/skore/src/skore/cli/cli.py +++ b/skore/src/skore/cli/cli.py @@ -6,7 +6,7 @@ from skore.cli.color_format import ColorArgumentParser from skore.project import open from skore.project._create import _create -from skore.project._launch import _launch +from skore.project._launch import _cleanup_potential_zombie_process, _launch from skore.project._load import _load @@ -45,6 +45,15 @@ def cli(args: list[str]): "project_name", help="the name or path of the project to open", ) + parser_launch.add_argument( + "--keep-alive", + action=argparse.BooleanOptionalAction, + help=( + "whether to keep the server alive once the main process finishes " + "(default: %(default)s)" + ), + default="auto", + ) parser_launch.add_argument( "--port", type=int, @@ -66,6 +75,16 @@ def cli(args: list[str]): help="increase logging verbosity", ) + # cleanup potential zombie processes + parser_cleanup = subparsers.add_parser( + "cleanup", help="Clean up all UI running processes" + ) + parser_cleanup.add_argument( + "--verbose", + action="store_true", + help="increase logging verbosity", + ) + # open a skore project parser_open = subparsers.add_parser( "open", help='Create a "project.skore" file and start the UI' @@ -93,6 +112,15 @@ def cli(args: list[str]): help=("whether to serve the project (default: %(default)s)"), default=True, ) + parser_open.add_argument( + "--keep-alive", + action=argparse.BooleanOptionalAction, + help=( + "whether to keep the server alive once the main process finishes " + "(default: %(default)s)" + ), + default="auto", + ) parser_open.add_argument( "--port", type=int, @@ -116,6 +144,7 @@ def cli(args: list[str]): elif parsed_args.subcommand == "launch": _launch( project=_load(project_name=parsed_args.project_name), + keep_alive=parsed_args.keep_alive, port=parsed_args.port, open_browser=parsed_args.open_browser, verbose=parsed_args.verbose, @@ -126,8 +155,11 @@ def cli(args: list[str]): create=parsed_args.create, overwrite=parsed_args.overwrite, serve=parsed_args.serve, + keep_alive=parsed_args.keep_alive, port=parsed_args.port, verbose=parsed_args.verbose, ) + elif parsed_args.subcommand == "cleanup": + _cleanup_potential_zombie_process() else: parser.print_help() diff --git a/skore/src/skore/cli/color_format.py b/skore/src/skore/cli/color_format.py index 6a3e3a960..eeba9994c 100644 --- a/skore/src/skore/cli/color_format.py +++ b/skore/src/skore/cli/color_format.py @@ -79,7 +79,7 @@ def format_help(self): # Color the subcommands in cyan help_text = re.sub( - r"(?<=\s)(launch|create|open)(?=\s+)", + r"(?<=\s)(launch|create|open|cleanup)(?=\s+)", r"[cyan bold]\1[/cyan bold]", help_text, ) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 2f586eb4f..40c60f4e3 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -17,6 +17,7 @@ from fastapi import FastAPI from skore.project.project import Project, logger +from skore.utils._environment import is_environment_notebook_like from skore.utils._logger import logger_context @@ -214,8 +215,27 @@ def cleanup_server(project: Project, timeout: float = 5.0) -> bool: return True +def _cleanup_potential_zombie_process(): + state_path = platformdirs.user_state_path(appname="skore") + for pid_file in state_path.glob("skore-server-*.json"): + try: + pid_info = json.load(pid_file.open()) + try: + process = psutil.Process(pid_info["pid"]) + process.terminate() + try: + process.wait(timeout=5.0) + except psutil.TimeoutExpired: + process.kill() + except psutil.NoSuchProcess: + pass + finally: + pid_file.unlink() + + def _launch( project: Project, + keep_alive: Union[str, bool] = "auto", port: Union[int, None] = None, open_browser: bool = True, verbose: bool = False, @@ -224,14 +244,20 @@ def _launch( Parameters ---------- - project : Project - The project to launch. - port : int, optional - The port to use for the server, by default None. - open_browser : bool, optional - Whether to open the browser, by default True. - verbose : bool, optional - Whether to print verbose output, by default False. + project : Project + The project to launch. + keep_alive : Union[str, bool], default="auto" + Whether to keep the server alive once the main process finishes. When False, + the server will be terminated when the main process finishes. When True, + the server will be kept alive and thus block the main process from exiting. + When `"auto"`, the server will be kept alive if the current execution context + is not a notebook-like environment. + port : int, optional + The port to use for the server, by default None. + open_browser : bool, optional + Whether to open the browser, by default True. + verbose : bool, optional + Whether to print verbose output, by default False. """ from skore import console # avoid circular import @@ -253,11 +279,13 @@ def _launch( ready_event = multiprocessing.Event() + daemon = is_environment_notebook_like() if keep_alive == "auto" else not keep_alive + with logger_context(logger, verbose): process = multiprocessing.Process( target=run_server, args=(project, port, open_browser, ready_event), - daemon=True, + daemon=daemon, ) process.start() project._server_info = ServerInfo(project, port, process.pid) @@ -265,8 +293,16 @@ def _launch( ready_event.wait() # wait for server to been started console.rule("[bold cyan]skore-UI[/bold cyan]") - console.print( - f"Running skore UI from '{project.name}' at URL http://localhost:{port}" - ) - atexit.register(cleanup_server, project) + msg = f"Running skore UI from '{project.name}' at URL http://localhost:{port}" + if not daemon: + msg += " (Press CTRL+C to quit)" + console.print(msg) + + if not daemon: + try: + process.join() + except KeyboardInterrupt: + cleanup_server(project) + else: + atexit.register(cleanup_server, project) diff --git a/skore/src/skore/project/_open.py b/skore/src/skore/project/_open.py index 931fff794..3bd14ba19 100644 --- a/skore/src/skore/project/_open.py +++ b/skore/src/skore/project/_open.py @@ -12,6 +12,7 @@ def open( create: bool = True, overwrite: bool = False, serve: bool = True, + keep_alive: Union[str, bool] = "auto", port: Union[int, None] = None, verbose: bool = False, ) -> Project: @@ -34,6 +35,12 @@ def open( Has no effect otherwise. serve: bool, default=True Whether to launch the skore UI. + keep_alive : Union[str, bool], default="auto" + Whether to keep the server alive once the main process finishes. When False, + the server will be terminated when the main process finishes. When True, + the server will be kept alive and thus block the main process from exiting. + When `"auto"`, the server will be kept alive if the current execution context + is not a notebook-like environment. port: int, default=None Port at which to bind the UI server. If ``None``, the server will be bound to an available port. @@ -68,6 +75,6 @@ def open( raise if serve: - _launch(project, port=port, verbose=verbose) + _launch(project, keep_alive=keep_alive, port=port, verbose=verbose) return project diff --git a/skore/src/skore/utils/_environment.py b/skore/src/skore/utils/_environment.py new file mode 100644 index 000000000..cd91d72fd --- /dev/null +++ b/skore/src/skore/utils/_environment.py @@ -0,0 +1,52 @@ +import os +import sys + + +def get_environment_info(): + """Detect the current Python execution environment. + + Returns a dictionary with information about the environment. + """ + env_info = { + "is_jupyter": False, + "is_vscode": False, + "is_interactive": False, + "environment_name": "standard_python", + "details": {}, + } + + # Check for interactive mode + env_info["is_interactive"] = hasattr(sys, "ps1") + + # Check for Jupyter + try: + shell = get_ipython().__class__.__name__ # type: ignore + env_info["details"]["ipython_shell"] = shell + + if shell == "ZMQInteractiveShell": # Jupyter notebook/lab + env_info["is_jupyter"] = True + env_info["environment_name"] = "jupyter" + elif shell == "TerminalInteractiveShell": # IPython terminal + env_info["environment_name"] = "ipython_terminal" + except NameError: + pass + + # Check for VSCode + if "VSCODE_PID" in os.environ: + env_info["is_vscode"] = True + if env_info["is_interactive"]: + env_info["environment_name"] = "vscode_interactive" + else: + env_info["environment_name"] = "vscode_script" + + # Add additional environment details + env_info["details"]["python_executable"] = sys.executable + env_info["details"]["python_version"] = sys.version + + return env_info + + +def is_environment_notebook_like() -> bool: + """Return `True` if the execution context dcan render HTML. `False` otherwise.""" + info = get_environment_info() + return info["is_vscode"] or info["is_jupyter"] diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 5af315ac6..94d6f9465 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -35,6 +35,7 @@ def test_cli_launch(tmp_project_path): [ "launch", str(tmp_project_path), + "--no-keep-alive", "--no-open-browser", "--verbose", ] @@ -44,7 +45,9 @@ def test_cli_launch(tmp_project_path): def test_cli_launch_no_project_name(): with pytest.raises(SystemExit): - cli(["launch", "--port", 0, "--no-open-browser", "--verbose"]) + cli( + ["launch", "--port", 0, "--no-keep-alive", "--no-open-browser", "--verbose"] + ) def test_cli_create(tmp_path): @@ -82,9 +85,9 @@ def test_cli_open(tmp_path, monkeypatch): assert (project_path / "items").exists() assert (project_path / "views").exists() - cli(["open", str(project_path), "--verbose"]) + cli(["open", str(project_path), "--no-keep-alive", "--verbose"]) close_project(project_path) - cli(["open", str(project_path), "--no-create"]) + cli(["open", str(project_path), "--no-keep-alive", "--no-create"]) close_project(project_path) with pytest.raises(FileNotFoundError): diff --git a/skore/tests/unit/cli/test_open.py b/skore/tests/unit/cli/test_open.py index b67a4ad19..ed74b99c1 100644 --- a/skore/tests/unit/cli/test_open.py +++ b/skore/tests/unit/cli/test_open.py @@ -20,8 +20,10 @@ def mock_launch(monkeypatch): """Fixture that mocks the _launch function and tracks calls to it.""" calls = [] - def _mock_launch(project, port=None, open_browser=True, verbose=False): - calls.append((project, port, open_browser, verbose)) + def _mock_launch( + project, keep_alive="auto", port=None, open_browser=True, verbose=False + ): + calls.append((project, keep_alive, port, open_browser, verbose)) monkeypatch.setattr("skore.project._launch._launch", _mock_launch) return calls @@ -38,6 +40,7 @@ def test_cli_open(tmp_project_path, mock_launch): "--port", "8000", "--serve", + "--no-keep-alive", "--verbose", ] ) @@ -49,7 +52,7 @@ def test_cli_open_creates_project(tmp_path, mock_launch): project_path = tmp_path / "new_project.skore" assert not project_path.exists() - cli(["open", str(project_path), "--create"]) + cli(["open", str(project_path), "--create", "--no-keep-alive"]) assert project_path.exists() assert len(mock_launch) == 1 @@ -59,7 +62,7 @@ def test_cli_open_no_create_fails(tmp_path, mock_launch): project_path = tmp_path / "nonexistent.skore" with pytest.raises(FileNotFoundError): - cli(["open", str(project_path), "--no-create"]) + cli(["open", str(project_path), "--no-create", "--no-keep-alive"]) assert len(mock_launch) == 0 @@ -67,10 +70,10 @@ def test_cli_open_overwrite(tmp_path, mock_launch): """Test that CLI open can overwrite existing project.""" project_path = tmp_path / "overwrite_test.skore" - cli(["open", str(project_path), "--create"]) + cli(["open", str(project_path), "--create", "--no-keep-alive"]) initial_time = os.path.getmtime(project_path) - cli(["open", str(project_path), "--create", "--overwrite"]) + cli(["open", str(project_path), "--create", "--overwrite", "--no-keep-alive"]) new_time = os.path.getmtime(project_path) assert new_time > initial_time assert len(mock_launch) == 2 diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 1e2174e40..18229656a 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -50,7 +50,7 @@ def test_server_info(on_disk_project): def test_launch(capsys, tmp_path): """Check the general behaviour of the launch function.""" skore_project = _create(tmp_path / "test_project") - _launch(skore_project, open_browser=False, verbose=True) + _launch(skore_project, open_browser=False, keep_alive=False, verbose=True) assert "Running skore UI" in capsys.readouterr().out assert skore_project._server_info is not None @@ -64,8 +64,12 @@ def test_launch(capsys, tmp_path): output = capsys.readouterr().out assert "Server that was running" in output - _launch(skore_project, port=8000, open_browser=False, verbose=True) - _launch(skore_project, port=8000, open_browser=False, verbose=True) + _launch( + skore_project, port=8000, open_browser=False, keep_alive=False, verbose=True + ) + _launch( + skore_project, port=8000, open_browser=False, keep_alive=False, verbose=True + ) assert "Server is already running" in capsys.readouterr().out @@ -139,7 +143,9 @@ def mock_kill(pid, signal): monkeypatch.setattr(os, "kill", mock_kill) - _launch(skore_project, port=8001, open_browser=False, verbose=True) + _launch( + skore_project, port=8001, open_browser=False, keep_alive=False, verbose=True + ) assert skore_project._server_info is not None assert skore_project._server_info.port == 8001 From f104af69e0b6b4b2011b117f29c1eb86a230188f Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Sat, 25 Jan 2025 23:36:06 +0100 Subject: [PATCH 42/57] iter --- skore/src/skore/cli/cli.py | 14 ++++++-------- skore/src/skore/cli/color_format.py | 2 +- skore/src/skore/project/_launch.py | 2 +- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/skore/src/skore/cli/cli.py b/skore/src/skore/cli/cli.py index d5bdf531e..90753cb12 100644 --- a/skore/src/skore/cli/cli.py +++ b/skore/src/skore/cli/cli.py @@ -6,7 +6,7 @@ from skore.cli.color_format import ColorArgumentParser from skore.project import open from skore.project._create import _create -from skore.project._launch import _cleanup_potential_zombie_process, _launch +from skore.project._launch import _kill_all_servers, _launch from skore.project._load import _load @@ -75,11 +75,9 @@ def cli(args: list[str]): help="increase logging verbosity", ) - # cleanup potential zombie processes - parser_cleanup = subparsers.add_parser( - "cleanup", help="Clean up all UI running processes" - ) - parser_cleanup.add_argument( + # kill all UI servers + parser_kill = subparsers.add_parser("kill", help="Kill all UI servers") + parser_kill.add_argument( "--verbose", action="store_true", help="increase logging verbosity", @@ -159,7 +157,7 @@ def cli(args: list[str]): port=parsed_args.port, verbose=parsed_args.verbose, ) - elif parsed_args.subcommand == "cleanup": - _cleanup_potential_zombie_process() + elif parsed_args.subcommand == "kill": + _kill_all_servers() else: parser.print_help() diff --git a/skore/src/skore/cli/color_format.py b/skore/src/skore/cli/color_format.py index eeba9994c..7ecbde5d9 100644 --- a/skore/src/skore/cli/color_format.py +++ b/skore/src/skore/cli/color_format.py @@ -79,7 +79,7 @@ def format_help(self): # Color the subcommands in cyan help_text = re.sub( - r"(?<=\s)(launch|create|open|cleanup)(?=\s+)", + r"(?<=\s)(launch|create|open|kill)(?=\s+)", r"[cyan bold]\1[/cyan bold]", help_text, ) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 40c60f4e3..586e8d885 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -215,7 +215,7 @@ def cleanup_server(project: Project, timeout: float = 5.0) -> bool: return True -def _cleanup_potential_zombie_process(): +def _kill_all_servers(): state_path = platformdirs.user_state_path(appname="skore") for pid_file in state_path.glob("skore-server-*.json"): try: From fb76b819560ab235531aa6c54f31b4b6fb2034d1 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Mon, 27 Jan 2025 22:36:19 +0100 Subject: [PATCH 43/57] iter --- skore/src/skore/project/_launch.py | 140 +++++++++++++++++++---------- skore/src/skore/ui/server.py | 34 +++++++ 2 files changed, 129 insertions(+), 45 deletions(-) create mode 100644 skore/src/skore/ui/server.py diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 586e8d885..c05fa895b 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -3,9 +3,11 @@ import atexit import contextlib import json -import multiprocessing import os import socket +import subprocess +import sys +import time import webbrowser from pathlib import Path from typing import Union @@ -13,8 +15,6 @@ import joblib import platformdirs import psutil -import uvicorn -from fastapi import FastAPI from skore.project.project import Project, logger from skore.utils._environment import is_environment_notebook_like @@ -139,32 +139,76 @@ def load_pid_file(self) -> Union[dict, None]: return None -def run_server( - project: Project, port: int, open_browser: bool, ready_event: multiprocessing.Event -): - """Run the server in a separate process. +def is_server_started(port: int, timeout: float = 10.0, interval: float = 0.1) -> bool: + """Wait for server to be ready by attempting to connect to the port. + + Parameters + ---------- + port : int + The port to check. + timeout : float, default=10.0 + Maximum time to wait in seconds. + interval : float, default=0.1 + Time between connection attempts in seconds. + + Returns + ------- + bool + True if server is ready, False if timeout occurred. + """ + start_time = time.time() + while time.time() - start_time < timeout: + try: + with socket.create_connection(("localhost", port), timeout=interval): + return True + except (socket.timeout, ConnectionRefusedError): + time.sleep(interval) + return False + + +def start_server_in_subprocess(project: Project, port: int, open_browser: bool): + """Start the server in a separate process. Parameters ---------- project : Project - The project to launch. + The project to associate with the server. port : int The port to use for the server. open_browser : bool Whether to open the browser. - ready_event : multiprocessing.Event - Event to signal that the server is ready. + + Returns + ------- + subprocess.Popen + The process running the server. + + Raises + ------ + RuntimeError + If the server failed to start within the timeout period. """ - from skore.ui.app import create_app + cmd = [ + sys.executable, + "-m", + "skore.ui.server", + "--port", + str(port), + "--project-path", + str(project.path), + ] + + process = subprocess.Popen( + cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True + ) - async def lifespan(app: FastAPI): - ready_event.set() # Signal that the server is ready + if is_server_started(port): if open_browser: - webbrowser.open(f"http://localhost:{port}") # pragma: no cover - yield - - app = create_app(project=project, lifespan=lifespan) - uvicorn.run(app, host="0.0.0.0", port=port, log_level="error") + webbrowser.open(f"http://localhost:{port}") + return process + else: + process.terminate() + raise RuntimeError("Server failed to start within timeout period") def cleanup_server(project: Project, timeout: float = 5.0) -> bool: @@ -215,7 +259,23 @@ def cleanup_server(project: Project, timeout: float = 5.0) -> bool: return True +def block_before_cleanup(project: Project, process: subprocess.Popen) -> bool: + from skore import console + + try: + console.print("Press Ctrl+C to stop the server...") + while True: + if process.poll() is not None: + break + time.sleep(0.1) + except KeyboardInterrupt: + console.print("\nReceived keyboard interrupt, shutting down...") + finally: + cleanup_server(project) + + def _kill_all_servers(): + """Kill all running servers.""" state_path = platformdirs.user_state_path(appname="skore") for pid_file in state_path.glob("skore-server-*.json"): try: @@ -246,18 +306,19 @@ def _launch( ---------- project : Project The project to launch. - keep_alive : Union[str, bool], default="auto" + keep_alive : str or bool, default="auto" Whether to keep the server alive once the main process finishes. When False, the server will be terminated when the main process finishes. When True, - the server will be kept alive and thus block the main process from exiting. + the server will be kept alive until interrupted by the user (Ctrl+C). When `"auto"`, the server will be kept alive if the current execution context is not a notebook-like environment. - port : int, optional - The port to use for the server, by default None. - open_browser : bool, optional - Whether to open the browser, by default True. - verbose : bool, optional - Whether to print verbose output, by default False. + port : int, default=None + The port to use for the server. If None, a free port will be found starting from + 22140. + open_browser : bool, default=True + Whether to open the browser. By default, the browser is opened. + verbose : bool, default=False + Whether to print verbose output. By default, no output is printed. """ from skore import console # avoid circular import @@ -265,6 +326,8 @@ def _launch( port = find_free_port() if project._server_info is not None: + # The project is already attached to a server. + # We need to check if the server is still running or if we need to restart it. try: os.kill(project._server_info.pid, 0) @@ -277,32 +340,19 @@ def _launch( except ProcessLookupError: # zombie process cleanup_server(project) - ready_event = multiprocessing.Event() - - daemon = is_environment_notebook_like() if keep_alive == "auto" else not keep_alive + if keep_alive == "auto": + keep_alive = is_environment_notebook_like() with logger_context(logger, verbose): - process = multiprocessing.Process( - target=run_server, - args=(project, port, open_browser, ready_event), - daemon=daemon, - ) - process.start() + process = start_server_in_subprocess(project, port, open_browser) project._server_info = ServerInfo(project, port, process.pid) project._server_info.save_pid_file() - ready_event.wait() # wait for server to been started console.rule("[bold cyan]skore-UI[/bold cyan]") - msg = f"Running skore UI from '{project.name}' at URL http://localhost:{port}" - if not daemon: - msg += " (Press CTRL+C to quit)" console.print(msg) - if not daemon: - try: - process.join() - except KeyboardInterrupt: - cleanup_server(project) - else: + if keep_alive: atexit.register(cleanup_server, project) + else: + atexit.register(block_before_cleanup, project, process) diff --git a/skore/src/skore/ui/server.py b/skore/src/skore/ui/server.py new file mode 100644 index 000000000..8b48c5a64 --- /dev/null +++ b/skore/src/skore/ui/server.py @@ -0,0 +1,34 @@ +"""Server module to run the FastAPI application.""" + +import argparse + +import uvicorn + +from skore.project._load import _load +from skore.ui.app import create_app + + +def run_server(port: int, project_path: str): + """Run the uvicorn server with the given project and port. + + Parameters + ---------- + port : int + The port number to run the server on. + project_path : str + Path to the skore project to load. + """ + project = _load(project_path) + app = create_app(project=project) + + config = uvicorn.Config(app=app, host="0.0.0.0", port=port, log_level="error") + server = uvicorn.Server(config) + server.run() + + +if __name__ == "__main__": + parser = argparse.ArgumentParser(description="Run the skore UI server") + parser.add_argument("--port", type=int, required=True) + parser.add_argument("--project-path", type=str, required=True) + args = parser.parse_args() + run_server(args.port, args.project_path) From 616110fc80edc53e7d5adf4f584357076d57b826 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Mon, 27 Jan 2025 22:39:50 +0100 Subject: [PATCH 44/57] do not launch server --- examples/getting_started/plot_quick_start.py | 2 +- examples/getting_started/plot_skore_getting_started.py | 2 +- examples/getting_started/plot_tracking_items.py | 2 +- examples/getting_started/plot_working_with_projects.py | 2 +- examples/model_evaluation/plot_cross_validate.py | 2 +- examples/use_cases/plot_employee_salaries.py | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/examples/getting_started/plot_quick_start.py b/examples/getting_started/plot_quick_start.py index 4d04bdf7d..8d1d99a3e 100644 --- a/examples/getting_started/plot_quick_start.py +++ b/examples/getting_started/plot_quick_start.py @@ -12,7 +12,7 @@ # %% import skore -my_project = skore.open("my_project", create=True) +my_project = skore.open("my_project", create=True, serve=False) # %% # This will create a skore project directory named ``quick_start.skore`` in your diff --git a/examples/getting_started/plot_skore_getting_started.py b/examples/getting_started/plot_skore_getting_started.py index 9f9d78726..d0970e0fb 100644 --- a/examples/getting_started/plot_skore_getting_started.py +++ b/examples/getting_started/plot_skore_getting_started.py @@ -40,7 +40,7 @@ # %% import skore -my_project = skore.open("my_project", create=True) +my_project = skore.open("my_project", create=True, serve=False) # %% # Now that the project exists, we can write some Python code (in the same diff --git a/examples/getting_started/plot_tracking_items.py b/examples/getting_started/plot_tracking_items.py index e7f12dbda..2cda6842b 100644 --- a/examples/getting_started/plot_tracking_items.py +++ b/examples/getting_started/plot_tracking_items.py @@ -19,7 +19,7 @@ # %% import skore -my_project = skore.open("my_project", create=True) +my_project = skore.open("my_project", create=True, serve=False) # %% # Tracking an integer diff --git a/examples/getting_started/plot_working_with_projects.py b/examples/getting_started/plot_working_with_projects.py index c7cdb5cc0..efec8a1e2 100644 --- a/examples/getting_started/plot_working_with_projects.py +++ b/examples/getting_started/plot_working_with_projects.py @@ -19,7 +19,7 @@ # %% import skore -my_project = skore.open("my_project", create=True) +my_project = skore.open("my_project", create=True, serve=False) # %% # Storing integers diff --git a/examples/model_evaluation/plot_cross_validate.py b/examples/model_evaluation/plot_cross_validate.py index 57cf43946..f0e37163b 100644 --- a/examples/model_evaluation/plot_cross_validate.py +++ b/examples/model_evaluation/plot_cross_validate.py @@ -24,7 +24,7 @@ # %% import skore -my_project = skore.open("my_project", create=True) +my_project = skore.open("my_project", create=True, serve=False) # %% # Cross-validation in scikit-learn diff --git a/examples/use_cases/plot_employee_salaries.py b/examples/use_cases/plot_employee_salaries.py index 6d51489f1..c6c462641 100644 --- a/examples/use_cases/plot_employee_salaries.py +++ b/examples/use_cases/plot_employee_salaries.py @@ -28,7 +28,7 @@ # experiments. import skore -project = skore.open("my_project", create=True) +project = skore.open("my_project", create=True, serve=False) # %% # From 9cba8ee930570713f4967cb505431b927f3ec252 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Mon, 27 Jan 2025 23:31:04 +0100 Subject: [PATCH 45/57] iter --- skore/src/skore/project/_launch.py | 6 +- skore/tests/unit/project/test_launch.py | 78 ++++++++++++++++++++++++- 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index c05fa895b..40b4d1b07 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -204,7 +204,7 @@ def start_server_in_subprocess(project: Project, port: int, open_browser: bool): if is_server_started(port): if open_browser: - webbrowser.open(f"http://localhost:{port}") + webbrowser.open(f"http://localhost:{port}") # pragma: no cover return process else: process.terminate() @@ -353,6 +353,6 @@ def _launch( console.print(msg) if keep_alive: - atexit.register(cleanup_server, project) - else: atexit.register(block_before_cleanup, project, process) + else: + atexit.register(cleanup_server, project) diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 18229656a..c7627a65f 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -5,7 +5,14 @@ import psutil import pytest from skore.project._create import _create -from skore.project._launch import ServerInfo, _launch, cleanup_server, find_free_port +from skore.project._launch import ( + ServerInfo, + _launch, + block_before_cleanup, + cleanup_server, + find_free_port, + is_server_started, +) def test_find_free_port(): @@ -150,3 +157,72 @@ def mock_kill(pid, signal): assert skore_project._server_info is not None assert skore_project._server_info.port == 8001 assert skore_project._server_info.pid != 1234 + + +def test_is_server_started(monkeypatch): + """Check the behaviour of the is_server_started function.""" + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + port = find_free_port() + sock.bind(("", port)) + sock.listen(1) + + try: + assert is_server_started(port, timeout=1) is True + + unused_port = find_free_port() + assert is_server_started(unused_port, timeout=1) is False + + def mock_connect_ex(*args, **kwargs): + raise socket.timeout() + + monkeypatch.setattr(socket.socket, "connect_ex", mock_connect_ex) + assert is_server_started(port, timeout=1) is False + + finally: + sock.close() + + +def test_launch_server_timeout(tmp_path, monkeypatch): + """Test that launching server raises RuntimeError when it fails to start.""" + monkeypatch.setattr( + "skore.project._launch.is_server_started", lambda *args, **kwargs: False + ) + + skore_project = _create(tmp_path / "test_project") + + err_msg = "Server failed to start within timeout period" + with pytest.raises(RuntimeError, match=err_msg): + _launch(skore_project, open_browser=False, keep_alive=False) + + +def test_block_before_cleanup(tmp_path, capsys, monkeypatch): + """Check the behaviour of the block_before_cleanup function.""" + skore_project = _create(tmp_path / "test_project") + + class MockProcess: + def __init__(self): + self._poll_count = 0 + + def poll(self): + self._poll_count += 1 + # Return None for first call, then process ID to simulate termination + return None if self._poll_count == 1 else 12345 + + # Test normal termination + mock_process = MockProcess() + block_before_cleanup(skore_project, mock_process) + output = capsys.readouterr().out + assert "Press Ctrl+C to stop the server" in output + + # Test keyboard interrupt + mock_process = MockProcess() + + def mock_sleep(*args): + raise KeyboardInterrupt() + + monkeypatch.setattr("time.sleep", mock_sleep) + + block_before_cleanup(skore_project, mock_process) + output = capsys.readouterr().out + assert "Press Ctrl+C to stop the server" in output + assert "Received keyboard interrupt" in output From 8cd9915606f72340e5a8027b55dc5f9bc9702f57 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Tue, 28 Jan 2025 22:50:14 +0100 Subject: [PATCH 46/57] more tests --- skore/src/skore/project/_launch.py | 11 ++- skore/src/skore/ui/server.py | 2 +- skore/tests/unit/cli/test_cli.py | 28 ++++++ skore/tests/unit/project/test_launch.py | 115 +++++++++++++++++++----- 4 files changed, 132 insertions(+), 24 deletions(-) diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 40b4d1b07..3097ed1bc 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -259,7 +259,16 @@ def cleanup_server(project: Project, timeout: float = 5.0) -> bool: return True -def block_before_cleanup(project: Project, process: subprocess.Popen) -> bool: +def block_before_cleanup(project: Project, process: subprocess.Popen) -> None: + """Block the main process until the server is terminated. + + Parameters + ---------- + project : Project + The project instance. + process : subprocess.Popen + The process running the server. + """ from skore import console try: diff --git a/skore/src/skore/ui/server.py b/skore/src/skore/ui/server.py index 8b48c5a64..e8818edda 100644 --- a/skore/src/skore/ui/server.py +++ b/skore/src/skore/ui/server.py @@ -21,7 +21,7 @@ def run_server(port: int, project_path: str): project = _load(project_path) app = create_app(project=project) - config = uvicorn.Config(app=app, host="0.0.0.0", port=port, log_level="error") + config = uvicorn.Config(app=app, host="127.0.0.1", port=port, log_level="error") server = uvicorn.Server(config) server.run() diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 94d6f9465..a0884363e 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -92,3 +92,31 @@ def test_cli_open(tmp_path, monkeypatch): with pytest.raises(FileNotFoundError): cli(["open", "nonexistent_project", "--no-create"]) + + +def test_cli_kill(tmp_project_path): + """Test that CLI kill command properly terminates all running servers.""" + cli( + [ + "launch", + str(tmp_project_path), + "--no-keep-alive", + "--no-open-browser", + "--verbose", + ] + ) + + from skore import open + from skore.project._launch import ServerInfo + + project = open(tmp_project_path, serve=False) + pid_file = ServerInfo._get_pid_file_path(project) + assert pid_file.exists() + + cli(["kill", "--verbose"]) + assert not pid_file.exists() + + +def test_cli_kill_no_servers(): + """Test that CLI kill command works safely when no servers are running.""" + cli(["kill", "--verbose"]) diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index c7627a65f..d19816c64 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -1,3 +1,4 @@ +import json import os import socket @@ -7,6 +8,7 @@ from skore.project._create import _create from skore.project._launch import ( ServerInfo, + _kill_all_servers, _launch, block_before_cleanup, cleanup_server, @@ -91,20 +93,24 @@ def test_cleanup_server_timeout(tmp_path, monkeypatch): skore_project = _create(tmp_path / "test_project") class MockProcess: - def __init__(self, pid): - self.terminate_called = False - self.kill_called = False + def __init__(self): + self.poll_count = 0 + + def poll(self): + self.poll_count += 1 + return None if self.poll_count == 5 else 0 def terminate(self): self.terminate_called = True - def wait(self, timeout): - raise psutil.TimeoutExpired(1, timeout) + def wait(self, timeout=0.1): + self.wait_called = True + raise psutil.TimeoutExpired(timeout) def kill(self): self.kill_called = True - mock_process = MockProcess(1234) + mock_process = MockProcess() monkeypatch.setattr(psutil, "Process", lambda pid: mock_process) server_info = ServerInfo(skore_project, port=8000, pid=1234) @@ -114,6 +120,7 @@ def kill(self): cleanup_server(skore_project) assert mock_process.terminate_called + assert mock_process.wait_called assert mock_process.kill_called assert not server_info.pid_file.exists() assert skore_project._server_info is None @@ -195,34 +202,98 @@ def test_launch_server_timeout(tmp_path, monkeypatch): _launch(skore_project, open_browser=False, keep_alive=False) -def test_block_before_cleanup(tmp_path, capsys, monkeypatch): - """Check the behaviour of the block_before_cleanup function.""" +def test_block_before_cleanup_keyboard_interrupt(tmp_path, capsys, monkeypatch): + """Test block_before_cleanup behavior when receiving a KeyboardInterrupt.""" skore_project = _create(tmp_path / "test_project") class MockProcess: - def __init__(self): - self._poll_count = 0 - def poll(self): - self._poll_count += 1 - # Return None for first call, then process ID to simulate termination - return None if self._poll_count == 1 else 12345 + return None - # Test normal termination mock_process = MockProcess() - block_before_cleanup(skore_project, mock_process) - output = capsys.readouterr().out - assert "Press Ctrl+C to stop the server" in output - # Test keyboard interrupt - mock_process = MockProcess() + # Mock time.sleep to raise KeyboardInterrupt on first call + sleep_called = False def mock_sleep(*args): - raise KeyboardInterrupt() + nonlocal sleep_called + if not sleep_called: + sleep_called = True + raise KeyboardInterrupt() monkeypatch.setattr("time.sleep", mock_sleep) block_before_cleanup(skore_project, mock_process) + output = capsys.readouterr().out assert "Press Ctrl+C to stop the server" in output - assert "Received keyboard interrupt" in output + assert "Received keyboard interrupt, shutting down" in output + + +def test_kill_all_servers(tmp_path, monkeypatch): + """Test that _kill_all_servers properly terminates all running servers.""" + + def mock_user_state_path(*args, **kwargs): + # let's write in a temporary directory to avoid side effects from other tests + return tmp_path + + monkeypatch.setattr("platformdirs.user_state_path", mock_user_state_path) + + pid_files = [ + {"pid": 1234, "port": 8000}, + {"pid": 5678, "port": 8001}, + ] + + for i, info in enumerate(pid_files): + pid_file = tmp_path / f"skore-server-test{i}.json" + with open(pid_file, "w") as f: + json.dump(info, f) + + terminated_pids, killed_pids = [], [] + + class MockProcess: + def __init__(self, pid): + self.pid = pid + + def terminate(self): + terminated_pids.append(self.pid) + + def wait(self, timeout): + if self.pid == 1234: # First process terminates normally + return + raise psutil.TimeoutExpired(timeout) # Second process needs to be killed + + def kill(self): + killed_pids.append(self.pid) + + def mock_process(pid): + return MockProcess(pid) + + monkeypatch.setattr(psutil, "Process", mock_process) + + _kill_all_servers() + assert sorted(terminated_pids) == [1234, 5678] + assert killed_pids == [5678] + assert not list(tmp_path.glob("skore-server-*.json")) + + +def test_kill_all_servers_no_such_process(tmp_path, monkeypatch): + """Test _kill_all_servers when processes don't exist.""" + + def mock_user_state_path(*args, **kwargs): + # let's write in a temporary directory to avoid side effects from other tests + return tmp_path + + monkeypatch.setattr("platformdirs.user_state_path", mock_user_state_path) + + pid_file = tmp_path / "skore-server-test.json" + with open(pid_file, "w") as f: + json.dump({"pid": 1234, "port": 8000}, f) + + def mock_process(pid): + raise psutil.NoSuchProcess(pid) + + monkeypatch.setattr(psutil, "Process", mock_process) + + _kill_all_servers() + assert not list(tmp_path.glob("skore-server-*.json")) From 9da454c2e0c7d740a582e8a3df9ef8c841c9f3fe Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Tue, 28 Jan 2025 23:08:18 +0100 Subject: [PATCH 47/57] iter --- skore/tests/unit/project/test_launch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index d19816c64..9793b602a 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -180,7 +180,7 @@ def test_is_server_started(monkeypatch): assert is_server_started(unused_port, timeout=1) is False def mock_connect_ex(*args, **kwargs): - raise socket.timeout() + raise socket.timeout("Connection timed out") monkeypatch.setattr(socket.socket, "connect_ex", mock_connect_ex) assert is_server_started(port, timeout=1) is False From a052ff1312b3228eb55ba0ce100cf7a0c3ff8973 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Tue, 28 Jan 2025 23:15:13 +0100 Subject: [PATCH 48/57] test --- skore/tests/unit/project/test_launch.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 9793b602a..7febe1601 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -179,10 +179,10 @@ def test_is_server_started(monkeypatch): unused_port = find_free_port() assert is_server_started(unused_port, timeout=1) is False - def mock_connect_ex(*args, **kwargs): + def mock_create_connection(*args, **kwargs): raise socket.timeout("Connection timed out") - monkeypatch.setattr(socket.socket, "connect_ex", mock_connect_ex) + monkeypatch.setattr(socket, "create_connection", mock_create_connection) assert is_server_started(port, timeout=1) is False finally: From 690250c80d19efbf72f1a1caa9c4bcce252a83bf Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Tue, 28 Jan 2025 23:35:55 +0100 Subject: [PATCH 49/57] tests --- skore/src/skore/cli/cli.py | 76 +++++------------------------- skore/src/skore/project/_launch.py | 41 ++++++++++------ skore/tests/unit/cli/test_cli.py | 48 +++++-------------- skore/tests/unit/cli/test_open.py | 9 ++-- 4 files changed, 54 insertions(+), 120 deletions(-) diff --git a/skore/src/skore/cli/cli.py b/skore/src/skore/cli/cli.py index 90753cb12..b345cf86c 100644 --- a/skore/src/skore/cli/cli.py +++ b/skore/src/skore/cli/cli.py @@ -6,8 +6,7 @@ from skore.cli.color_format import ColorArgumentParser from skore.project import open from skore.project._create import _create -from skore.project._launch import _kill_all_servers, _launch -from skore.project._load import _load +from skore.project._launch import _kill_all_servers def cli(args: list[str]): @@ -39,50 +38,6 @@ def cli(args: list[str]): help="increase logging verbosity", ) - # launch the skore UI - parser_launch = subparsers.add_parser("launch", help="Launch the web UI") - parser_launch.add_argument( - "project_name", - help="the name or path of the project to open", - ) - parser_launch.add_argument( - "--keep-alive", - action=argparse.BooleanOptionalAction, - help=( - "whether to keep the server alive once the main process finishes " - "(default: %(default)s)" - ), - default="auto", - ) - parser_launch.add_argument( - "--port", - type=int, - help="the port at which to bind the UI server (default: %(default)s)", - default=None, - ) - parser_launch.add_argument( - "--open-browser", - action=argparse.BooleanOptionalAction, - help=( - "whether to automatically open a browser tab showing the web UI " - "(default: %(default)s)" - ), - default=True, - ) - parser_launch.add_argument( - "--verbose", - action="store_true", - help="increase logging verbosity", - ) - - # kill all UI servers - parser_kill = subparsers.add_parser("kill", help="Kill all UI servers") - parser_kill.add_argument( - "--verbose", - action="store_true", - help="increase logging verbosity", - ) - # open a skore project parser_open = subparsers.add_parser( "open", help='Create a "project.skore" file and start the UI' @@ -110,15 +65,6 @@ def cli(args: list[str]): help=("whether to serve the project (default: %(default)s)"), default=True, ) - parser_open.add_argument( - "--keep-alive", - action=argparse.BooleanOptionalAction, - help=( - "whether to keep the server alive once the main process finishes " - "(default: %(default)s)" - ), - default="auto", - ) parser_open.add_argument( "--port", type=int, @@ -131,6 +77,14 @@ def cli(args: list[str]): help="increase logging verbosity", ) + # kill all UI servers + parser_kill = subparsers.add_parser("kill", help="Kill all UI servers") + parser_kill.add_argument( + "--verbose", + action="store_true", + help="increase logging verbosity", + ) + parsed_args: argparse.Namespace = parser.parse_args(args) if parsed_args.subcommand == "create": @@ -139,25 +93,17 @@ def cli(args: list[str]): overwrite=parsed_args.overwrite, verbose=parsed_args.verbose, ) - elif parsed_args.subcommand == "launch": - _launch( - project=_load(project_name=parsed_args.project_name), - keep_alive=parsed_args.keep_alive, - port=parsed_args.port, - open_browser=parsed_args.open_browser, - verbose=parsed_args.verbose, - ) elif parsed_args.subcommand == "open": open( project_path=parsed_args.project_name, create=parsed_args.create, overwrite=parsed_args.overwrite, serve=parsed_args.serve, - keep_alive=parsed_args.keep_alive, + keep_alive=True, port=parsed_args.port, verbose=parsed_args.verbose, ) elif parsed_args.subcommand == "kill": - _kill_all_servers() + _kill_all_servers(verbose=parsed_args.verbose) else: parser.print_help() diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 3097ed1bc..cd3a1a0ca 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -283,23 +283,36 @@ def block_before_cleanup(project: Project, process: subprocess.Popen) -> None: cleanup_server(project) -def _kill_all_servers(): - """Kill all running servers.""" +def _kill_all_servers(verbose: bool = False): + """Kill all running servers. + + Parameters + ---------- + verbose : bool, default=False + Whether to print verbose output. + """ + from skore import console + state_path = platformdirs.user_state_path(appname="skore") - for pid_file in state_path.glob("skore-server-*.json"): - try: - pid_info = json.load(pid_file.open()) + with logger_context(logger, verbose): + for pid_file in state_path.glob("skore-server-*.json"): try: - process = psutil.Process(pid_info["pid"]) - process.terminate() + pid_info = json.load(pid_file.open()) try: - process.wait(timeout=5.0) - except psutil.TimeoutExpired: - process.kill() - except psutil.NoSuchProcess: - pass - finally: - pid_file.unlink() + process = psutil.Process(pid_info["pid"]) + process.terminate() + try: + process.wait(timeout=5.0) + except psutil.TimeoutExpired: + process.kill() + console.print( + f"Killed server at http://localhost:{pid_info['port']}" + ) + except psutil.NoSuchProcess: + pass + finally: + console.print(f"Deleted PID file {pid_file}") + pid_file.unlink() def _launch( diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index a0884363e..b3cdeb1ea 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -3,7 +3,9 @@ import os import pytest +from skore import open from skore.cli.cli import cli +from skore.project._launch import ServerInfo @pytest.fixture @@ -29,27 +31,6 @@ def close_project(path): assert not pid_file.exists() -def test_cli_launch(tmp_project_path): - """Test that CLI launch starts server with correct parameters.""" - cli( - [ - "launch", - str(tmp_project_path), - "--no-keep-alive", - "--no-open-browser", - "--verbose", - ] - ) - close_project(tmp_project_path) - - -def test_cli_launch_no_project_name(): - with pytest.raises(SystemExit): - cli( - ["launch", "--port", 0, "--no-keep-alive", "--no-open-browser", "--verbose"] - ) - - def test_cli_create(tmp_path): """Test that CLI create command creates the expected directory structure.""" os.chdir(tmp_path) # Change to temp directory for relative path testing @@ -85,31 +66,26 @@ def test_cli_open(tmp_path, monkeypatch): assert (project_path / "items").exists() assert (project_path / "views").exists() - cli(["open", str(project_path), "--no-keep-alive", "--verbose"]) + cli(["open", str(project_path), "--verbose"]) close_project(project_path) - cli(["open", str(project_path), "--no-keep-alive", "--no-create"]) + cli(["open", str(project_path), "--no-create"]) close_project(project_path) with pytest.raises(FileNotFoundError): cli(["open", "nonexistent_project", "--no-create"]) -def test_cli_kill(tmp_project_path): +def test_cli_kill(tmp_project_path, monkeypatch): """Test that CLI kill command properly terminates all running servers.""" - cli( - [ - "launch", - str(tmp_project_path), - "--no-keep-alive", - "--no-open-browser", - "--verbose", - ] - ) + # Force open_browser to False for all _launch calls + from skore.project._launch import _launch - from skore import open - from skore.project._launch import ServerInfo + monkeypatch.setattr( + "skore.project._launch._launch", + lambda *args, **kwargs: _launch(*args, **{**kwargs, "open_browser": False}), + ) - project = open(tmp_project_path, serve=False) + project = open(tmp_project_path, serve=True, keep_alive=False) pid_file = ServerInfo._get_pid_file_path(project) assert pid_file.exists() diff --git a/skore/tests/unit/cli/test_open.py b/skore/tests/unit/cli/test_open.py index ed74b99c1..388daa5e7 100644 --- a/skore/tests/unit/cli/test_open.py +++ b/skore/tests/unit/cli/test_open.py @@ -40,7 +40,6 @@ def test_cli_open(tmp_project_path, mock_launch): "--port", "8000", "--serve", - "--no-keep-alive", "--verbose", ] ) @@ -52,7 +51,7 @@ def test_cli_open_creates_project(tmp_path, mock_launch): project_path = tmp_path / "new_project.skore" assert not project_path.exists() - cli(["open", str(project_path), "--create", "--no-keep-alive"]) + cli(["open", str(project_path), "--create"]) assert project_path.exists() assert len(mock_launch) == 1 @@ -62,7 +61,7 @@ def test_cli_open_no_create_fails(tmp_path, mock_launch): project_path = tmp_path / "nonexistent.skore" with pytest.raises(FileNotFoundError): - cli(["open", str(project_path), "--no-create", "--no-keep-alive"]) + cli(["open", str(project_path), "--no-create"]) assert len(mock_launch) == 0 @@ -70,10 +69,10 @@ def test_cli_open_overwrite(tmp_path, mock_launch): """Test that CLI open can overwrite existing project.""" project_path = tmp_path / "overwrite_test.skore" - cli(["open", str(project_path), "--create", "--no-keep-alive"]) + cli(["open", str(project_path), "--create"]) initial_time = os.path.getmtime(project_path) - cli(["open", str(project_path), "--create", "--overwrite", "--no-keep-alive"]) + cli(["open", str(project_path), "--create", "--overwrite"]) new_time = os.path.getmtime(project_path) assert new_time > initial_time assert len(mock_launch) == 2 From 128480847e29c345447a9fc2730ca0cbe1eccf14 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Wed, 29 Jan 2025 00:53:44 +0100 Subject: [PATCH 50/57] more tests --- skore/src/skore/project/_launch.py | 4 +- skore/src/skore/ui/server.py | 8 +- skore/src/skore/utils/_environment.py | 6 +- skore/tests/integration/ui/test_server.py | 42 ++++++++++ skore/tests/unit/project/test_launch.py | 6 +- skore/tests/unit/utils/test_environment.py | 93 ++++++++++++++++++++++ 6 files changed, 149 insertions(+), 10 deletions(-) create mode 100644 skore/tests/integration/ui/test_server.py create mode 100644 skore/tests/unit/utils/test_environment.py diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index cd3a1a0ca..e2ad9563d 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -363,7 +363,7 @@ def _launch( cleanup_server(project) if keep_alive == "auto": - keep_alive = is_environment_notebook_like() + keep_alive = is_environment_notebook_like() # pragma: no cover with logger_context(logger, verbose): process = start_server_in_subprocess(project, port, open_browser) @@ -375,6 +375,6 @@ def _launch( console.print(msg) if keep_alive: - atexit.register(block_before_cleanup, project, process) + atexit.register(block_before_cleanup, project, process) # pragma: no cover else: atexit.register(cleanup_server, project) diff --git a/skore/src/skore/ui/server.py b/skore/src/skore/ui/server.py index e8818edda..f08f082f1 100644 --- a/skore/src/skore/ui/server.py +++ b/skore/src/skore/ui/server.py @@ -8,7 +8,7 @@ from skore.ui.app import create_app -def run_server(port: int, project_path: str): +def run_server(port: int, project_path: str) -> uvicorn.Server: """Run the uvicorn server with the given project and port. Parameters @@ -17,6 +17,11 @@ def run_server(port: int, project_path: str): The port number to run the server on. project_path : str Path to the skore project to load. + + Returns + ------- + uvicorn.Server + The uvicorn server instance. """ project = _load(project_path) app = create_app(project=project) @@ -24,6 +29,7 @@ def run_server(port: int, project_path: str): config = uvicorn.Config(app=app, host="127.0.0.1", port=port, log_level="error") server = uvicorn.Server(config) server.run() + return server if __name__ == "__main__": diff --git a/skore/src/skore/utils/_environment.py b/skore/src/skore/utils/_environment.py index cd91d72fd..e0c1a1218 100644 --- a/skore/src/skore/utils/_environment.py +++ b/skore/src/skore/utils/_environment.py @@ -15,11 +15,11 @@ def get_environment_info(): "details": {}, } - # Check for interactive mode env_info["is_interactive"] = hasattr(sys, "ps1") - # Check for Jupyter try: + # get_ipython() is defined when running in Jupyter or IPython + # there is no need to import IPython here shell = get_ipython().__class__.__name__ # type: ignore env_info["details"]["ipython_shell"] = shell @@ -31,7 +31,6 @@ def get_environment_info(): except NameError: pass - # Check for VSCode if "VSCODE_PID" in os.environ: env_info["is_vscode"] = True if env_info["is_interactive"]: @@ -39,7 +38,6 @@ def get_environment_info(): else: env_info["environment_name"] = "vscode_script" - # Add additional environment details env_info["details"]["python_executable"] = sys.executable env_info["details"]["python_version"] = sys.version diff --git a/skore/tests/integration/ui/test_server.py b/skore/tests/integration/ui/test_server.py new file mode 100644 index 000000000..20b1b909a --- /dev/null +++ b/skore/tests/integration/ui/test_server.py @@ -0,0 +1,42 @@ +from unittest.mock import MagicMock, patch + +import pytest +import skore +from skore.project._launch import find_free_port +from skore.ui.server import run_server + + +@pytest.fixture +def mock_uvicorn(): + """Fixture to mock uvicorn components and capture the server instance.""" + server_instance = MagicMock() + mock_config = MagicMock() + + with ( + patch("uvicorn.Server", return_value=server_instance) as mock_server, + patch("uvicorn.Config", return_value=mock_config) as mock_config_class, + ): + yield { + "server": mock_server, + "server_instance": server_instance, + "config": mock_config_class, + "config_instance": mock_config, + } + + +def test_run_server(mock_uvicorn, tmp_path): + """Test that run_server creates and runs a server with correct configuration.""" + project_dir = tmp_path / "test_project" + skore.open(project_dir, serve=False) + + port = find_free_port() + run_server(port=port, project_path=str(project_dir)) + + mock_uvicorn["config"].assert_called_once() + config_kwargs = mock_uvicorn["config"].call_args[1] + assert config_kwargs["port"] == port + assert config_kwargs["host"] == "127.0.0.1" + assert config_kwargs["log_level"] == "error" + + mock_uvicorn["server"].assert_called_once_with(mock_uvicorn["config_instance"]) + mock_uvicorn["server_instance"].run.assert_called_once() diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 7febe1601..3a9d03bfd 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -174,16 +174,16 @@ def test_is_server_started(monkeypatch): sock.listen(1) try: - assert is_server_started(port, timeout=1) is True + assert is_server_started(port, timeout=3) is True unused_port = find_free_port() - assert is_server_started(unused_port, timeout=1) is False + assert is_server_started(unused_port, timeout=3) is False def mock_create_connection(*args, **kwargs): raise socket.timeout("Connection timed out") monkeypatch.setattr(socket, "create_connection", mock_create_connection) - assert is_server_started(port, timeout=1) is False + assert is_server_started(port, timeout=3) is False finally: sock.close() diff --git a/skore/tests/unit/utils/test_environment.py b/skore/tests/unit/utils/test_environment.py new file mode 100644 index 000000000..e1cdd41f7 --- /dev/null +++ b/skore/tests/unit/utils/test_environment.py @@ -0,0 +1,93 @@ +import sys +from unittest.mock import patch + +import pytest +from skore.utils._environment import get_environment_info, is_environment_notebook_like + + +@pytest.fixture +def mock_sys_attributes(): + """Fixture to control sys attributes""" + with ( + patch.object(sys, "ps1", create=True), + patch.object(sys, "executable", "python3"), + patch.object(sys, "version", "3.8.0"), + ): + yield + + +def test_get_environment_info_standard_python(mock_sys_attributes): + """Test environment detection for standard Python execution""" + with patch.dict("os.environ", {}, clear=True): + info = get_environment_info() + + assert isinstance(info, dict) + assert info["is_jupyter"] is False + assert info["is_vscode"] is False + assert info["is_interactive"] is True # Due to mocked sys.ps1 + assert info["environment_name"] == "standard_python" + assert info["details"]["python_executable"] == "python3" + assert info["details"]["python_version"] == "3.8.0" + + +def test_get_environment_info_vscode(): + """Test environment detection for VS Code""" + with patch.dict("os.environ", {"VSCODE_PID": "12345"}): + info = get_environment_info() + + assert info["is_vscode"] is True + assert "vscode" in info["environment_name"] + + +@patch("skore.utils._environment.get_ipython", create=True) +def test_get_environment_info_jupyter(mock_get_ipython): + """Test environment detection for Jupyter""" + mock_get_ipython.return_value.__class__.__name__ = "ZMQInteractiveShell" + + info = get_environment_info() + + assert info["is_jupyter"] is True + assert info["environment_name"] == "jupyter" + assert info["details"]["ipython_shell"] == "ZMQInteractiveShell" + + +def test_is_environment_notebook_like(): + """Test notebook-like environment detection""" + with patch.dict("os.environ", {"VSCODE_PID": "12345"}): + assert is_environment_notebook_like() is True + + with patch.dict("os.environ", {}, clear=True): + assert is_environment_notebook_like() is False + + +@patch("skore.utils._environment.get_ipython", create=True) +def test_is_environment_notebook_like_jupyter(mock_get_ipython): + """Test notebook-like environment detection for Jupyter""" + mock_get_ipython.return_value.__class__.__name__ = "ZMQInteractiveShell" + + assert is_environment_notebook_like() is True + + +@patch("skore.utils._environment.get_ipython", create=True) +def test_get_environment_info_ipython_terminal(mock_get_ipython): + """Test environment detection for IPython terminal""" + mock_get_ipython.return_value.__class__.__name__ = "TerminalInteractiveShell" + + info = get_environment_info() + + assert info["is_jupyter"] is False + assert info["environment_name"] == "ipython_terminal" + assert info["details"]["ipython_shell"] == "TerminalInteractiveShell" + + +@patch("skore.utils._environment.os.environ", {"VSCODE_PID": "12345"}) +@patch("skore.utils._environment.sys") +def test_get_environment_info_vscode_interactive(mock_sys): + """Test environment detection for VSCode in interactive mode""" + mock_sys.ps1 = True + + info = get_environment_info() + + assert info["is_vscode"] is True + assert info["is_interactive"] is True + assert info["environment_name"] == "vscode_interactive" From fcc15911785576ccb9d3b645103490047581ba82 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Wed, 29 Jan 2025 00:58:07 +0100 Subject: [PATCH 51/57] iter --- skore/src/skore/ui/app.py | 4 +--- skore/src/skore/ui/project_routes.py | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/skore/src/skore/ui/app.py b/skore/src/skore/ui/app.py index 699b59923..452a8221d 100644 --- a/skore/src/skore/ui/app.py +++ b/skore/src/skore/ui/app.py @@ -15,9 +15,7 @@ from skore.ui.project_routes import router as project_router -def create_app( - project: Optional[Project] = None, lifespan: Optional[Lifespan] = None -) -> FastAPI: +def create_app(project: Project, lifespan: Optional[Lifespan] = None) -> FastAPI: """FastAPI factory used to create the API to interact with `stores`.""" app = FastAPI(lifespan=lifespan) diff --git a/skore/src/skore/ui/project_routes.py b/skore/src/skore/ui/project_routes.py index 271f2011c..e931dce38 100644 --- a/skore/src/skore/ui/project_routes.py +++ b/skore/src/skore/ui/project_routes.py @@ -72,7 +72,7 @@ def __project_as_serializable(project: Project) -> SerializableProject: @router.get("/items", response_class=ORJSONResponse) async def get_items(request: Request): """Serialize a project and send it.""" - project = request.app.state.project + project: Project = request.app.state.project return __project_as_serializable(project) @@ -140,6 +140,6 @@ class NotePayload: @router.put("/note", status_code=status.HTTP_201_CREATED) async def set_note(request: Request, payload: NotePayload): """Add a note to the given item.""" - project = request.app.state.project + project: Project = request.app.state.project project.set_note(key=payload.key, note=payload.message, version=payload.version) return __project_as_serializable(project) From 1682aa491ead77141b9345de01acd9070a9638e9 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Wed, 29 Jan 2025 00:59:43 +0100 Subject: [PATCH 52/57] remove launch --- skore/src/skore/cli/color_format.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skore/src/skore/cli/color_format.py b/skore/src/skore/cli/color_format.py index 7ecbde5d9..7927eb21e 100644 --- a/skore/src/skore/cli/color_format.py +++ b/skore/src/skore/cli/color_format.py @@ -79,7 +79,7 @@ def format_help(self): # Color the subcommands in cyan help_text = re.sub( - r"(?<=\s)(launch|create|open|kill)(?=\s+)", + r"(?<=\s)(create|open|kill)(?=\s+)", r"[cyan bold]\1[/cyan bold]", help_text, ) From e6dd52ab6a1a3a86a57b21a23a25bc368bf8c6d0 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Wed, 29 Jan 2025 20:34:12 +0100 Subject: [PATCH 53/57] iter --- skore/tests/unit/cli/test_cli.py | 71 ++++++++++++-------- skore/tests/unit/cli/test_open.py | 86 ------------------------- skore/tests/unit/project/test_open.py | 93 --------------------------- 3 files changed, 45 insertions(+), 205 deletions(-) delete mode 100644 skore/tests/unit/cli/test_open.py delete mode 100644 skore/tests/unit/project/test_open.py diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 5cb9ea7fe..777dffc4e 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -1,44 +1,63 @@ +import os + import pytest from skore.cli.cli import cli -def test_cli(monkeypatch, tmp_path): - """cli passes its arguments down to `launch`.""" +@pytest.fixture +def tmp_project_path(tmp_path): + """Create a project at `tmp_path` and return its absolute path.""" + # Project path must end with ".skore" + project_path = tmp_path.parent / (tmp_path.name + ".skore") + os.mkdir(project_path) + os.mkdir(project_path / "items") + os.mkdir(project_path / "views") + return project_path + - launch_project_name = None - launch_port = None - launch_open_browser = None - launch_verbose = None +@pytest.fixture +def mock_launch(monkeypatch): + """Fixture that mocks the _launch function and tracks calls to it.""" + calls = [] - def fake_launch(project_name, port, open_browser, verbose): - nonlocal launch_project_name - nonlocal launch_port - nonlocal launch_open_browser - nonlocal launch_verbose + def _mock_launch( + project, keep_alive="auto", port=None, open_browser=True, verbose=False + ): + calls.append((project, keep_alive, port, open_browser, verbose)) - launch_project_name = project_name - launch_port = port - launch_open_browser = open_browser - launch_verbose = verbose + monkeypatch.setattr("skore.project._launch._launch", _mock_launch) + return calls - monkeypatch.setattr("skore.cli.cli.launch", fake_launch) +def test_cli_open(tmp_project_path, mock_launch): + """Test that CLI open creates a project and starts server with correct + parameters.""" cli( [ - str(tmp_path / "my_project.skore"), + "open", + str(tmp_project_path), "--port", - "888", - "--no-open-browser", + "8000", + "--serve", "--verbose", ] ) + assert len(mock_launch) == 1 + + +def test_cli_open_creates_project(tmp_path, mock_launch): + """Test that CLI open creates a project when it doesn't exist.""" + project_path = tmp_path / "new_project.skore" + assert not project_path.exists() + + cli(["open", str(project_path)]) + assert project_path.exists() + assert len(mock_launch) == 1 - assert launch_project_name == str(tmp_path / "my_project.skore") - assert launch_port == 888 - assert launch_open_browser is False - assert launch_verbose is True +def test_cli_open_no_serve(tmp_path, mock_launch): + """Test that server is not started when --no-serve flag is passed.""" + project_path = tmp_path / "no_serve.skore" -def test_cli_no_project_name(): - with pytest.raises(SystemExit): - cli([]) + cli(["open", str(project_path), "--no-serve"]) + assert len(mock_launch) == 0 diff --git a/skore/tests/unit/cli/test_open.py b/skore/tests/unit/cli/test_open.py deleted file mode 100644 index 388daa5e7..000000000 --- a/skore/tests/unit/cli/test_open.py +++ /dev/null @@ -1,86 +0,0 @@ -import os - -import pytest -from skore.cli.cli import cli - - -@pytest.fixture -def tmp_project_path(tmp_path): - """Create a project at `tmp_path` and return its absolute path.""" - # Project path must end with ".skore" - project_path = tmp_path.parent / (tmp_path.name + ".skore") - os.mkdir(project_path) - os.mkdir(project_path / "items") - os.mkdir(project_path / "views") - return project_path - - -@pytest.fixture -def mock_launch(monkeypatch): - """Fixture that mocks the _launch function and tracks calls to it.""" - calls = [] - - def _mock_launch( - project, keep_alive="auto", port=None, open_browser=True, verbose=False - ): - calls.append((project, keep_alive, port, open_browser, verbose)) - - monkeypatch.setattr("skore.project._launch._launch", _mock_launch) - return calls - - -def test_cli_open(tmp_project_path, mock_launch): - """Test that CLI open creates a project and starts server with correct - parameters.""" - cli( - [ - "open", - str(tmp_project_path), - "--overwrite", - "--port", - "8000", - "--serve", - "--verbose", - ] - ) - assert len(mock_launch) == 1 - - -def test_cli_open_creates_project(tmp_path, mock_launch): - """Test that CLI open creates a project when it doesn't exist.""" - project_path = tmp_path / "new_project.skore" - assert not project_path.exists() - - cli(["open", str(project_path), "--create"]) - assert project_path.exists() - assert len(mock_launch) == 1 - - -def test_cli_open_no_create_fails(tmp_path, mock_launch): - """Test that CLI open fails when project doesn't exist and create=False.""" - project_path = tmp_path / "nonexistent.skore" - - with pytest.raises(FileNotFoundError): - cli(["open", str(project_path), "--no-create"]) - assert len(mock_launch) == 0 - - -def test_cli_open_overwrite(tmp_path, mock_launch): - """Test that CLI open can overwrite existing project.""" - project_path = tmp_path / "overwrite_test.skore" - - cli(["open", str(project_path), "--create"]) - initial_time = os.path.getmtime(project_path) - - cli(["open", str(project_path), "--create", "--overwrite"]) - new_time = os.path.getmtime(project_path) - assert new_time > initial_time - assert len(mock_launch) == 2 - - -def test_cli_open_no_serve(tmp_path, mock_launch): - """Test that server is not started when --no-serve flag is passed.""" - project_path = tmp_path / "no_serve.skore" - - cli(["open", str(project_path), "--create", "--no-serve"]) - assert len(mock_launch) == 0 diff --git a/skore/tests/unit/project/test_open.py b/skore/tests/unit/project/test_open.py deleted file mode 100644 index 446ffb431..000000000 --- a/skore/tests/unit/project/test_open.py +++ /dev/null @@ -1,93 +0,0 @@ -import os -from contextlib import contextmanager - -import pytest -from skore.project import Project, open - - -@pytest.fixture -def tmp_project_path(tmp_path): - """Create a project at `tmp_path` and return its absolute path.""" - # Project path must end with ".skore" - project_path = tmp_path.parent / (tmp_path.name + ".skore") - os.mkdir(project_path) - os.mkdir(project_path / "items") - os.mkdir(project_path / "views") - return project_path - - -@contextmanager -def project_changed(project_path, modified=True): - """Assert that the project at `project_path` was changed. - - If `modified` is False, instead assert that it was *not* changed. - """ - (project_path / "my_test_file").write_text("hello") - yield - if modified: - assert not (project_path / "my_test_file").exists() - else: - assert (project_path / "my_test_file").exists() - - -def test_open_relative_path(tmp_project_path): - """If passed a relative path, `open` operates in the current working directory.""" - os.chdir(tmp_project_path.parent) - p = open(tmp_project_path.name, create=False, serve=False) - assert isinstance(p, Project) - - -def test_open_default(tmp_project_path): - """If a project already exists, `open` loads it.""" - with project_changed(tmp_project_path, modified=False): - p = open(tmp_project_path, serve=False) - assert isinstance(p, Project) - - -def test_open_default_no_project(tmp_path): - """If no project exists, `open` creates it.""" - with project_changed(tmp_path, modified=False): - p = open(tmp_path, serve=False) - assert isinstance(p, Project) - - -def test_open_project_exists_create_true_overwrite_true(tmp_project_path): - """If the project exists, and `create` and `overwrite` are set to `True`, - `open` overwrites it with a new one.""" - with project_changed(tmp_project_path): - open(tmp_project_path, create=True, overwrite=True, serve=False) - - -def test_open_project_exists_create_true_overwrite_false(tmp_project_path): - with project_changed(tmp_project_path, modified=False): - open(tmp_project_path, create=True, overwrite=False, serve=False) - - -def test_open_project_exists_create_false_overwrite_true(tmp_project_path): - p = open(tmp_project_path, create=False, overwrite=True, serve=False) - assert isinstance(p, Project) - - -def test_open_project_exists_create_false_overwrite_false(tmp_project_path): - p = open(tmp_project_path, create=False, overwrite=False, serve=False) - assert isinstance(p, Project) - - -def test_open_no_project_create_true_overwrite_true(tmp_path): - p = open(tmp_path, create=True, overwrite=True, serve=False) - assert isinstance(p, Project) - - -def test_open_no_project_create_true_overwrite_false(tmp_path): - p = open(tmp_path, create=True, overwrite=False, serve=False) - assert isinstance(p, Project) - - -def test_open_no_project_create_false_overwrite_true(tmp_path): - with pytest.raises(FileNotFoundError): - open(tmp_path, create=False, overwrite=True, serve=False) - - -def test_open_no_project_create_false_overwrite_false(tmp_path): - with pytest.raises(FileNotFoundError): - open(tmp_path, create=False, overwrite=False, serve=False) From 38f1c8d823b027b964e0c9761e4d42bc038bd49c Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Wed, 29 Jan 2025 20:49:28 +0100 Subject: [PATCH 54/57] iter --- skore/src/skore/project/_launch.py | 2 +- skore/src/skore/project/project.py | 1 + skore/tests/unit/cli/test_cli.py | 16 ++- skore/tests/unit/project/test_create.py | 152 ------------------------ skore/tests/unit/project/test_launch.py | 24 ++-- 5 files changed, 17 insertions(+), 178 deletions(-) delete mode 100644 skore/tests/unit/project/test_create.py diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 0e6693713..66ae8b876 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -87,7 +87,7 @@ def _get_pid_file_path(project: Project) -> Path: if project.path is not None: project_identifier = joblib.hash(str(project.path), hash_name="sha1") else: - project_identifier = joblib.hash(project.name, hash_name="sha1") + raise ValueError("Project path is required to create a PID file") state_path = platformdirs.user_state_path(appname="skore") return state_path / f"skore-server-{project_identifier}.json" diff --git a/skore/src/skore/project/project.py b/skore/src/skore/project/project.py index 757940485..b46b9c971 100644 --- a/skore/src/skore/project/project.py +++ b/skore/src/skore/project/project.py @@ -80,6 +80,7 @@ def __init__( self.path = Path(path) self.path = self.path.with_suffix(".skore") self.path = self.path.resolve() + self.name = self.path.name if if_exists == "raise" and self.path.exists(): raise FileExistsError(f"Project '{str(path)}' already exists.") diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 777dffc4e..92ad532c6 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -29,12 +29,10 @@ def _mock_launch( return calls -def test_cli_open(tmp_project_path, mock_launch): - """Test that CLI open creates a project and starts server with correct - parameters.""" +def test_cli(tmp_project_path, mock_launch): + """Check that the CLI create a project and launch the server.""" cli( [ - "open", str(tmp_project_path), "--port", "8000", @@ -55,9 +53,9 @@ def test_cli_open_creates_project(tmp_path, mock_launch): assert len(mock_launch) == 1 -def test_cli_open_no_serve(tmp_path, mock_launch): - """Test that server is not started when --no-serve flag is passed.""" - project_path = tmp_path / "no_serve.skore" +# def test_cli_open_no_serve(tmp_path, mock_launch): +# """Test that server is not started when --no-serve flag is passed.""" +# project_path = tmp_path / "no_serve.skore" - cli(["open", str(project_path), "--no-serve"]) - assert len(mock_launch) == 0 +# cli(["open", str(project_path), "--no-serve"]) +# assert len(mock_launch) == 0 diff --git a/skore/tests/unit/project/test_create.py b/skore/tests/unit/project/test_create.py deleted file mode 100644 index 60658f8eb..000000000 --- a/skore/tests/unit/project/test_create.py +++ /dev/null @@ -1,152 +0,0 @@ -from pathlib import Path - -import pytest -from skore.exceptions import ( - InvalidProjectNameError, - ProjectCreationError, - ProjectPermissionError, -) -from skore.project._create import _create, _validate_project_name - - -@pytest.mark.parametrize( - "project_name,expected", - [ - ( - "a" * 250, - (False, InvalidProjectNameError()), - ), - ( - "%", - (False, InvalidProjectNameError()), - ), - ( - "hello world", - (False, InvalidProjectNameError()), - ), - ], -) -def test_validate_project_name(project_name, expected): - result, exception = _validate_project_name(project_name) - expected_result, expected_exception = expected - assert result == expected_result - assert type(exception) is type(expected_exception) - - -@pytest.mark.parametrize("project_name", ["hello", "hello.skore"]) -def test_create_project(project_name, tmp_path): - _create(tmp_path / project_name) - assert (tmp_path / "hello.skore").exists() - - -# TODO: If using fixtures in test cases is possible, join this with -# `test_create_project` -def test_create_project_absolute_path(tmp_path): - _create(tmp_path / "hello") - assert (tmp_path / "hello.skore").exists() - - -def test_create_project_fails_if_file_exists(tmp_path): - _create(tmp_path / "hello") - assert (tmp_path / "hello.skore").exists() - with pytest.raises(FileExistsError): - _create(tmp_path / "hello") - - -def test_create_project_fails_if_permission_denied(tmp_path): - with pytest.raises(ProjectCreationError): - _create("/") - - -@pytest.mark.parametrize("project_name", ["hello.txt", "%%%", "COM1"]) -def test_create_project_fails_if_invalid_name(project_name, tmp_path): - with pytest.raises(ProjectCreationError): - _create(tmp_path / project_name) - - -def test_create_project_invalid_names(): - """Test project creation with invalid names.""" - invalid_names = [ - "CON", # Reserved name - "COM1", # Reserved name - "LPT1", # Reserved name - "-invalid", # Starts with hyphen - "_invalid", # Starts with underscore - "invalid@project", # Invalid character - "a" * 251, # Too long (255 - len('.skore')) - ] - - for name in invalid_names: - with pytest.raises(ProjectCreationError): - _create(name) - - -def test_create_project_existing_no_overwrite(tmp_path): - """Test creating project with existing name without overwrite flag.""" - project_path = tmp_path / "existing_project" - - _create(project_path) - with pytest.raises(FileExistsError): - _create(project_path) - - -def test_create_project_permission_error(tmp_path, monkeypatch): - """Test project creation with permission error.""" - - def mock_mkdir(*args, **kwargs): - raise PermissionError("Permission denied") - - # Patch Path.mkdir to raise PermissionError - monkeypatch.setattr(Path, "mkdir", mock_mkdir) - - with pytest.raises(ProjectPermissionError): - _create(tmp_path / "permission_denied") - - -def test_create_project_general_error(tmp_path, monkeypatch): - """Test project creation with general error.""" - - def mock_mkdir(*args, **kwargs): - raise RuntimeError("Unknown error") - - monkeypatch.setattr(Path, "mkdir", mock_mkdir) - - with pytest.raises(ProjectCreationError): - _create(tmp_path / "error_project") - - -def test_create_project_subdirectory_errors(tmp_path, monkeypatch): - """Test project creation fails when unable to create subdirectories.""" - - original_mkdir = Path.mkdir - failed_path = None - - def mock_mkdir(*args, **kwargs): - nonlocal failed_path - self = args[0] if args else kwargs.get("self") - - # Let the project directory creation succeed - if self.name.endswith(".skore"): - return original_mkdir(*args, **kwargs) - - # Fail if this is the path we want to fail - if failed_path and self.name == failed_path: - raise RuntimeError(f"Failed to create {failed_path} directory") - - return original_mkdir(*args, **kwargs) - - monkeypatch.setattr(Path, "mkdir", mock_mkdir) - - # Test items directory creation failure - failed_path = "items" - with pytest.raises(ProjectCreationError) as exc_info: - _create(tmp_path / "failed_items") - assert "Unable to create project file" in str(exc_info.value) - assert (tmp_path / "failed_items.skore").exists() - - # Test views directory creation failure - failed_path = "views" - with pytest.raises(ProjectCreationError) as exc_info: - _create(tmp_path / "failed_views") - assert "Unable to create project file" in str(exc_info.value) - assert (tmp_path / "failed_views.skore").exists() diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index 3a9d03bfd..d71506b70 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -5,7 +5,6 @@ import joblib import psutil import pytest -from skore.project._create import _create from skore.project._launch import ( ServerInfo, _kill_all_servers, @@ -15,6 +14,7 @@ find_free_port, is_server_started, ) +from skore.project.project import Project def test_find_free_port(): @@ -36,14 +36,6 @@ def test_find_free_port(): sock.close() -def test_server_info_in_memory_project(in_memory_project): - """Check the generation of the PID file path for an in-memory project.""" - server_info = ServerInfo(in_memory_project, port=20000, pid=1234) - assert server_info.port == 20000 - assert server_info.pid == 1234 - assert server_info.pid_file.name.startswith("skore-server-") - - def test_server_info(on_disk_project): """Check the ServerInfo class behaviour.""" server_info = ServerInfo(on_disk_project, port=30000, pid=1234) @@ -58,7 +50,7 @@ def test_server_info(on_disk_project): def test_launch(capsys, tmp_path): """Check the general behaviour of the launch function.""" - skore_project = _create(tmp_path / "test_project") + skore_project = Project(tmp_path / "test_project") _launch(skore_project, open_browser=False, keep_alive=False, verbose=True) assert "Running skore UI" in capsys.readouterr().out assert skore_project._server_info is not None @@ -84,13 +76,13 @@ def test_launch(capsys, tmp_path): def test_cleanup_server_not_running(tmp_path): """Check that cleanup does not fail when the server is not running.""" - skore_project = _create(tmp_path / "test_project") + skore_project = Project(tmp_path / "test_project") cleanup_server(skore_project) def test_cleanup_server_timeout(tmp_path, monkeypatch): """Test cleanup_server when process termination times out.""" - skore_project = _create(tmp_path / "test_project") + skore_project = Project(tmp_path / "test_project") class MockProcess: def __init__(self): @@ -128,7 +120,7 @@ def kill(self): def test_cleanup_server_no_process(tmp_path, monkeypatch): """Test cleanup_server when the process no longer exists.""" - skore_project = _create(tmp_path / "test_project") + skore_project = Project(tmp_path / "test_project") def mock_process_init(pid): raise psutil.NoSuchProcess(pid) @@ -147,7 +139,7 @@ def mock_process_init(pid): def test_launch_zombie_process(tmp_path, monkeypatch): """Test launch handling when encountering a zombie process.""" - skore_project = _create(tmp_path / "test_project") + skore_project = Project(tmp_path / "test_project") server_info = ServerInfo(skore_project, port=8000, pid=1234) skore_project._server_info = server_info server_info.save_pid_file() @@ -195,7 +187,7 @@ def test_launch_server_timeout(tmp_path, monkeypatch): "skore.project._launch.is_server_started", lambda *args, **kwargs: False ) - skore_project = _create(tmp_path / "test_project") + skore_project = Project(tmp_path / "test_project") err_msg = "Server failed to start within timeout period" with pytest.raises(RuntimeError, match=err_msg): @@ -204,7 +196,7 @@ def test_launch_server_timeout(tmp_path, monkeypatch): def test_block_before_cleanup_keyboard_interrupt(tmp_path, capsys, monkeypatch): """Test block_before_cleanup behavior when receiving a KeyboardInterrupt.""" - skore_project = _create(tmp_path / "test_project") + skore_project = Project(tmp_path / "test_project") class MockProcess: def poll(self): From 2683f3e326001a2e5593abc44609ee83c0751f80 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Wed, 29 Jan 2025 21:12:44 +0100 Subject: [PATCH 55/57] iter --- skore/src/skore/__init__.py | 3 ++- skore/tests/unit/cli/test_cli.py | 18 ------------------ 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/skore/src/skore/__init__.py b/skore/src/skore/__init__.py index cdf666230..72bad3ce8 100644 --- a/skore/src/skore/__init__.py +++ b/skore/src/skore/__init__.py @@ -5,7 +5,7 @@ from rich.console import Console from rich.theme import Theme -from skore.project import Project +from skore.project import Project, open from skore.sklearn import ( CrossValidationReport, CrossValidationReporter, @@ -20,6 +20,7 @@ "CrossValidationReport", "EstimatorReport", "Project", + "open", "show_versions", "train_test_split", ] diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 92ad532c6..416bcfcc4 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -41,21 +41,3 @@ def test_cli(tmp_project_path, mock_launch): ] ) assert len(mock_launch) == 1 - - -def test_cli_open_creates_project(tmp_path, mock_launch): - """Test that CLI open creates a project when it doesn't exist.""" - project_path = tmp_path / "new_project.skore" - assert not project_path.exists() - - cli(["open", str(project_path)]) - assert project_path.exists() - assert len(mock_launch) == 1 - - -# def test_cli_open_no_serve(tmp_path, mock_launch): -# """Test that server is not started when --no-serve flag is passed.""" -# project_path = tmp_path / "no_serve.skore" - -# cli(["open", str(project_path), "--no-serve"]) -# assert len(mock_launch) == 0 From 602030f6c9206ce999c9a60a8d700ca564872cd3 Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Wed, 29 Jan 2025 23:14:39 +0100 Subject: [PATCH 56/57] test command line --- skore/src/skore/cli/cli.py | 75 ++++++++++--------- skore/src/skore/project/_launch.py | 2 +- skore/src/skore/project/project.py | 5 ++ skore/tests/unit/cli/test_cli.py | 96 +++++++++++++++---------- skore/tests/unit/project/test_launch.py | 11 ++- 5 files changed, 114 insertions(+), 75 deletions(-) diff --git a/skore/src/skore/cli/cli.py b/skore/src/skore/cli/cli.py index 411345927..68be98643 100644 --- a/skore/src/skore/cli/cli.py +++ b/skore/src/skore/cli/cli.py @@ -4,60 +4,67 @@ from importlib.metadata import version from skore.cli.color_format import ColorArgumentParser -from skore.project._open import open +from skore.project import open +from skore.project._launch import _kill_all_servers -def argumentparser(): - """Argument parser for the Skore CLI.""" - parser = ColorArgumentParser( - prog="skore-ui", - description="Launch the skore UI on a defined skore project.", - ) +def cli(args: list[str]): + """CLI for Skore.""" + parser = ColorArgumentParser(prog="skore-ui") parser.add_argument( - "--version", - action="version", - version=f"%(prog)s {version('skore')}", + "--version", action="version", version=f"%(prog)s {version('skore')}" ) - parser.add_argument( - "project_name", - help="the name or path of the project to be created or opened", - ) + subparsers = parser.add_subparsers(dest="subcommand") - parser.add_argument( + # open a skore project + parser_open = subparsers.add_parser( + "open", help="Open a skore project and start the UI" + ) + parser_open.add_argument( + "project_path", + nargs="?", + help="the name or path of the project to be opened", + ) + parser_open.add_argument( "--serve", action=argparse.BooleanOptionalAction, help=("whether to serve the project (default: %(default)s)"), default=True, ) - - parser.add_argument( + parser_open.add_argument( "--port", type=int, help="the port at which to bind the UI server (default: %(default)s)", - default=22140, + default=None, ) - - parser.add_argument( + parser_open.add_argument( "--verbose", action="store_true", help="increase logging verbosity", ) - return parser + # kill all UI servers + parser_kill = subparsers.add_parser("kill", help="Kill all UI servers") + parser_kill.add_argument( + "--verbose", + action="store_true", + help="increase logging verbosity", + ) + parsed_args: argparse.Namespace = parser.parse_args(args) -def cli(args: list[str]): - """CLI for Skore.""" - parser = argumentparser() - arguments = parser.parse_args(args) - - open( - project_path=arguments.project_name, - if_exists="load", - serve=arguments.serve, - keep_alive=True, - port=arguments.port, - verbose=arguments.verbose, - ) + if parsed_args.subcommand == "open": + open( + project_path=parsed_args.project_path, + if_exists="load", + serve=parsed_args.serve, + keep_alive=True, + port=parsed_args.port, + verbose=parsed_args.verbose, + ) + elif parsed_args.subcommand == "kill": + _kill_all_servers(verbose=parsed_args.verbose) + else: + parser.print_help() diff --git a/skore/src/skore/project/_launch.py b/skore/src/skore/project/_launch.py index 66ae8b876..74524c022 100644 --- a/skore/src/skore/project/_launch.py +++ b/skore/src/skore/project/_launch.py @@ -113,7 +113,7 @@ def rejoin(cls, project: Project): """ pid_file = cls._get_pid_file_path(project) if not pid_file.exists(): - return + return None info = json.load(pid_file.open()) return cls(project, info["port"], info["pid"]) diff --git a/skore/src/skore/project/project.py b/skore/src/skore/project/project.py index b46b9c971..9935f0380 100644 --- a/skore/src/skore/project/project.py +++ b/skore/src/skore/project/project.py @@ -100,6 +100,11 @@ def __init__( if "default" not in self._view_repository: self._view_repository.put_view("default", View(layout=[])) + # Check if the project should rejoin a server + from skore.project._launch import ServerInfo # avoid circular import + + self._server_info = ServerInfo.rejoin(self) + def clear(self): """Clear the project.""" for item_key in self._item_repository: diff --git a/skore/tests/unit/cli/test_cli.py b/skore/tests/unit/cli/test_cli.py index 416bcfcc4..81faa0d5e 100644 --- a/skore/tests/unit/cli/test_cli.py +++ b/skore/tests/unit/cli/test_cli.py @@ -1,43 +1,61 @@ -import os - import pytest from skore.cli.cli import cli -@pytest.fixture -def tmp_project_path(tmp_path): - """Create a project at `tmp_path` and return its absolute path.""" - # Project path must end with ".skore" - project_path = tmp_path.parent / (tmp_path.name + ".skore") - os.mkdir(project_path) - os.mkdir(project_path / "items") - os.mkdir(project_path / "views") - return project_path - - -@pytest.fixture -def mock_launch(monkeypatch): - """Fixture that mocks the _launch function and tracks calls to it.""" - calls = [] - - def _mock_launch( - project, keep_alive="auto", port=None, open_browser=True, verbose=False - ): - calls.append((project, keep_alive, port, open_browser, verbose)) - - monkeypatch.setattr("skore.project._launch._launch", _mock_launch) - return calls - - -def test_cli(tmp_project_path, mock_launch): - """Check that the CLI create a project and launch the server.""" - cli( - [ - str(tmp_project_path), - "--port", - "8000", - "--serve", - "--verbose", - ] - ) - assert len(mock_launch) == 1 +@pytest.mark.parametrize( + "args,expected_outputs", + [ + (["--help"], ["usage: skore-ui", "open", "kill"]), + (["-h"], ["usage: skore-ui", "open", "kill"]), + (["open", "--help"], ["usage: skore-ui open", "project_path", "--serve"]), + (["open", "-h"], ["usage: skore-ui open", "project_path", "--serve"]), + ], +) +def test_help_messages(capsys, args, expected_outputs): + """Test that help messages are displayed correctly.""" + with pytest.raises(SystemExit) as exc_info: + cli(args) + + assert exc_info.value.code == 0 + captured = capsys.readouterr() + for expected in expected_outputs: + assert expected in captured.out + + +def test_kill_command(tmp_path, monkeypatch): + """Test that kill command executes without errors and calls the right function.""" + # Mock platformdirs state dir to use a temporary directory and avoid side-effects + with monkeypatch.context() as m: + m.setattr("platformdirs.user_state_path", lambda appname: tmp_path) + cli(["kill"]) + + +def test_open_command(tmp_path, monkeypatch): + """Test that open command.""" + project_path = tmp_path / "test_project.skore" + project_path.mkdir() + + # Mock _launch to prevent browser opening and match the actual signature + with monkeypatch.context() as m: + m.setattr( + "skore.project._launch._launch", + lambda project, + keep_alive="auto", + port=None, + open_browser=True, + verbose=False: None, + ) + + cli(["open", str(project_path), "--no-serve"]) + + assert project_path.exists() + assert project_path.is_dir() + + # We should be able to open the same project again + cli(["open", str(project_path), "--no-serve"]) + + assert project_path.exists() + assert project_path.is_dir() + + # Let's start the server + cli(["open", str(project_path), "--serve"]) diff --git a/skore/tests/unit/project/test_launch.py b/skore/tests/unit/project/test_launch.py index d71506b70..64ed9a983 100644 --- a/skore/tests/unit/project/test_launch.py +++ b/skore/tests/unit/project/test_launch.py @@ -36,17 +36,26 @@ def test_find_free_port(): sock.close() -def test_server_info(on_disk_project): +def test_server_info(on_disk_project, in_memory_project): """Check the ServerInfo class behaviour.""" server_info = ServerInfo(on_disk_project, port=30000, pid=1234) server_info.save_pid_file() assert server_info.pid_file.exists() assert server_info.load_pid_file() == {"port": 30000, "pid": 1234} + server_info = ServerInfo.rejoin(on_disk_project) + assert server_info is not None + assert server_info.port == 30000 + assert server_info.pid == 1234 + server_info.delete_pid_file() assert not server_info.pid_file.exists() assert server_info.load_pid_file() is None + # in_memory_project does not expose a path and should raise an error + with pytest.raises(ValueError): + ServerInfo(in_memory_project, port=30000, pid=1234) + def test_launch(capsys, tmp_path): """Check the general behaviour of the launch function.""" From c217d51cf40b4af01959e19282eefc191c53b6ff Mon Sep 17 00:00:00 2001 From: Guillaume Lemaitre Date: Thu, 30 Jan 2025 11:44:55 +0100 Subject: [PATCH 57/57] update documentation --- skore/src/skore/project/project.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/skore/src/skore/project/project.py b/skore/src/skore/project/project.py index 9935f0380..a9b824d2b 100644 --- a/skore/src/skore/project/project.py +++ b/skore/src/skore/project/project.py @@ -41,6 +41,8 @@ class Project: ---------- path : Path The unified path of the project. + name : str + The name of the project. Corresponds to `path.name`. Examples --------