-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Allow defining meaningful hostnames on multi-tesseract network #206
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -468,6 +468,17 @@ def serve( | |||||||
), | ||||||||
), | ||||||||
] = False, | ||||||||
service_names: Annotated[ | ||||||||
str | None, | ||||||||
typer.Option( | ||||||||
"--service-names", | ||||||||
help=( | ||||||||
"Comma-separated list of service names by which each tesseract should be exposed " | ||||||||
"in the shared network. " | ||||||||
"Tesseracts are reachable from one another at http://{service_name}:8000" | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
), | ||||||||
), | ||||||||
] = None | ||||||||
) -> None: | ||||||||
"""Serve one or more Tesseract images. | ||||||||
|
||||||||
|
@@ -491,6 +502,11 @@ def serve( | |||||||
else: | ||||||||
ports = None | ||||||||
|
||||||||
if service_names is not None: | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's check here if |
||||||||
service_names_list = service_names.split(',') | ||||||||
else: | ||||||||
service_names_list = None | ||||||||
|
||||||||
try: | ||||||||
project_id = engine.serve( | ||||||||
image_names, | ||||||||
|
@@ -501,6 +517,7 @@ def serve( | |||||||
propagate_tracebacks, | ||||||||
num_workers, | ||||||||
no_compose, | ||||||||
service_names_list, | ||||||||
) | ||||||||
except RuntimeError as ex: | ||||||||
raise UserError( | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,6 +9,7 @@ | |||||
import optparse | ||||||
import os | ||||||
import random | ||||||
import re | ||||||
import socket | ||||||
import string | ||||||
import tempfile | ||||||
|
@@ -544,6 +545,7 @@ def serve( | |||||
propagate_tracebacks: bool = False, | ||||||
num_workers: int = 1, | ||||||
no_compose: bool = False, | ||||||
service_names: list[str] | None = None, | ||||||
) -> str: | ||||||
"""Serve one or more Tesseract images. | ||||||
|
||||||
|
@@ -559,6 +561,7 @@ def serve( | |||||
WARNING: This may expose sensitive information, use with caution (and never in production). | ||||||
num_workers: number of workers to use for serving the Tesseracts. | ||||||
no_compose: if True, do not use Docker Compose to serve the Tesseracts. | ||||||
service_names: list of service names under which to expose each Tesseract container on the shared network. | ||||||
|
||||||
Returns: | ||||||
A string representing the Tesseract project ID. | ||||||
|
@@ -579,12 +582,23 @@ def serve( | |||||
f"Number of ports ({len(ports)}) must match number of images ({len(image_ids)})" | ||||||
) | ||||||
|
||||||
if service_names is not None: | ||||||
if len(service_names) != len(image_ids): | ||||||
raise ValueError( | ||||||
f"Number of service names ({len(service_names)}) must match number of images ({len(image_ids)})" | ||||||
) | ||||||
_validate_service_names(service_names) | ||||||
|
||||||
if no_compose: | ||||||
if len(images) > 1: | ||||||
raise ValueError( | ||||||
"Docker Compose is required to serve multiple Tesseracts. " | ||||||
f"Currently attempting to serve `{len(images)}` Tesseracts." | ||||||
) | ||||||
if service_names is not None: | ||||||
raise ValueError( | ||||||
"Tesseract service names are only meaningful with Docker Compose." | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
) | ||||||
args = [] | ||||||
container_port = "8000" | ||||||
args.extend(["--port", container_port]) | ||||||
|
@@ -640,6 +654,7 @@ def serve( | |||||
template = _create_docker_compose_template( | ||||||
image_ids, | ||||||
host_ip, | ||||||
service_names, | ||||||
ports, | ||||||
volumes, | ||||||
gpus, | ||||||
|
@@ -664,6 +679,7 @@ def serve( | |||||
def _create_docker_compose_template( | ||||||
image_ids: list[str], | ||||||
host_ip: str = "127.0.0.1", | ||||||
service_names: list[str] | None = None, | ||||||
ports: list[str] | None = None, | ||||||
volumes: list[str] | None = None, | ||||||
gpus: list[str] | None = None, | ||||||
|
@@ -673,6 +689,12 @@ def _create_docker_compose_template( | |||||
"""Create Docker Compose template.""" | ||||||
services = [] | ||||||
|
||||||
# Generate random service names for each image if not provided | ||||||
if service_names is None: | ||||||
service_names = [] | ||||||
for image_id in image_ids: | ||||||
service_names.append(f"{image_id.split(':')[0]}-{_id_generator()}") | ||||||
|
||||||
# Get random unique ports for each image if not provided | ||||||
if ports is None: | ||||||
ports = [] | ||||||
|
@@ -701,9 +723,9 @@ def _create_docker_compose_template( | |||||
else: | ||||||
gpu_settings = f"device_ids: {gpus}" | ||||||
|
||||||
for image_id, port in zip(image_ids, ports, strict=True): | ||||||
for service_name, image_id, port in zip(service_names, image_ids, ports, strict=True): | ||||||
service = { | ||||||
"name": f"{image_id.split(':')[0]}-{_id_generator()}", | ||||||
"name": service_name, | ||||||
"image": image_id, | ||||||
"port": f"{port}:8000", | ||||||
"volumes": volumes, | ||||||
|
@@ -752,6 +774,23 @@ def _parse_option(option: str): | |||||
return dict(_parse_option(opt) for opt in options) | ||||||
|
||||||
|
||||||
def _validate_service_names(service_names: list[str]) -> None: | ||||||
if len(set(service_names)) != len(service_names): | ||||||
raise ValueError("Service names must be unique") | ||||||
|
||||||
print(service_names) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
invalid_names = [] | ||||||
for name in service_names: | ||||||
print(name) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
if not re.match(r'^[A-Za-z0-9][A-Za-z0-9-]*$', name): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not even underscores? |
||||||
invalid_names.append(name) | ||||||
if len(invalid_names) != 0: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
raise ValueError( | ||||||
"Service names must contain only alphanumeric characters and hyphens, and must " | ||||||
f"not begin with a hyphen. Found invalid names: f{invalid_names}." | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
) | ||||||
|
||||||
|
||||||
def run_tesseract( | ||||||
image: str, | ||||||
command: str, | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -449,6 +449,55 @@ def test_tesseract_serve_with_volumes(built_image_name, tmp_path, docker_client) | |||||
assert run_res.exit_code == 0, run_res.stderr | ||||||
|
||||||
|
||||||
def test_tesseract_serve_interop(built_image_name, docker_client): | ||||||
cli_runner = CliRunner(mix_stderr=False) | ||||||
|
||||||
run_res = cli_runner.invoke( | ||||||
app, | ||||||
[ | ||||||
"serve", | ||||||
built_image_name, | ||||||
built_image_name, | ||||||
"--service-names", | ||||||
"T1,T2", | ||||||
], | ||||||
env={"COLUMNS": "1000"}, | ||||||
catch_exceptions=False, | ||||||
) | ||||||
assert run_res.exit_code == 0 | ||||||
|
||||||
project_meta = json.loads(run_res.stdout) | ||||||
project_id = project_meta["project_id"] | ||||||
project_containers = [project_meta["containers"][i]["name"] for i in range(2)] | ||||||
|
||||||
T1 = docker_client.containers.get(project_containers[0]) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use lower case for non-class variable names
Suggested change
|
||||||
|
||||||
try: | ||||||
returncode, stdout = T1.exec_run([ | ||||||
"python", | ||||||
"-c", | ||||||
"import requests; requests.get(\"http://T2:8000/health\").raise_for_status()" | ||||||
# I get a "tesseract_core has no attribute Tesseract" error from this?? | ||||||
#"import tesseract_core; tesseract_core.Tesseract.from_url(\"http://t-9000:8000\").health()" | ||||||
dionhaefner marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+480
to
+481
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is expected, see comment thread
Suggested change
|
||||||
]) | ||||||
assert returncode == 0, stdout | ||||||
except Exception as e: | ||||||
assert False, e | ||||||
Comment on lines
+484
to
+485
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to do anything, just let the exception bubble up.
Suggested change
|
||||||
finally: | ||||||
return | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This prevents cleanup from running! |
||||||
if project_id: | ||||||
run_res = cli_runner.invoke( | ||||||
app, | ||||||
[ | ||||||
"teardown", | ||||||
project_id, | ||||||
], | ||||||
catch_exceptions=False, | ||||||
) | ||||||
assert run_res.exit_code == 0, run_res.stderr | ||||||
Comment on lines
+488
to
+497
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest you use the |
||||||
|
||||||
|
||||||
|
||||||
@pytest.mark.parametrize("no_compose", [True, False]) | ||||||
def test_serve_nonstandard_host_ip( | ||||||
docker_client, built_image_name, docker_cleanup, free_port, no_compose | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.