feat: Make open to spin-off UI server and to be used in CLI and notebook
#1157
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Coverage Report for backend
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rouk1
reviewed
Jan 20, 2025
rouk1
reviewed
Jan 20, 2025
rouk1
reviewed
Jan 20, 2025
rouk1
reviewed
Jan 27, 2025
There was a problem hiding this comment.
Tested in jupyter, vscode interactive, and script.
jupyter: works well
vscode interactive: works well except log orders
script: works well except log orders
exemple of strange log orders
──────────────────────────────────────── skore ─────────────────────────────────────────
Project file '/Users/rouk1/dev/skore/project.skore' was successfully created.
──────────────────────────────────────── skore ─────────────────────────────────────────
Project file '/Users/rouk1/dev/skore/project.skore' was successfully created.
─────────────────────────────────────── skore-UI ───────────────────────────────────────
Server is already running at http://localhost:22140
Processing cross-validation ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100%
for Lasso
Processing column 3 / 3
Processing column 8 / 8
yeah
─────────────────────────────────────── skore-UI ───────────────────────────────────────
Running skore UI from 'project.skore' at URL http://localhost:22140 (Press CTRL+C to
quit)
^C─────────────────────────────────────── skore-UI ───────────────────────────────────────
Server that was running at http://localhost:22140 has been closed gracefully
Processing cross-validation ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100%
for Lasso
Processing column 3 / 3
Processing column 8 / 8
yeah
rouk1
reviewed
Jan 28, 2025
There was a problem hiding this comment.
This works fine in a interactive and in jupyter, but when running it from a script the keep_alive argument seems ignored.
# %%
from pathlib import Path
import skore
repo_root = Path(__file__).resolve().parent.parent
project_path = repo_root / "mini.skore"
p = skore.open(project_path, overwrite=True, keep_alive=True)
p.put("markdown", "# yo markdown\n- a\n- b")
p.put("dict", {"a": "zertyuio"})
# %%
p.shutdown_web_ui()Here is the output I get (I did not hit CTRL+C)
(venv) ➜ skore git:(is/1002) python scratch/mini.py
──────────────────────────────────────── skore ─────────────────────────────────────────
Project file '/Users/rouk1/dev/skore/mini.skore' was successfully created.
─────────────────────────────────────── skore-UI ───────────────────────────────────────
Running skore UI from 'mini.skore' at URL http://localhost:22140
─────────────────────────────────────── skore-UI ───────────────────────────────────────
Server that was running at http://localhost:22140 has been closed gracefully
Press Ctrl+C to stop the server...
(venv) ➜ skore git:(is/1002)
Sorry if that was not ready for re-review : )
ogrisel
reviewed
Jan 28, 2025
| ] | ||
|
|
||
| process = subprocess.Popen( | ||
| cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True |
There was a problem hiding this comment.
Don't you need to use /dev/null for stdin, stdout and stderr to detach the server process from the parent process and make it survive after jupyter kernel restart or script ending (daemon-like process)?
There was a problem hiding this comment.
I manually handle it:
if keep_alive:
atexit.register(block_before_cleanup, project, process)
else:
atexit.register(cleanup_server, project)In this case, I can rejoin the server process and request a KeyboardInterrupt to cleanup. I find it explicit.
make it survive after jupyter kernel restart or script ending
I would expect this behaviour for the script but it was not my first thought for jupyter: I would expect that when restarting the kernel, I'll go and rerun a set of cell and I'll re-execute the skore.open that is going to restart the server.
But I'm open to changes on this side.
My bad it works fine in script aswell when you do not stop manually the server. |
glemaitre
commented
Jan 29, 2025
There was a problem hiding this comment.
OK @rouk1 I did not change anything regarding the implementation but I added more tests for _environment.py or server.py and the kill CLI function.
I remove the launch and replace it open.
I think we can make a follow up PR just for the documentation.
thomass-dev
reviewed
Jan 30, 2025
rouk1
approved these changes
Jan 30, 2025
thomass-dev
reviewed
Jan 30, 2025
| @@ -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)(create|open|kill)(?=\s+)", | |||
There was a problem hiding this comment.
Suggested change
| r"(?<=\s)(create|open|kill)(?=\s+)", | |
| r"(?<=\s)(open|kill)(?=\s+)", |
There was a problem hiding this comment.
Nitpick: create doesn't exist anymore.
thomass-dev
approved these changes
Jan 30, 2025
|
🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #1002
This PR proposed to resolve the issue discussed in #1002:
openshould provide the possibility to spin-off a UI serveropencan therefore be used in the notebook and with the CLIkillto kill all alive servers (normally we tested that we should not have some case or zombie processes).So in short, we allow for the following:
Implementation details
We launch the UI in a separate process. Why process: if we would attach to the async loop for the notebook or run in a separate thread from the main process, we might get into trouble when this main process starts to execute large computation making the UI unresponsive.
So we are starting a new process with the webserver inside. For the moment, we expect to have 1 skore project associated with 1 skore UI server. So we don't allow to spin a new webserver if one already exist and is associated with a running skore project. Instead, we provide the link to the server.
Also, we have a
rejoinmechanism: if a project was closed but the UI was not, then when opening a project, we reattach the web server to it.Finally, we found with @rouk1 that you have the following use-cases:
serve=Falsewhen callingopenserve=Trueand in this case, you most probably want to keep the webs server alive (so deamon + atexit is not what we want)serve=Trueand you will be interested to have access to the UI the time that the notebook is alive. The current setup will kill the web server.So we added a
keep_aliveoption that detect if you are in a specific environment and choose a sensible default but it can be easily overwritten.Notes
Some of the covering is not really working with the multiprocessing because we explicitly check for the console message in the test and the coverage mentioned that we don't cover it.
TODO
keep_aliveoptionkillcli