-
Notifications
You must be signed in to change notification settings - Fork 2k
[ENH] Expose host and port to CloudClient constructor #5997
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
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Expose configurable host/port in cloud JS clients Adds optional Key Changes• Extended Affected Areas• clients/new-js/packages/chromadb/src/cloud-client.ts This summary was automatically generated by @propel-code-bot |
| args: Partial<{ | ||
| /** API key for authentication (or set CHROMA_API_KEY env var) */ | ||
| apiKey?: string; | ||
| /** Host address of the Chroma cloud server. Defaults to 'api.trychroma.com' */ | ||
| host?: string; | ||
| /** Port number of the Chroma cloud server. Defaults to 443 */ | ||
| port?: number; | ||
| /** Additional fetch options for HTTP requests */ | ||
| fetchOptions?: RequestInit; | ||
| }> = {}, |
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.
[Maintainability] To improve maintainability and reduce code duplication, consider defining a shared interface for the cloud client constructor arguments. Both CloudClient and AdminCloudClient now share several configuration options (apiKey, host, port, fetchOptions).
You could create a base interface for these common options and then extend it for CloudClient, which has additional properties.
For example:
interface BaseCloudClientArgs {
/** API key for authentication (or set CHROMA_API_KEY env var) */
apiKey?: string;
/** Host address of the Chroma cloud server. Defaults to 'api.trychroma.com' */
host?: string;
/** Port number of the Chroma cloud server. Defaults to 443 */
port?: number;
/** Additional fetch options for HTTP requests */
fetchOptions?: RequestInit;
}
interface CloudClientArgs extends BaseCloudClientArgs {
/** Tenant name for multi-tenant deployments */
tenant?: string;
/** Database name to connect to */
database?: string;
}This would allow you to use Partial<CloudClientArgs> and Partial<BaseCloudClientArgs> in the respective constructors, making the code more DRY.
Context for Agents
To improve maintainability and reduce code duplication, consider defining a shared interface for the cloud client constructor arguments. Both `CloudClient` and `AdminCloudClient` now share several configuration options (`apiKey`, `host`, `port`, `fetchOptions`).
You could create a base interface for these common options and then extend it for `CloudClient`, which has additional properties.
For example:
```typescript
interface BaseCloudClientArgs {
/** API key for authentication (or set CHROMA_API_KEY env var) */
apiKey?: string;
/** Host address of the Chroma cloud server. Defaults to 'api.trychroma.com' */
host?: string;
/** Port number of the Chroma cloud server. Defaults to 443 */
port?: number;
/** Additional fetch options for HTTP requests */
fetchOptions?: RequestInit;
}
interface CloudClientArgs extends BaseCloudClientArgs {
/** Tenant name for multi-tenant deployments */
tenant?: string;
/** Database name to connect to */
database?: string;
}
```
This would allow you to use `Partial<CloudClientArgs>` and `Partial<BaseCloudClientArgs>` in the respective constructors, making the code more DRY.
File: clients/new-js/packages/chromadb/src/cloud-client.ts
Line: 76| super({ | ||
| host: "api.trychroma.com", | ||
| port: 8000, | ||
| host: args.host ?? "api.trychroma.com", |
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.
maybe use || here to handle empty string fallback
e06f95a to
0fd1782
Compare
Defaults them to
api.trychroma.comat443