Skip to content
Merged
10 changes: 10 additions & 0 deletions packaging/nginx.conf.template
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ http {
proxy_send_timeout 360000s;
proxy_read_timeout 360000s;
}

location /management {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good job using the existing API 👍

client_max_body_size 16G;
proxy_pass http://localhost:9000;
proxy_http_version 1.1;

proxy_connect_timeout 360000s;
proxy_send_timeout 360000s;
proxy_read_timeout 360000s;
}
}
}

Expand Down
29 changes: 28 additions & 1 deletion python-client/giskard/commands/cli_server.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import logging
import os
import time
from pathlib import Path
from typing import Optional
from urllib.parse import urlparse
Expand All @@ -25,7 +26,6 @@
giskard_settings_path = settings.home_dir / "server-settings.yml"
IMAGE_NAME = "docker.io/giskardai/giskard"


def create_docker_client() -> DockerClient:
try:
return docker.from_env()
Expand Down Expand Up @@ -96,6 +96,30 @@ def get_container(version=None, quit_if_not_exists=True) -> Optional[Container]:
return None


def _backend_ready(endpoint) -> bool:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_is_backend_ready is a bit cleaner IMO

try:
response = requests.get(endpoint)

if response.status_code != 200:
return False

return "UP" == response.json()['components']['readinessState']['status']

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm testing on dev setup, but my json structure isn't the same, I don't have components readinessState

image

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I got a different format, will use status instead to be sure.

image

except OSError:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

requests can throw other errors (ConnectTimeout for example), I think we can simply catch BaseException and return False if it happens. (make sure to handle KeyboardInterrupt separately and reraise click.Abort())

In this case you can even replace

        if response.status_code != 200:
            return False

with

response.raise_for_status()

since it'll be caught by the same BaseException handler

return False
except KeyError:
return False


def _await_backend_ready(port: int):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: await has an asynchronous connotation in python (async/await). There's also an async sleep counterpart that'd be used for it: await asyncio.sleep()
In order to remove the confusion I'd rename it to just _wait_backend_ready

Suggested change
def _await_backend_ready(port: int):
def _wait_backend_ready(port: int):

endpoint = f"http://localhost:{port}/management/health"
backoff_time = 2
up = False

while not up:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should have a waiting limit. Let's say we wait for 3 minutes and if _backend_ready still returns False we break the loop and print an explanation like:

Giskard backend takes unusually long time to start, please check the logs with `giskard server logs backend`

Also I think it's worth adding a feedback while waiting, for example printing dots every backoff_time seconds, like this it's clear that the process isn't frozen, but is actually checking the status

click.echo(".", nl=False)

should do the trick, because print(end='') won't work in the loop without explicit stdout flushing

time.sleep(backoff_time)
up = _backend_ready(endpoint)


def _start(attached=False, version=None):
logger.info("Starting Giskard Server")

Expand All @@ -120,6 +144,9 @@ def _start(attached=False, version=None):
volumes={home_volume.name: {"bind": "/home/giskard/datadir", "mode": "rw"}},
)
container.start()

_await_backend_ready(port)

logger.info(f"Giskard Server {version} started. You can access it at http://localhost:{port}")


Expand Down