-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ENH] use httpx over requests #2336
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas |
docs/docs.trychroma.com/pages/integrations/hugging-face-server.md
Outdated
Show resolved
Hide resolved
@@ -1614,19 +1613,3 @@ def test_ssl_self_signed_without_ssl_verify(client_ssl): | |||
) | |||
client_ssl.clear_system_cache() | |||
assert "CERTIFICATE_VERIFY_FAILED" in "".join(stack_trace) | |||
|
|||
|
|||
def test_ssl_self_signed_with_verify_false(client_ssl): |
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.
httpx does not emit warnings when verify is explicitly set to False.
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.
does it error or give any indication?
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.
no, which imo is more correct behavior
@@ -62,27 +62,35 @@ def __init__(self, system: System): | |||
default_api_path=system.settings.chroma_server_api_default_path, | |||
) | |||
|
|||
self._session = requests.Session() | |||
self._session = httpx.Client(timeout=None) |
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.
checking (seems to be yes) default behavior was previously no timeout? We should probably add config here - let me cut a ticket.
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.
yep, requests doesn't have a default timeout but httpx does
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.
the default timeout was causing issues in CI
url = self._api_url + escaped_path | ||
|
||
response = self._session.request(method, url, **cast(Any, kwargs)) | ||
BaseHTTPClient._raise_chroma_error(response) |
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.
nice encapsulation
@@ -3,16 +3,16 @@ name = "chromadb-client" | |||
dynamic = ["version"] | |||
|
|||
authors = [ | |||
{ name="Jeff Huber", email="[email protected]" }, | |||
{ name="Anton Troynikov", email="[email protected]" } | |||
{ name = "Jeff Huber", email = "[email protected]" }, |
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.
whats with the formatting thrashing here lol - i've seen 2-3 prs where this file is only getting formatted.
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.
adding httpx here ;)
I just tested and VS Code added the formatting changes upon save, I assume because .vscode/settings.json->editor.formatOnSave
is true
not sure what formatter it's actually using for that though
No description provided.