API for getting all annotations from a collection #432
Conversation
Also added support to annotations_service for setting annotation collection name, e.g. you can have "ground truth" and "prediction" annotation collections.
lightly_studio/src/lightly_studio/resolvers/annotation_resolver/get_all_by_collection_name.py
Show resolved
Hide resolved
|
/review |
Compared to read_annotations(), read_annotations_by_collection_name() does not allow filtering by annotation collection ID, as that conflicts with filtering by collection name.
| description="Types of annotation to filter (e.g., 'object_detection')", | ||
| ) | ||
| collection_ids: list[UUID] | None = Field(default=None, description="List of collection UUIDs") | ||
| collection_names: list[str] | None = Field(default=None, description="List of collection names") |
There was a problem hiding this comment.
I wonder whether we should pair annotation collection name with parent collection ID.
I assume that we can have multiple parent collections whose annotations are grouped into "predictions" and "ground truth", so the name won't be unique.
There was a problem hiding this comment.
We should. Do you want to do this in this PR or a follow-up?
There was a problem hiding this comment.
We should. Do you want to do this in this PR or a follow-up?
I think it makes sense to do this in a followup. We need to drop the uniqueness requirement on annotation collection name first, before it makes sense to include the parent collection ID. Currently the annotation collection name has to be unique, so it's sufficient to identify the collection.
| ) | ||
|
|
||
| @model_validator(mode="after") | ||
| def check_collection_identifiers(self) -> AnnotationsFilter: # noqa: N804 |
There was a problem hiding this comment.
I'll add a test for this, but in the next PR as this one is already too big IMO.
There was a problem hiding this comment.
Optional: I would not mind if we leave out the check. All conditions in the filter are combined with and, it is a valid behaviour to return an empty set.
| description="Types of annotation to filter (e.g., 'object_detection')", | ||
| ) | ||
| collection_ids: list[UUID] | None = Field(default=None, description="List of collection UUIDs") | ||
| collection_names: list[str] | None = Field(default=None, description="List of collection names") |
There was a problem hiding this comment.
We should. Do you want to do this in this PR or a follow-up?
| ) | ||
|
|
||
| @model_validator(mode="after") | ||
| def check_collection_identifiers(self) -> AnnotationsFilter: # noqa: N804 |
There was a problem hiding this comment.
Optional: I would not mind if we leave out the check. All conditions in the filter are combined with and, it is a valid behaviour to return an empty set.
| tag_ids: Annotated[list[UUID] | None, Query()] = None, | ||
| ) -> GetAllAnnotationsResult: | ||
| """Retrieve a list of annotations from the database.""" | ||
| return annotation_resolver.get_all_by_collection_name( |
There was a problem hiding this comment.
I suggest to remove get_all_by_collection_name resolver function and just use get_all with properly set filter. WDYT?
There was a problem hiding this comment.
I've done that locally, we can rely on AnnotationsFilter instead.
|
|
||
|
|
||
| @annotations_router.get( | ||
| "/annotations/collection-name/{annotation_collection_name}", |
There was a problem hiding this comment.
I am not sure if this endpoint fits well with the other endpoints here. Let's discuss in person.
|
|
||
|
|
||
| @annotations_router.get( | ||
| "/annotations/collection-name/{annotation_collection_name}", |
There was a problem hiding this comment.
Do we need a new endpoint? We could use read_annotations and add the annotation_collection_name as an optional parameter, and then as Michal suggested pass it in the filter
There was a problem hiding this comment.
Let's jump on a call.
We have two disjoint use cases:
- querying annotation collection by ID
- querying annotation collection by name and parent collection ID
In Rust I could consume an enum (sum type) as an argument, something like:
enum Query {
ById(UUID),
ByName(String, UUID)
}To emulate this in a HTTP API, it's best IMO to have two separate endpoints, but I'm open to suggestions.
There was a problem hiding this comment.
The alternative would be to remove the collection ID from the arguments and pass the filter directly instead. IMO that would be the cleanest, however it would require changes to the current endpoint calls.
There was a problem hiding this comment.
In addition, there is the parallel "with payload" endpoint, broadening the "impact radius" of changes.
|
Closing after offline discussion. |
What has changed and why?
We want to expose creating and reading annotations collections.
Also added support to
annotations_service forsetting annotation collection name, e.g. you can have "ground truth" and "prediction" annotation collections.How has it been tested?
Added tests.
Did you update CHANGELOG.md?