-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[BUG]: Auth + custom headers + SSL-cert support for async client #2489
base: main
Are you sure you want to change the base?
[BUG]: Auth + custom headers + SSL-cert support for async client #2489
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
Need to update negative tests for auth + ssl |
b2e1067
to
a7b5caf
Compare
chromadb/test/conftest.py
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.
We may have to detect whether it is an async client in tests and wrap that in asyncio task.
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.
awesome
else: | ||
self._clients[loop_hash].headers.update(self._headers) |
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.
when would we need to update the headers like this?
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.
Tests were failing. I think it is due to having pytest use the same event loop - which caused async client to be reused with invalid credentials from prev test.
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.
but the clients aren't globally cached, the cache is local to the class instance--seems strange
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, it took some time to figure out what the issue was. I can dig deeper to find why pytest does this.
…sting client with stale headers - Added test case ids for easier traceability
3c51330
to
b339f0f
Compare
@codetheweb, still investigating some failures in async client close that seems to attempt an async call on a terminated loop |
Closes #2488
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
TBD