Skip to content
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

LabelBOT #1012

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from
Draft

LabelBOT #1012

wants to merge 22 commits into from

Conversation

gkourie
Copy link
Contributor

@gkourie gkourie commented Dec 12, 2024

TODO

Backend

  • Adjust request format for storing image annotations: The store method in ImageAnnotationController requires a label_id in the request, but when LabelBOT is active, a request with a feature vector should be accepted without label_id.
  • Attach additional labels to the response: Since LabelBOT may suggest multiple labels, these labels should be included in the response appropriately.
  • Check if HNSW index exists before running ANN if not, switch to KNN.
  • Temporarily drop the HNSW index and rollback the transaction: When switching to exact KNN, the HNSW index must be temporarily dropped, and the transaction should be rolled back.

Frontend

  • If Label Tree doesn't contain any labels, then UI should show an appropriate message (LabelBOT can't be activated).

@gkourie gkourie linked an issue Dec 12, 2024 that may be closed by this pull request
@gkourie gkourie requested a review from mzur December 12, 2024 14:59
Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't implement all the logic as helper methods. Since the logic is used only in the single controller (for now), implement it as (protected) controller methods there.

Also please us camelCase for variable names.

Finally, please fix the failing checks (except the test-current check which fails because biigle/largo is not there). The lint checks can be run locally with composer lint and composer fix.

@gkourie gkourie requested a review from mzur December 19, 2024 16:32
Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor comments left now. We have to make sure that we get the exact queries here. Maybe you can dump the two queries as I explained below and post them here for comparison?

app/Http/Controllers/Api/ImageAnnotationController.php Outdated Show resolved Hide resolved
app/Http/Controllers/Api/ImageAnnotationController.php Outdated Show resolved Hide resolved
app/Http/Controllers/Api/ImageAnnotationController.php Outdated Show resolved Hide resolved
app/Http/Controllers/Api/ImageAnnotationController.php Outdated Show resolved Hide resolved
app/Http/Controllers/Api/ImageAnnotationController.php Outdated Show resolved Hide resolved
app/Http/Controllers/Api/ImageAnnotationController.php Outdated Show resolved Hide resolved
app/Http/Requests/StoreImageAnnotation.php Outdated Show resolved Hide resolved
config/labelbot.php Outdated Show resolved Hide resolved
@gkourie gkourie requested a review from mzur December 20, 2024 16:19
Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine so far (ignoring the missing TODOs). Please go ahead 😉

app/ImageAnnotation.php Outdated Show resolved Hide resolved
@gkourie gkourie requested a review from mzur January 9, 2025 09:42
@gkourie
Copy link
Contributor Author

gkourie commented Jan 9, 2025

@mzur what do you think about the last three points of TODO?

Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query must always be limited to the label trees of the project. Otherwise users might see labels of other users which they are not allowed to see. If no labels exist, the result is empty and that's ok. The UI will show an appropriate message if no label exists (i.e. LabelBOT can't be activated).

If the label tree contains only one label then the existing search will automatically return this label, right? No need to implement a special case for this.

| The B-Tree index is built on the label_tree_id column in the
| image_annotation_label_feature_vectors table.
*/
'B_Tree_ImgAnno_index_name' => 'image_annotation_label_feature_vectors_label_tree_id_index',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The label_tree_id index is always there. It is created in a migration. You can just assume that it is there. Also the index name is fixed (by Laravel conventions). It should not be configurable.

| The HNSW index is built on the vector column in the
| image_annotation_label_feature_vectors table.
*/
'HNSW_ImgAnno_index_name' => 'image_annotation_label_feature_vectors_vector_idx',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HNSW index will also be created in a migration and the name will be fixed. After the update, the migration is run and you can assume that the index exists.

@gkourie
Copy link
Contributor Author

gkourie commented Jan 10, 2025

okay, thanks.

@gkourie gkourie changed the title Labelbot - Backend LabelBOT Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LabelBOT
2 participants