JS client parity and broken out tests #292
Conversation
jeffchuber
changed the title
| @@ -70,6 +70,7 @@ def __init__(self, settings): | |||
| self.router.add_api_route("/api/v1", self.root, methods=["GET"]) | |||
| self.router.add_api_route("/api/v1/reset", self.reset, methods=["POST"]) | |||
| self.router.add_api_route("/api/v1/version", self.version, methods=["GET"]) | |||
| self.router.add_api_route("/api/v1/heartbeat", self.heartbeat, methods=["GET"]) | |||
There was a problem hiding this comment.
added an explicit url for this to make it easy for our OpenAPI spec to pick it up
There was a problem hiding this comment.
the python frontend is still using the "old" route... self.router.add_api_route("/api/v1", self.root, methods=["GET"]), but we could move it over to the new one as well. perhaps it makes sense to do that in this PR? Though having isolation b/t JS and python changes is nice.
| "db:clean": "cd ../.. && docker-compose -f docker-compose-js-tests.yml down --volumes", | ||
| "db:run": "cd ../.. && docker-compose -f docker-compose-js-tests.yml up --detach && sleep 5", | ||
| "db:clean": "cd ../.. && docker-compose -f docker-compose.test.yml down --volumes", | ||
| "db:run": "cd ../.. && docker-compose -f docker-compose.test.yml up --detach && sleep 5", |
There was a problem hiding this comment.
we removed an "extra" docker file, but didnt fix this up.
| public async query( | ||
| query_embeddings: number[] | number[][] | undefined, | ||
| n_results: number = 10, | ||
| where?: object, | ||
| query_text?: string | string[], | ||
| query_text?: string | string[], // TODO: should be named query_texts to match python API |
There was a problem hiding this comment.
Should we complete this TODO?
There was a problem hiding this comment.
this would cause a breaking change in the API, and I was trying to constrain this diff to have no breaking changes.
I think we will soon move over to object-based inputs, eg as laid out here #254, and i was planning on making that change then.
thoughts?
| } | ||
|
|
||
| public async persist() { | ||
| throw new Error("Not implemented in JS client") |
There was a problem hiding this comment.
If a user is connected to a backend with duckdb+parquet, do we want this to work?
There was a problem hiding this comment.
I don't think right now since that is not a formal path we want to support (standing up in-memory chroma as a backend service). What do you think?
| const collection = await chroma.createCollection('test') | ||
| await collection.add(IDS, EMBEDDINGS) | ||
| const count = await collection.count() | ||
| expect(count).toBe(3) |
There was a problem hiding this comment.
can we get the embeddings and verify their contents?
| const embedding = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] | ||
| const metadata = { test: 'test' } | ||
| await collection.add(id, embedding, metadata) | ||
| const count = await collection.count() |
There was a problem hiding this comment.
Can we get the embeddings and verify their contents?
| await chroma.reset() | ||
| const collection = await chroma.createCollection('test') | ||
| expect(collection).toBeDefined() | ||
| expect(collection).toHaveProperty('name') |
There was a problem hiding this comment.
can we verify the name is correct?
| const collection = await chroma.createCollection('test') | ||
| expect(collection).toBeDefined() | ||
| expect(collection).toHaveProperty('name') | ||
| let collections = await chroma.listCollections() |
There was a problem hiding this comment.
Can we test non-null metadata?
|
@HammadB i went through every test and added verification of values for every test |
This PR adds full JS client parity with the python client and also breaks out tests into individual files.
Should likely be landed after #297 - so that CI can pick up the tests
After merge, a separate PR will be created to bump the npm version number and cut a new release.