Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: solara run should only print url when the server is running #776

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions solara/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
from enum import Enum
from pathlib import Path

import rich
import rich_click as click
import uvicorn
from rich import print as rprint
Expand All @@ -18,6 +17,7 @@
import solara
from solara.server import settings
import solara.server.threaded
import solara.server.server

from .server import telemetry

Expand Down Expand Up @@ -361,16 +361,19 @@ def run(
# # window.show()
# webview.start(debug=True)

def open_browser():
while not failed and (server is None or not server.started):
def check_server_started():
while not failed and (server is None or not server.started) or not solara.server.server.is_ready(url):
Copy link
Collaborator

@iisakkirotko iisakkirotko Sep 6, 2024

Choose a reason for hiding this comment

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

So I think I've solved my second issue

if I run solara run solara.website.pages, I get an internal server error, because of:

This is due to the change in import order. Because we now import solara.server.server, which in turn imports from . import app, app.py runs before this line. Then app.apps is empty, and the server can't start.

For a solution, we could import solara.server.server here instead of at the top, although I'm not sure if this could introduce a race condition of some sort.

Suggested change
while not failed and (server is None or not server.started) or not solara.server.server.is_ready(url):
import solara.server.server
while not failed and (server is None or not server.started) or not solara.server.server.is_ready(url):

Another option would be to move this check down to below where we set environmental variables.

time.sleep(0.1)
if not failed:
webbrowser.open(url)
# remove the Solara server is starting ... line and print the url (in green)
print(f"\r\x1b[1;32mSolara server is running at {url}\x1b[0m")
Comment on lines +368 to +369
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be nicer to leave the Solara server is starting... in, and print Solara server started at {url} in green after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, maybe the \r can be removed.

if open:
webbrowser.open(url)

if open:
threading.Thread(target=open_browser, daemon=True).start()
threading.Thread(target=check_server_started, daemon=True).start()

rich.print(f"Solara server is starting at {url}")
# print in yellow that the server is starting
print("\x1b[1;33mSolara server is starting...\x1b[0m", end="", flush=True)

if log_level is not None:
LOGGING_CONFIG["loggers"]["solara"]["level"] = log_level.upper()
Expand All @@ -396,7 +399,7 @@ def open_browser():
settings.main.tracer = tracer
settings.main.timing = timing
items = (
"theme_variant_user_selectable dark theme_variant theme_loader use_pdb server open_browser open url failed dev tracer"
"theme_variant_user_selectable dark theme_variant theme_loader use_pdb server check_server_started open url failed dev tracer"
" timing ssg search check_version production".split()
)
for item in items:
Expand Down
Loading