-
Notifications
You must be signed in to change notification settings - Fork 115
[FEATURE] uv provider
#164
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 3 commits
939de3c
1108d8d
01a5835
b1d213d
ec693dc
f79eef8
eb6a7fe
857ed4b
8ebfdeb
ea3b1ec
f4ca8c7
1e626c5
8a8a4b3
0041641
ea492b5
c2fe6fd
701094e
42e59cf
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 | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,183 @@ | ||||||
| """Providers for launching Hugging Face Spaces via ``uv run``.""" | ||||||
|
|
||||||
| from __future__ import annotations | ||||||
|
|
||||||
| import os | ||||||
| import socket | ||||||
| import subprocess | ||||||
| import time | ||||||
| from dataclasses import dataclass, field | ||||||
| from typing import Dict, Optional | ||||||
|
|
||||||
| import requests | ||||||
|
|
||||||
| from .providers import ContainerProvider | ||||||
|
|
||||||
|
|
||||||
| def _poll_health(health_url: str, timeout_s: float) -> None: | ||||||
| """Poll a health endpoint until it returns HTTP 200 or times out.""" | ||||||
|
|
||||||
| deadline = time.time() + timeout_s | ||||||
| while time.time() < deadline: | ||||||
| try: | ||||||
| response = requests.get(health_url, timeout=2.0) | ||||||
| if response.status_code == 200: | ||||||
| return | ||||||
| except requests.RequestException: | ||||||
| pass | ||||||
burtenshaw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| time.sleep(0.5) | ||||||
|
|
||||||
| raise TimeoutError( | ||||||
| f"Server did not become ready within {timeout_s:.1f} seconds" | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| def _create_uv_command( | ||||||
| repo_id: str, | ||||||
| host: str, | ||||||
| port: int, | ||||||
| reload: bool, | ||||||
| project_url: Optional[str] = None, | ||||||
| ) -> list[str]: | ||||||
| command = [ | ||||||
| "uv", | ||||||
| "run", | ||||||
| "--project", | ||||||
| project_url or f"git+https://huggingface.co/spaces/{repo_id}", | ||||||
| "--", | ||||||
| "server", | ||||||
| "--host", | ||||||
| host, | ||||||
| "--port", | ||||||
| str(port), | ||||||
| ] | ||||||
| if reload: | ||||||
| command.append("--reload") | ||||||
| return command | ||||||
|
|
||||||
|
|
||||||
| @dataclass | ||||||
burtenshaw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| class UVProvider(ContainerProvider): | ||||||
|
||||||
| class UVProvider(ContainerProvider): | |
| class UVRuntime(RuntimeProvider): |
(naming suggestion to be aligned with the "runtime/" folder)
burtenshaw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
burtenshaw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
burtenshaw marked this conversation as resolved.
Show resolved
Hide resolved
burtenshaw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
burtenshaw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
I do like having start and stop methods not taking any argument. In fact I think that the abstract class should define these instead of start_container/stop_container. Now that we have two classes inheriting from ContainerProvider I would take advantage of it to factorize what can be (when it makes sense at least).
Same for wait_for_ready which should take any argument IMO.
Outdated
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.
I would let only 1 way to define the timeout (either in wait_for_ready or __init__ but not in both)
burtenshaw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
burtenshaw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
Same implementation as in docker provider (with different name). I would move the implementation to the parent class and reuse it.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,7 +17,7 @@ | |||||||||||||||||||||||||||||||||
| import requests | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| from .client_types import StepResult | ||||||||||||||||||||||||||||||||||
| from .containers.runtime import LocalDockerProvider | ||||||||||||||||||||||||||||||||||
| from .containers.runtime import LocalDockerProvider, UVProvider | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if TYPE_CHECKING: | ||||||||||||||||||||||||||||||||||
| from .containers.runtime import ContainerProvider | ||||||||||||||||||||||||||||||||||
|
|
@@ -106,22 +106,70 @@ def from_docker_image( | |||||||||||||||||||||||||||||||||
| return cls(base_url=base_url, provider=provider) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||||||
| def from_hub(cls: Type[EnvClientT], repo_id: str, provider: Optional["ContainerProvider"] = None, **kwargs: Any) -> EnvClientT: | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| Create an environment client by pulling from a Hugging Face model hub. | ||||||||||||||||||||||||||||||||||
| def from_hub( | ||||||||||||||||||||||||||||||||||
| cls: Type[EnvClientT], | ||||||||||||||||||||||||||||||||||
| repo_id: str, | ||||||||||||||||||||||||||||||||||
| *, | ||||||||||||||||||||||||||||||||||
| use_docker: bool = False, | ||||||||||||||||||||||||||||||||||
| provider: Optional["ContainerProvider"] = None, | ||||||||||||||||||||||||||||||||||
| host: str = "0.0.0.0", | ||||||||||||||||||||||||||||||||||
| port: Optional[int] = None, | ||||||||||||||||||||||||||||||||||
| reload: bool = False, | ||||||||||||||||||||||||||||||||||
| timeout_s: float = 60.0, | ||||||||||||||||||||||||||||||||||
| runner: Optional[UVProvider] = None, | ||||||||||||||||||||||||||||||||||
| project_url: Optional[str] = None, | ||||||||||||||||||||||||||||||||||
| connect_host: Optional[str] = None, | ||||||||||||||||||||||||||||||||||
| extra_env: Optional[Dict[str, str]] = None, | ||||||||||||||||||||||||||||||||||
| **provider_kwargs: Any, | ||||||||||||||||||||||||||||||||||
| ) -> EnvClientT: | ||||||||||||||||||||||||||||||||||
|
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 find this signature very hard to understand without context since it's mixing kwargs for docker and for uv. Also I don't think one needs 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. My suggestion would be to remove from typing import Any, Dict, NotRequired, TypedDict, Unpack, overload
class DockerKwargs(TypedDict, total=False):
tag: NotRequired[str]
env: NotRequired[Dict[str, str]]
class UVProvider(TypedDict):
host: NotRequired[str]
port: NotRequired[int]
reload: NotRequired[bool]
timeout_s: NotRequired[float]
env: NotRequired[Dict[str, str]]
@overload
def from_hub(repo_id: str, *, use_docker: bool = True, **kwargs: Unpack[DockerKwargs]) -> str: ...
@overload
def from_hub(repo_id: str, *, use_docker: bool = False, **kwargs: Unpack[UVProvider]) -> str: ...
def from_hub(repo_id: str, *, use_docker: bool = False, **kwargs: Any) -> str:
raise NotImplementedError()
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. Note that I'm not a fan of overloads and typed dict but it's the only solution I see to correctly document the signature while keeping a single method. Another solution is to have
Collaborator
Author
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. Thanks for this. I've simplified the signatures right down, but I haven't added type overloading in this PR. |
||||||||||||||||||||||||||||||||||
| """Create a client from a Hugging Face Space. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Set ``use_docker=True`` to launch the registry image with a container | ||||||||||||||||||||||||||||||||||
| provider. The default ``use_docker=False`` runs the Space locally using | ||||||||||||||||||||||||||||||||||
| ``uv run`` through :class:`UVProvider`. | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if provider is None: | ||||||||||||||||||||||||||||||||||
| provider = LocalDockerProvider() | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if "tag" in kwargs: | ||||||||||||||||||||||||||||||||||
| tag = kwargs["tag"] | ||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||
| tag = "latest" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| base_url = f"registry.hf.space/{repo_id.replace('/', '-')}:{tag}" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return cls.from_docker_image(image=base_url, provider=provider) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if use_docker: | ||||||||||||||||||||||||||||||||||
| if provider is None: | ||||||||||||||||||||||||||||||||||
| provider = LocalDockerProvider() | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| tag = provider_kwargs.pop("tag", "latest") | ||||||||||||||||||||||||||||||||||
| image = provider_kwargs.pop( | ||||||||||||||||||||||||||||||||||
| "image", | ||||||||||||||||||||||||||||||||||
burtenshaw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||
| f"registry.hf.space/{repo_id.replace('/', '-')}:" f"{tag}", | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| base_url = provider.start_container(image, **provider_kwargs) | ||||||||||||||||||||||||||||||||||
| provider.wait_for_ready(base_url, timeout_s=timeout_s) | ||||||||||||||||||||||||||||||||||
| return cls(base_url=base_url, provider=provider) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| uv_runner = runner or UVProvider( | ||||||||||||||||||||||||||||||||||
| repo_id=repo_id, | ||||||||||||||||||||||||||||||||||
| host=host, | ||||||||||||||||||||||||||||||||||
| port=port, | ||||||||||||||||||||||||||||||||||
| reload=reload, | ||||||||||||||||||||||||||||||||||
| project_url=project_url, | ||||||||||||||||||||||||||||||||||
| connect_host=connect_host, | ||||||||||||||||||||||||||||||||||
| extra_env=extra_env, | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
| uv_runner = runner or UVProvider( | |
| repo_id=repo_id, | |
| host=host, | |
| port=port, | |
| reload=reload, | |
| project_url=project_url, | |
| connect_host=connect_host, | |
| extra_env=extra_env, | |
| ) | |
| runner = UVProvider( | |
| project=f"git+https://huggingface.co/spaces/{repo_id}", | |
| host=host, | |
| port=port, | |
| reload=reload, | |
| env=env, | |
| ) |
With all the suggestions above, that would be the runner call (i.e. provide a project url, remove connect_host, remove project_url, rename extra_env to env, and remove runner). I don't think final user loose much in this simplification.
Outdated
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.
Once the runner is defined and instantiated, I feel that the start and wait calls should be made in the __init__ instead. This way you get the same behavior both in from_hub and from_docker. With current implementation, one stops the runner if failing to start, the other don't (which is not consistent).


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.
Since
uvis not per-se a container, I would rename the abstract classRuntimeProvider(or simplyRuntime?) and the abstract methodsstart,stop, andwait_for_ready(instead ofstart_container, etc.). I feel that semantically it would be more accurate.(just my 2c, feel free to ignore 🤗 )
Uh oh!
There was an error while loading. Please reload this page.
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.
I agree with you @Wauplin , we should clarify naming here. However, I'm reluctant to let this PR swell whilst we're trying to move fast.
For now, I've added a new
ABCnamedRuntimeProviderwhich has no container specific methods and is used by theUVProvider.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.
Understandable! Works like this as well :)