Fix: Add transformers for videos and bounding boxes - #720
Open
ad-claw000 wants to merge 117 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds transformer support for non-image ingestion artifacts (videos and shape annotations) so common/static properties and derived metadata can be applied consistently during transformer-based ingestion pipelines (Fixes #335).
Changes:
- Added
VideoPropertiestransformer to compute basic blob-derived video metadata and ensure_Video.adb_data_sourceis indexed. - Added
BoundingBoxPropertiestransformer to attach annotation-related properties toAddBoundingBoxandAddPolygoncommands. - Extended
Transformer/CommonPropertiesto detect and apply properties toAddVideo,AddBoundingBox, andAddPolygon, and fixedImagePropertiesproperty initialization/blob mapping.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| aperturedb/transformers/video_properties.py | New transformer computing and attaching video metadata properties. |
| aperturedb/transformers/bounding_box_properties.py | New transformer attaching annotation properties to bounding boxes and polygons. |
| aperturedb/transformers/transformer.py | Tracks indices for AddVideo, AddBoundingBox, and AddPolygon commands. |
| aperturedb/transformers/image_properties.py | Fixes property dict initialization and corrects blob indexing logic. |
| aperturedb/transformers/common_properties.py | Applies common/static properties to images, videos, bboxes, and polygons. |
| aperturedb/transformers/init.py | Introduces package exports for transformers (but currently has critical import issues). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
aperturedb/transformers/bounding_box_properties.py:35
- This transformer iterates over
_add_bounding_box_index/_add_polygon_index, which are computed from the first transaction only. If later transactions contain a different number/order ofAddBoundingBox/AddPolygoncommands (common for per-image variable annotation counts), some annotations won’t getannotation_source/annotation_modeapplied, and transactions with fewer annotations can raise IndexError/KeyError (then skip the rest of the updates). Prefer iterating over the current transaction’s command list (x[0]) and applying properties to every command whose key isAddBoundingBoxorAddPolygon.
for ic in getattr(self, "_add_bounding_box_index", []):
src_properties = x[0][ic]["AddBoundingBox"].setdefault(
"properties", {})
if self.annotation_source:
src_properties["annotation_source"] = self.annotation_source
if self.annotation_mode:
src_properties["annotation_mode"] = self.annotation_mode
for ic in getattr(self, "_add_polygon_index", []):
src_properties = x[0][ic]["AddPolygon"].setdefault(
"properties", {})
if self.annotation_source:
src_properties["annotation_source"] = self.annotation_source
if self.annotation_mode:
src_properties["annotation_mode"] = self.annotation_mode
…tion Removes cached add_bounding_box_index and add_polygon_index from the first transaction in Transformer, as these counts frequently vary per-item (e.g. COCO bounding boxes). CommonProperties and BoundingBoxProperties now iterate over the current transactions commands directly, avoiding IndexError when subsequent items have fewer annotations, and ensuring proper property application when they have more. Added a test case to explicitly check behavior with variable annotation counts.
- Remove unused pytest import in test_Transformers.py - Remove caching of AddImage and AddVideo indices in Transformer.__init__ to handle variable per-item counts - Update CommonProperties and ImageProperties to scan for AddVideo dynamically instead of relying on cached indices - Remove unused _blob_index_map from ImageProperties and VideoProperties - Add unit test coverage for VideoProperties to ensure dynamic properties apply correctly
…ally in remaining embeddings
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
aperturedb/transformers/clip_pytorch_embeddings.py:36
- The descriptorset is only created if the first transaction contains an
AddImage. Ifself.data[0]has no images (but later items do), this transformer will still appendAddDescriptorcommands ingetitem, but may never create the descriptor set in the DB, causing ingestion failures. Consider scanning the first few items for anAddImageblob (or lazily initializing on firstAddImageingetitem).
# Let's sample some data to figure out the descriptorset we need.
sample_blob = None
for i, c in enumerate(self.data[0][0]):
if list(c.keys())[0] == "AddImage":
blob_idx = self._blob_index.index(i)
sample_blob = self.data[0][1][blob_idx]
break
if sample_blob is not None:
sample = generate_embedding(sample_blob)
utils = self.get_utils()
utils.add_descriptorset(
self.search_set_name, dim=len(sample) // 4, metric=["CS"])
Contributor
Author
|
Addressed the review comments, including factoring common properties into a helper, updating docstrings, iterating over |
…d improperly skipping tests
Addresses review feedback from copilot-pull-request-reviewer[bot]
Contributor
Author
|
Partially addressed feedback from @Copilot:
Updated in commit 2a6f1c4 |
Contributor
Author
|
Note: CI is failing with |
Addresses review feedback from reviewers
Addresses top-level PR review comment by adding thorough assertions for exception handling during descriptorset initialization and embedding generation in both clip and facenet transformers.
Contributor
Author
|
Addressed @luisremis's feedback in commit cbea972 by adding tests for backend condition exceptions and embedding generation failures during initialization. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (4)
aperturedb/transformers/clip_pytorch_embeddings.py:46
- On missing blobs, the code increments blob_index and continues. If the transaction is malformed but still contains later blobs, this can cause subsequent blobs to be associated with the wrong AddImage commands, producing incorrect embeddings/descriptors. Prefer stopping further blob-based processing for this transaction once the blob list is exhausted.
except IndexError:
logger.warning(
f"Missing blob for AddImage at index {blob_index}")
blob_index += 1
continue
aperturedb/transformers/facenet_pytorch_embeddings.py:56
- On missing blobs, the code increments blob_index and continues. If later blob-bearing commands exist, this risks misaligning blobs and generating embeddings/descriptors for the wrong images. Prefer stopping further blob-based processing for this transaction once the blob list is exhausted.
except IndexError:
logger.warning(
f"Missing blob for AddImage at index {blob_index}")
blob_index += 1
continue
test/run_test_container.sh:77
- Same issue as in teardown(): chmod is run without sudo, so it can silently fail on root-owned directories, leaving permissions unchanged. Using $(get_sudo) for chmod makes the pre-run permission normalization effective when sudo is available.
$(get_sudo) chown -R $(id -u):$(id -g) "$(pwd)/aperturedb/logs" "$(pwd)/aperturedb/db_"* "$(pwd)/aperturedb/certificate_"* 2>/dev/null || true
chmod -R u+rwX,go-rwx "$(pwd)/aperturedb/logs" "$(pwd)/aperturedb/db_"* "$(pwd)/aperturedb/certificate_"* 2>/dev/null || true
$(get_sudo) chown -R $(id -u):$(id -g) "$(pwd)"/*_ca 2>/dev/null || true
chmod -R u+rwX,go-rwx "$(pwd)"/*_ca 2>/dev/null || true
docker/dependencies/Dockerfile:60
- This Dockerfile change removes
RUN jupyter lab buildin addition to dropping the redundant numpy install. Since the PR description only mentions numpy, please either document why the build step is no longer needed (preferred) or restore it if the image still relies on a build step for extensions/assets.
RUN --mount=type=cache,target=/root/.cache/pip \
PIP_TRUSTED_HOST=${PIP_TRUSTED_HOST} PIP_INDEX_URL=${PIP_INDEX_URL} \
pip install jupyterlab jupyterlab-dash dash-cytoscape plotly jupyter-dash
RUN jupyter labextension disable "@jupyterlab/apputils-extension:announcements"
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docker/dependencies/Dockerfile:60
- The PR description mentions removing redundant
numpyfrom this Dockerfile, but this hunk also removes theRUN jupyter lab buildstep. If the build removal is intentional (e.g., to speed up builds or because JupyterLab no longer needs it), it should be called out in the PR description; otherwise consider restoring it to avoid changing the resulting JupyterLab image behavior.
RUN --mount=type=cache,target=/root/.cache/pip \
PIP_TRUSTED_HOST=${PIP_TRUSTED_HOST} PIP_INDEX_URL=${PIP_INDEX_URL} \
pip install jupyterlab jupyterlab-dash dash-cytoscape plotly jupyter-dash
RUN jupyter labextension disable "@jupyterlab/apputils-extension:announcements"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
VideoPropertiesandBoundingBoxPropertiestransformers to handleAddVideo,AddBoundingBox, andAddPolygoncommands.Updates
CommonPropertiesto process these new annotation types.Additional Changes Included
To fully support the transformer features and ensure tests run successfully in CI, this PR also includes:
.github/workflows/pr.yamlto address permission issues and ensure proper teardown between workflow jobs.CLIPPyTorchEmbeddingsandFacenetPyTorchEmbeddingsto properly handle variable annotation counts, missing blobs, and backend initialization failures.numpyinstallation indocker/dependencies/Dockerfileto fix version conflict issues during image builds.Verification
Manually verified that the property setting works properly for bounding boxes and polygons in the transformer workflow, and that all new edge-case tests pass consistently.
Fixes #335