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]: Thin client imports #2466

Merged
merged 5 commits into from
Jul 8, 2024
Merged

[BUG]: Thin client imports #2466

merged 5 commits into from
Jul 8, 2024

Conversation

tazarov
Copy link
Contributor

@tazarov tazarov commented Jul 6, 2024 •
edited
Loading

Description of changes

Closes #2459

Summarize the changes made by this PR.

  • Improvements & Bug fixes
    • Fixed imports for onnxruntime and auth module to avoid import failures for the thin client

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 6, 2024

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@tazarov Graphite App
Copy link
Contributor Author

tazarov commented Jul 6, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tazarov and the rest of your teammates on Graphite Graphite

@tazarov tazarov requested a review from codetheweb July 6, 2024 14:21
@tazarov tazarov added the bug Something isn't working label Jul 7, 2024
codetheweb
codetheweb approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@codetheweb codetheweb left a comment

Choose a reason for hiding this comment

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

looks good, is there a way to add a test to ensure this doesn't happen again?

chromadb/auth/token_authn/__init__.py Outdated Show resolved Hide resolved
@tazarov
fix: Dynamic import of HTTPException
f9196b5 
- Added a step to verify thin client package import works
@tazarov
Copy link
Contributor Author

tazarov commented Jul 8, 2024

looks good, is there a way to add a test to ensure this doesn't happen again?

I've added a test-package.sh step after the thin client build that should install the package and import it.

We can try to refactor the CI for client tests so that it uses deps from clients/python/ instead of the core project.

@codetheweb
Copy link
Contributor

looks good, is there a way to add a test to ensure this doesn't happen again?

I've added a test-package.sh step after the thin client build that should install the package and import it.

We can try to refactor the CI for client tests so that it uses deps from clients/python/ instead of the core project.

oh sorry I missed that, that seems perfect

@tazarov tazarov merged commit f132a89 into main Jul 8, 2024
66 checks passed
HammadB
HammadB reviewed Jul 8, 2024
View reviewed changes
chromadb/server/fastapi/__init__.py
@@ -341,7 +341,7 @@ def auth_and_get_tenant_and_database_for_request(
if not self.authn_provider:
return (tenant, database)

user_identity = self.authn_provider.authenticate_or_raise(headers)
user_identity = self.authn_provider.authenticate_or_raise(dict(headers))
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to fix a header check that was previously self._token_transport_header.value not in headers, which doesn't work with dict.

HammadB
HammadB reviewed Jul 8, 2024
View reviewed changes
chromadb/auth/token_authn/__init__.py
@@ -232,4 +231,6 @@ def authenticate_or_raise(self, headers: Headers) -> UserIdentity:
time.sleep(
random.uniform(0.001, 0.005)
) # add some jitter to avoid timing attacks
from fastapi import HTTPException
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to import in the hot path here? Why do we need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good point. I had it initially in the __init__ as a self.HTTPException. Shall I move it back?

Copy link
Contributor Author

@tazarov tazarov Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@HammadB, address this separately here - #2477 (a more elegant solution to the import problem)

HammadB
HammadB reviewed Jul 8, 2024
View reviewed changes
chromadb/auth/basic_authn/__init__.py
@@ -100,11 +102,11 @@ def __init__(self, system: System) -> None:
"BasicAuthenticationServerProvider.authenticate", OpenTelemetryGranularity.ALL
)
@override
def authenticate_or_raise(self, headers: Headers) -> UserIdentity:
def authenticate_or_raise(self, headers: Dict[str, str]) -> UserIdentity:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a breaking change for anyone overriding auth. We should be more careful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. But the use of starlette and fastapi here was already a breaking change for the thin client, which did not require (or import) either of those. Technically, duck-typing helps us here too.

@HammadB HammadB mentioned this pull request Jul 10, 2024
Merged
1 task
HammadB added a commit that referenced this pull request Jul 10, 2024
@HammadB
[BUG][TST] Fix thin client package test (#2497)
8b546be 
## Description of changes

*Summarize the changes made by this PR.*
 - Improvements & Bug fixes
	 - Fix thin client package bug introduced by #2466 by @tazarov 
- Do this as opposed to #2490 - as it is cleaner and more explicit.
Making it less brittle to changes.
 - New functionality
	 - ..

## Test plan
*How are these changes tested?*

- [ ] Tests pass locally with `pytest` for python, `yarn test` for js,
`cargo test` for rust

## Documentation Changes
*Are all docstrings for user-facing APIs updated if required? Do we need
to make documentation changes in the [docs
repository](https://github.com/chroma-core/docs)?*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HammadB HammadB HammadB left review comments

@codetheweb codetheweb codetheweb approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: chromadb-client: ModuleNotFoundError
3 participants
@tazarov @codetheweb @HammadB