-
Notifications
You must be signed in to change notification settings - Fork 453
Add Chroma Retrieval #323 #324
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?
Conversation
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.
thanks for this!!! added a very clarification requests/comments
port: Optional[int] = None | ||
ssl: Optional[bool] = False | ||
n_results: Optional[int] = 4 | ||
embedding_function: Optional[Any] = 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.
should this be typed? also im thinking it shouldnt be optional?
host: Optional[str] = None | ||
port: Optional[int] = None | ||
ssl: Optional[bool] = False | ||
n_results: Optional[int] = 4 |
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.
suggest higher default value like 10?
results = await self.retrieve(text) | ||
return self.combine_retrieval_results(results) | ||
|
||
async def retrieve_and_generate(self, text: str) -> Any: |
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.
what does this do and is there a reason it cant be implemented?
@@ -40,6 +40,7 @@ | |||
"@aws-sdk/util-dynamodb": "^3.621.0", | |||
"@libsql/client": "^0.14.0", | |||
"axios": "^1.7.2", | |||
"chromadb": "^1.8.1", |
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.
why did you set 1.8.1 here? the latest version is 2.4.3
@@ -49,6 +50,7 @@ | |||
"devDependencies": { | |||
"@types/jest": "^29.5.12", | |||
"@types/mocha": "^10.0.7", | |||
"@types/node": "^20.11.30", |
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.
was this a required change?
port?: number; | ||
ssl?: boolean; | ||
nResults?: number; | ||
embeddingFunction?: any; |
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 as python - should this be required and typed?
|
||
const results = await this.collection.query({ | ||
queryTexts: [text], | ||
nResults: this.options.nResults || 4 |
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.
bump default results to 10?
} | ||
|
||
/** | ||
* Placeholder for retrieve and generate functionality |
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 question as python
setup( | ||
install_requires=[ |
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.
will the maintainers be ok with this?
Issue Link (REQUIRED)
#323
Fixes #323
Summary
Changes
ChromaRetriever
class andChromaRetrieverOptions
classChromaRetriever
class andChromaRetrieverOptions
interfaceUser experience
Before:
After:
Usage examples:
Python:
TypeScript:
Checklist
Is this a breaking change?
RFC issue number: N/A
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.