Skip to content

Commit

Permalink
[CLN] Make delete return None (#2880)
Browse files Browse the repository at this point in the history
## Description of changes

The `Collection.delete` API seemingly returns None, but our internal
implementation returns a list of the IDs that were deleted. This PR
makes all `delete` implementations return `None` for consistency.

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

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

## Documentation Changes
The documentation does not mention the API returning a list of deleted
IDs, so it is consistent with the expected behavior.
  • Loading branch information
itaismith authored Oct 10, 2024
1 parent c9d8855 commit 4ae5543
Show file tree
Hide file tree
Showing 11 changed files with 35 additions and 34 deletions.
2 changes: 1 addition & 1 deletion chromadb/api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ def _delete(
ids: Optional[IDs],
where: Optional[Where] = {},
where_document: Optional[WhereDocument] = {},
) -> IDs:
) -> None:
"""[Internal] Deletes entries from a collection specified by UUID.
Args:
Expand Down
2 changes: 1 addition & 1 deletion chromadb/api/async_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ async def _delete(
ids: Optional[IDs],
where: Optional[Where] = {},
where_document: Optional[WhereDocument] = {},
) -> IDs:
) -> None:
"""[Internal] Deletes entries from a collection specified by UUID.
Args:
Expand Down
4 changes: 2 additions & 2 deletions chromadb/api/async_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,8 +356,8 @@ async def _delete(
ids: Optional[IDs],
where: Optional[Where] = {},
where_document: Optional[WhereDocument] = {},
) -> IDs:
return await self._server._delete(
) -> None:
await self._server._delete(
collection_id=collection_id,
ids=ids,
where=where,
Expand Down
7 changes: 3 additions & 4 deletions chromadb/api/async_fastapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -404,14 +404,13 @@ async def _delete(
ids: Optional[IDs] = None,
where: Optional[Where] = {},
where_document: Optional[WhereDocument] = {},
) -> IDs:
resp_json = await self._make_request(
) -> None:
await self._make_request(
"post",
"/collections/" + str(collection_id) + "/delete",
json={"where": where, "ids": ids, "where_document": where_document},
)

return cast(IDs, resp_json)
return None

@trace_method("AsyncFastAPI._submit_batch", OpenTelemetryGranularity.ALL)
async def _submit_batch(
Expand Down
4 changes: 2 additions & 2 deletions chromadb/api/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,8 @@ def _delete(
ids: Optional[IDs],
where: Optional[Where] = {},
where_document: Optional[WhereDocument] = {},
) -> IDs:
return self._server._delete(
) -> None:
self._server._delete(
collection_id=collection_id,
ids=ids,
where=where,
Expand Down
6 changes: 3 additions & 3 deletions chromadb/api/fastapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -365,9 +365,9 @@ def _delete(
ids: Optional[IDs] = None,
where: Optional[Where] = {},
where_document: Optional[WhereDocument] = {},
) -> IDs:
) -> None:
"""Deletes embeddings from the database"""
resp_json = self._make_request(
self._make_request(
"post",
"/collections/" + str(collection_id) + "/delete",
json={
Expand All @@ -376,7 +376,7 @@ def _delete(
"where_document": where_document,
},
)
return cast(IDs, resp_json)
return None

@trace_method("FastAPI._submit_batch", OpenTelemetryGranularity.ALL)
def _submit_batch(
Expand Down
5 changes: 2 additions & 3 deletions chromadb/api/segment.py
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ def _delete(
ids: Optional[IDs] = None,
where: Optional[Where] = None,
where_document: Optional[WhereDocument] = None,
) -> IDs:
) -> None:
add_attributes_to_current_span(
{
"collection_id": str(collection_id),
Expand Down Expand Up @@ -646,7 +646,7 @@ def _delete(
ids_to_delete = ids

if len(ids_to_delete) == 0:
return []
return

records_to_submit = list(
_records(operation=t.Operation.DELETE, ids=ids_to_delete)
Expand All @@ -659,7 +659,6 @@ def _delete(
collection_uuid=str(collection_id), delete_amount=len(ids_to_delete)
)
)
return ids_to_delete

@trace_method("SegmentAPI._count", OpenTelemetryGranularity.OPERATION)
@retry( # type: ignore[misc]
Expand Down
2 changes: 1 addition & 1 deletion chromadb/db/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ def delete(
collection_uuid: Optional[UUID] = None,
ids: Optional[IDs] = None,
where_document: WhereDocument = {},
) -> List[str]:
) -> None:
pass

@abstractmethod
Expand Down
19 changes: 7 additions & 12 deletions chromadb/server/fastapi/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
Callable,
cast,
Dict,
List,
Sequence,
Optional,
Tuple,
Expand All @@ -21,7 +20,6 @@
from fastapi.middleware.cors import CORSMiddleware
from fastapi.routing import APIRoute
from fastapi import HTTPException, status
from uuid import UUID

from chromadb.api.configuration import CollectionConfigurationInternal
from pydantic import BaseModel
Expand Down Expand Up @@ -918,8 +916,8 @@ def process_get(request: Request, raw_body: bytes) -> GetResult:
@trace_method("FastAPI.delete", OpenTelemetryGranularity.OPERATION)
async def delete(
self, collection_id: str, request: Request, body: DeleteEmbedding = Body(...)
) -> List[UUID]:
def process_delete(request: Request, raw_body: bytes) -> List[str]:
) -> None:
def process_delete(request: Request, raw_body: bytes) -> None:
delete = validate_model(DeleteEmbedding, orjson.loads(raw_body))
self.auth_and_get_tenant_and_database_for_request(
request.headers,
Expand All @@ -935,14 +933,11 @@ def process_delete(request: Request, raw_body: bytes) -> List[str]:
where_document=delete.where_document,
)

return cast(
List[UUID],
await to_thread.run_sync(
process_delete,
request,
await request.body(),
limiter=self._capacity_limiter,
),
await to_thread.run_sync(
process_delete,
request,
await request.body(),
limiter=self._capacity_limiter,
)

@trace_method("FastAPI.count", OpenTelemetryGranularity.OPERATION)
Expand Down
6 changes: 6 additions & 0 deletions chromadb/test/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,12 @@ def test_delete(client):
with pytest.raises(Exception):
collection.delete()

def test_delete_returns_none(client):
client.reset()
collection = client.create_collection("testspace")
collection.add(**batch_records)
assert collection.count() == 2
assert collection.delete(ids=batch_records["ids"]) is None

def test_delete_with_index(client):
client.reset()
Expand Down
12 changes: 7 additions & 5 deletions clients/js/src/Collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -367,16 +367,18 @@ export class Collection {
* });
* ```
*/
async delete({ ids, where, whereDocument }: DeleteParams = {}): Promise<
string[]
> {
async delete({
ids,
where,
whereDocument,
}: DeleteParams = {}): Promise<void> {
await this.client.init();
let idsArray = undefined;
if (ids !== undefined) idsArray = toArray(ids);
return (await this.client.api.aDelete(
await this.client.api.aDelete(
this.id,
{ ids: idsArray, where: where, where_document: whereDocument },
this.client.api.options,
)) as string[];
);
}
}

0 comments on commit 4ae5543

Please sign in to comment.