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.

Sign up for GitHub

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

Jump to bottom

[BUG] Make sure Client parameters are strings #1577

Merged
merged 5 commits into from
Feb 20, 2024
Merged

[BUG] Make sure Client parameters are strings #1577

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3302021
Make sure Client parameters are strings
beggers Dec 23, 2023
6cbe869
int port
beggers Dec 23, 2023
07c78b4
return new headers from stringify headers
beggers Dec 23, 2023
e3ce49c
Merge branch 'main' into beggers/stringify-port
beggers Dec 23, 2023
36d1d2b
Pr comments
beggers Jan 3, 2024
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
35 changes: 34 additions & 1 deletion chromadb/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ def EphemeralClient(
settings = Settings()
settings.is_persistent = False

# https://github.com/chroma-core/chroma/issues/1573
tenant = str(tenant)
database = str(database)

return ClientCreator(settings=settings, tenant=tenant, database=database)


Expand All @@ -135,6 +139,10 @@ def PersistentClient(
settings.persist_directory = path
settings.is_persistent = True

# https://github.com/chroma-core/chroma/issues/1573
tenant = str(tenant)
database = str(database)

return ClientCreator(tenant=tenant, database=database, settings=settings)


Expand Down Expand Up @@ -165,6 +173,14 @@ def HttpClient(
if settings is None:
settings = Settings()

# https://github.com/chroma-core/chroma/issues/1573
host = str(host)
port = str(port)
ssl = bool(ssl)
_stringify_headers(headers)
tenant = str(tenant)
database = str(database)

settings.chroma_api_impl = "chromadb.api.fastapi.FastAPI"
if settings.chroma_server_host and settings.chroma_server_host != host:
raise ValueError(
Expand Down Expand Up @@ -217,6 +233,14 @@ def CloudClient(
if settings is None:
settings = Settings()

# https://github.com/chroma-core/chroma/issues/1573
tenant = str(tenant)
database = str(database)
api_key = str(api_key)
cloud_host = str(cloud_host)
cloud_port = str(cloud_port)
enable_ssl = bool(enable_ssl)

settings.chroma_api_impl = "chromadb.api.fastapi.FastAPI"
settings.chroma_server_host = cloud_host
settings.chroma_server_http_port = cloud_port
Expand All @@ -242,9 +266,12 @@ def Client(

tenant: The tenant to use for this client. Defaults to the default tenant.
database: The database to use for this client. Defaults to the default database.

"""

# https://github.com/chroma-core/chroma/issues/1573
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to link to this issue in 4 separate places? just feels a little messy- id rather add one-liner with the rationale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed these all to a comment, lmk what you think. Happy to change.

tenant = str(tenant)
database = str(database)

return ClientCreator(tenant=tenant, database=database, settings=settings)


Expand All @@ -255,3 +282,9 @@ def AdminClient(settings: Settings = Settings()) -> AdminAPI:

"""
return AdminClientCreator(settings=settings)


def _stringify_headers(headers: Optional[Dict[str, str]]) -> None:
Copy link
Contributor

@rancomp rancomp Dec 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like we can be more suggestive with the type-hinting here. If headers is already Dict[str, str] then we would not need to cast. More explicitly,

Suggested change
def _stringify_headers(headers: Optional[Dict[str, str]]) -> None:
def _stringify_headers(headers: Optional[Dict[str, str | int]]) -> None:

I would have suggested a SupportsStr type but I think every(?) object in python has a __str__ method? see related exchange https://www.mail-archive.com/[email protected]/msg24545.html

if headers is not None:
for key, value in headers.items():
headers[key] = str(value)
Loading