feat: Add CSV ingestion support for Frames

[ad-claw000]

Closes #70

This PR adds a FrameDataCSV class which simply inherits from ImageDataCSV and overrides the command to AddFrame. It also registers IngestType.FRAME in the CLI, closing the loop on Frame OM and ingestion support without duplicating the complex loading/validation logic of Images.

[Copilot]

Pull request overview

This PR extends the Python SDK’s CSV ingestion pipeline to support Frame objects by introducing a FrameDataCSV adapter over the existing image ingestion logic, and wiring the new ingest type into the CLI.

Changes:

• Allow ImageDataCSV subclasses to override the command used in CSVParser-generated queries (AddImage vs AddFrame).
• Add FrameDataCSV for frame ingestion via CSV.
• Register IngestType.FRAME in the adb ingest from-csv CLI mapping.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
aperturedb/ImageDataCSV.py	Makes the ingestion command overrideable (enables reuse for Frames).
aperturedb/FrameDataCSV.py	Adds a new CSV ingester for Frames (needs _Frame index override).
aperturedb/cli/ingest.py	Registers FRAME ingest type in CLI (currently missing BBoxDataCSV import).

Suppressed comments (1)

aperturedb/cli/ingest.py:205

• BBoxDataCSV is referenced in ingest_types but is no longer imported in this function, which will raise NameError when from_csv runs (even for non-bounding-box ingest types, since the dict is constructed unconditionally). Re-add the missing import.

    from aperturedb.ImageDataCSV import ImageDataCSV
    from aperturedb.FrameDataCSV import FrameDataCSV
    from aperturedb.EntityDataCSV import EntityDataCSV
    from aperturedb.BlobDataCSV import BlobDataCSV
    from aperturedb.ConnectionDataCSV import ConnectionDataCSV
    from aperturedb.PolygonDataCSV import PolygonDataCSV
    from aperturedb.VideoDataCSV import VideoDataCSV
    from aperturedb.DescriptorDataCSV import DescriptorDataCSV
    from aperturedb.DescriptorSetDataCSV import DescriptorSetDataCSV

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[luisremis]

add testing to the new FrameDataCSV.
add Frame to the OM, analogous to how Image and Video work.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

aperturedb/cli/ingest.py:199

• from_csv() builds the ingest_types dict using BBoxDataCSV (for IngestType.BOUNDING_BOX) but the import was removed. This will raise a NameError when invoking adb ingest from-csv with --ingest-type BOUNDING_BOX. Re-add the missing import alongside the other CSV parsers.

    from aperturedb.ImageDataCSV import ImageDataCSV
    from aperturedb.FrameDataCSV import FrameDataCSV
    from aperturedb.EntityDataCSV import EntityDataCSV
    from aperturedb.BlobDataCSV import BlobDataCSV

aperturedb/FrameDataCSV.py:14

• New FrameDataCSV behavior (overridden command + _Frame indices) is not covered by tests. There are existing tests exercising other CSV ingesters (e.g., test/test_SPARQL.py uses ImageDataCSV and EntityDataCSV), so adding at least a small unit test for FrameDataCSV would help prevent regressions (e.g., verifying command == "AddFrame" and get_indices() targets _Frame).

class FrameDataCSV(ImageDataCSV):
    def __init__(self, *args, **kwargs):
        self.command = "AddFrame"
        super().__init__(*args, **kwargs)

    def get_indices(self):
        return {
            "entity": {
                "_Frame": self.get_indexed_properties()
            }
        }

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

aperturedb/Images.py:1004

• Frames used to be defined in aperturedb/Images.py; removing it from this module is a breaking change for any code that does from aperturedb.Images import Frames. If the intent is just to move the implementation, consider adding a backwards-compatible shim (e.g., module __getattr__) that resolves Frames lazily from aperturedb.Frames.

        return return_dictionary

test/test_FrameDataCSV.py:6

• pytest and pandas are imported but never used in this test, which adds unnecessary dependencies and can trigger unused-import lint failures.

import pytest
import pandas as pd
import tempfile
import os
from aperturedb.FrameDataCSV import FrameDataCSV

aperturedb/FrameDataCSV.py:8

• FrameDataCSV only needs to override the command string; defining a variadic __init__ drops the base-class signature and makes introspection/type checking harder. With ImageDataCSV now using getattr(self, "command", ...), you can set command as a class attribute and remove the custom initializer.

class FrameDataCSV(ImageDataCSV):
    def __init__(self, *args, **kwargs):
        self.command = "AddFrame"
        super().__init__(*args, **kwargs)

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (3)

aperturedb/Images.py:1004

• Frames used to be defined in this module; removing it breaks existing user code that imports Frames via from aperturedb.Images import Frames. Consider re-exporting the new implementation from this module to preserve backward compatibility while keeping the real class in aperturedb/Frames.py.

        return return_dictionary

aperturedb/FrameDataCSV.py:8

• FrameDataCSV is a new public CSV-ingestion helper but it lacks the class-level docstring that other *DataCSV classes provide (e.g. BBoxDataCSV). Adding a short docstring helps generated docs and keeps the module consistent.

class FrameDataCSV(ImageDataCSV):
    def __init__(self, *args, **kwargs):
        self.command = "AddFrame"
        super().__init__(*args, **kwargs)

test/test_FrameDataCSV.py:5

• pytest and pandas are imported but never used in this test module; removing unused imports keeps the test lightweight and avoids unnecessary dependency coupling.

import pytest
import pandas as pd
import tempfile
import os
from aperturedb.FrameDataCSV import FrameDataCSV

[ad-claw000]

Addressed the latest Copilot reviewer comments in commit cce1bc7 (added backward-compatibility shim for Frames in Images.py, added docstring to FrameDataCSV, and removed unused imports in the test module).

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (3)

test/test_FrameDataCSV.py:7

• This line is long enough that autopep8 will likely reformat it; in this repo CI enforces autopep8 formatting, so it’s better to wrap it now to avoid formatting-only CI failures.

    with tempfile.NamedTemporaryFile(suffix=".csv", mode="w", delete=False) as f:

aperturedb/Frames.py:21

• This super().__init__ call exceeds typical autopep8 line wrapping; wrapping it avoids formatting-only CI failures and keeps it consistent with the rest of the codebase’s style.

    def __init__(self, client, batch_size=100, response=None, **kwargs):
        super().__init__(client, batch_size=batch_size, response=response, **kwargs)

aperturedb/DataModels.py:76

• Changing FrameDataModel from IdentityDataModel to BlobDataModel makes url a required field (via BlobDataModel). If any callers construct FrameDataModel without a URL today, this is a breaking API change. If this is intended, it should be called out in the PR description/changelog; if not intended, consider keeping IdentityDataModel or making url optional on FrameDataModel.

class FrameDataModel(BlobDataModel):
    """Frame data model for ApertureDB.
    """
    type = ObjectType.FRAME

[ad-claw000]

Addressed the latest Copilot reviewer comments in commit 3a92a2f:

• Images.py: Added explicit __all__ and __dir__ to preserve backward compatibility for import * users.
• test_DataModels.py: Removed unused ValidationError and pytest imports.
• test_Entities.py: Removed unused pytest import.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

aperturedb/Images.py:1019

• Introducing __all__ is a behavioral/API change: from aperturedb.Images import * will now only export the listed symbols, whereas previously it would export all non-underscore names. If users rely on star-imports for other public helpers/classes in this module, this becomes a breaking change. Consider either removing __all__ (if not required), or ensuring it includes the full intended public surface of aperturedb.Images (not just a subset + Frames).

__all__ = ["np_arr_img_to_bytes", "image_to_bytes",
           "rotate", "resolve", "Images", "Frames"]

aperturedb/Images.py:1023

• __dir__() can return duplicate entries once Frames is cached into globals() by __getattr__ (since it will be present in both globals().keys() and the appended [\"Frames\"]). Using a set/union before sorting avoids duplicates and keeps dir() output stable.

def __dir__():
    return sorted(list(globals().keys()) + ["Frames"])

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (3)

aperturedb/Images.py:1019

• Introducing __all__ in a previously-exporting module can be a breaking change for consumers using from aperturedb.Images import *, because it will now export only these names (instead of all public symbols). If this module previously relied on the default star-import behavior, consider either removing __all__ or expanding it to include the full intended public surface (including any names that were previously available via star import).

__all__ = ["np_arr_img_to_bytes", "image_to_bytes",
           "rotate", "resolve", "Images", "Frames"]

test/test_Frames.py:14

• The variable name frame is misleading here because it holds a Frames wrapper instance (collection/wrapper), not a single frame. Renaming to frames (or similar) would make the test intent clearer.

    client = MockClient()
    frame = Frames(client)
    assert frame.client == client
    assert frame.db_object == ObjectType.FRAME

test/test_cli_ingest.py:9

• This test verifies that FrameDataCSV is instantiated, but it doesn’t assert that the ingestion pipeline proceeds to call _process_data. Adding an assertion like mock_process_data.assert_called_once() (and optionally checking key arguments such as sample_count, batchsize, and num_workers) would better catch regressions where control flow stops before processing.

    with patch("aperturedb.cli.ingest._process_data") as mock_process_data:
        mock_process_data.return_value = None

[ad-claw000]

Addressed the final Copilot reviewer comments in commit a576464:

• Images.py: Expanded __all__ to explicitly include the module's full public surface (rather than just a subset) to ensure from aperturedb.Images import * remains backward-compatible.
• test_Frames.py: Renamed the misleading frame variable to frames to clarify it represents a collection/wrapper.
• test_cli_ingest.py: Added an assertion (mock_process_data.assert_called_once()) to verify the ingestion pipeline actually proceeds to process data.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (4)

aperturedb/Images.py:1025

• Defining __all__ changes the public surface of aperturedb.Images (notably for from aperturedb.Images import *) and now requires this list to be complete and correct long-term. This is particularly risky in a file intended to preserve backward-compatibility: any missing name becomes a breaking change, and exporting many internal/third-party symbols (e.g., cv2, np) broadens the public API unintentionally. Consider removing __all__ entirely (if not required for the shim), or restricting it to a minimal curated set of truly supported exports (including Frames via __getattr__).

__all__ = [
    "Any", "BytesIO", "Constraints", "DataFrame", "Dict", "Entities",
    "Frames", "HTML", "Image", "Images", "Iterable", "List",
    "ObjectType", "QueryBuilder", "TYPE_CHECKING", "Tuple", "Union",
    "Utils", "annotations", "base64", "class_entity", "cv2", "display",
    "execute_query", "image_to_bytes", "logger", "logging", "math",
    "np", "np_arr_img_to_bytes", "plt", "resolve", "rotate", "widgets"
]

aperturedb/Images.py:1029

• __dir__() may return duplicate entries once Frames has been resolved and cached into globals() by __getattr__. Consider de-duplicating (e.g., via a set) before sorting to keep dir(aperturedb.Images) stable and clean.

def __dir__():
    return sorted(list(globals().keys()) + ["Frames"])

test/test_Frames.py:11

• This MockClient provides no attributes/methods, so the test may become brittle if Images.__init__ (or other Frames/Images logic executed during initialization) starts using client properties (a common pattern). Using a MagicMock() (optionally with a spec for the expected client interface) generally makes the test more robust while still keeping it lightweight.

class MockClient:
    def __init__(self):
        pass

test/test_Frames.py:11

• This MockClient provides no attributes/methods, so the test may become brittle if Images.__init__ (or other Frames/Images logic executed during initialization) starts using client properties (a common pattern). Using a MagicMock() (optionally with a spec for the expected client interface) generally makes the test more robust while still keeping it lightweight.

    client = MockClient()
    frames = Frames(client)

[ad-claw000]

Addressed the remaining changes requested by @luisremis (adding Frame fully to the Object Model logic, analogous to Image and Video for dataset loaders, Query builder, and transformers). See commit 06291df.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

aperturedb/Images.py:1029

• __dir__ can return duplicate entries once Frames has been accessed (because globals() will then already contain Frames). Consider returning a de-duplicated collection (e.g., using a set) before sorting to avoid duplicates and keep introspection stable.

def __dir__():
    return sorted(list(globals().keys()) + ["Frames"])

aperturedb/transformers/transformer.py:71

• The blob-bearing command list is duplicated across multiple modules (transformers, datasets, CommonLibrary, etc.), and this PR extends that duplication by adding AddFrame/FindFrame in many places. To reduce drift risk, consider introducing a shared constant (e.g., BLOB_ADD_COMMANDS / BLOB_FIND_COMMANDS) in a common module and referencing it everywhere (optional but will make future additions safer).

            if command in ["AddImage", "AddDescriptor", "AddVideo", "AddBlob", "AddFrame"]:
                self._blob_index.append(i)

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Suppressed comments (3)

aperturedb/Images.py:1025

• Adding an explicit __all__ is a public API change: it alters from aperturedb.Images import * behavior and can also break star imports at runtime if any listed names are not always defined (or are conditionally imported). If you don't intend to change star-import exports, consider removing __all__; otherwise ensure every name listed is guaranteed to exist in all supported environments, and avoid including non-API/internal names like TYPE_CHECKING.

__all__ = [
    "Any", "BytesIO", "Constraints", "DataFrame", "Dict", "Entities",
    "Frames", "HTML", "Image", "Images", "Iterable", "List",
    "ObjectType", "QueryBuilder", "TYPE_CHECKING", "Tuple", "Union",
    "Utils", "annotations", "base64", "class_entity", "cv2", "display",
    "execute_query", "image_to_bytes", "logger", "logging", "math",
    "np", "np_arr_img_to_bytes", "plt", "resolve", "rotate", "widgets"
]

aperturedb/transformers/transformer.py:71

• The set of blob-bearing commands is duplicated across several transformer modules and was updated in multiple places to add AddFrame. To reduce the risk of future drift (e.g., forgetting to add a new command in one file), consider defining a shared constant (e.g., BLOB_COMMANDS = {...}) in a common module and reusing it everywhere these checks occur.

            if command in ["AddImage", "AddDescriptor", "AddVideo", "AddBlob", "AddFrame"]:
                self._blob_index.append(i)

aperturedb/TensorFlowDataset.py:182

• Support for FindFrame is newly introduced here (including image-style decoding). There are new tests added for ingestion and models, but no tests shown validating dataset behavior for frame queries (e.g., allowed_find_commands acceptance and decoding path). Adding a unit test that exercises FindFrame end-to-end (or at least the decoding branch selection) would help prevent regressions.

            if self.command_name in ("FindImage", "FindFrame"):
                nparr = np.frombuffer(blob, dtype=np.uint8)
                blob = cv2.imdecode(nparr, cv2.IMREAD_COLOR)

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Suppressed comments (2)

aperturedb/Images.py:1025

• Defining __all__ here is potentially a breaking API change: it changes what from aperturedb.Images import * exports, and it can also raise at import-time if any listed name is not actually present in module globals. Unless __all__ is required, consider removing it; otherwise, ensure it is complete and guarantees every name in the list is defined at runtime (and ideally restrict it to the intended public API surface rather than a large set of transitive imports).

__all__ = [
    "Any", "BytesIO", "Constraints", "DataFrame", "Dict", "Entities",
    "Frames", "HTML", "Image", "Images", "Iterable", "List",
    "ObjectType", "QueryBuilder", "TYPE_CHECKING", "Tuple", "Union",
    "Utils", "base64", "class_entity", "cv2", "display",
    "execute_query", "image_to_bytes", "logger", "logging", "math",
    "np", "np_arr_img_to_bytes", "plt", "resolve", "rotate", "widgets"
]

aperturedb/transformers/transformer.py:72

• The set of blob-bearing add commands is duplicated across multiple transformer modules (and now needs repeated updates like adding AddFrame). To prevent future drift, consider centralizing these command names (e.g., a shared constant in a common transformer/util module) and reusing it everywhere these checks occur.

            command = None
            if isinstance(c, dict) and len(c) > 0:
                command = next(iter(c.keys()))
            if command in ["AddImage", "AddDescriptor", "AddVideo", "AddBlob", "AddFrame"]:
                self._blob_index.append(i)
                bc += 1

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Suppressed comments (2)

aperturedb/Images.py:1025

• Defining a large __all__ list here is an API surface change: it restricts what from aperturedb.Images import * exports and will raise at import-time if any listed symbol is not actually present in the module globals. Since this PR’s goal is backward compatibility for Frames, consider either (a) removing __all__ entirely, or (b) limiting __all__ to a small, curated set of stable public exports (and avoid exporting typing helpers like TYPE_CHECKING).

__all__ = [
    "Any", "BytesIO", "Constraints", "DataFrame", "Dict", "Entities",
    "Frames", "HTML", "Image", "Images", "Iterable", "List",
    "ObjectType", "QueryBuilder", "TYPE_CHECKING", "Tuple", "Union",
    "Utils", "base64", "class_entity", "cv2", "display",
    "execute_query", "image_to_bytes", "logger", "logging", "math",
    "np", "np_arr_img_to_bytes", "plt", "resolve", "rotate", "widgets"
]

aperturedb/transformers/transformer.py:70

• The list of blob-bearing commands (now including AddFrame) is duplicated across several modules (multiple transformers + other utilities). This duplication increases the chance future command additions will be missed in one place. Consider centralizing these command sets into a shared constant (e.g., a single module-level definition) and reusing it everywhere.

            if command in ["AddImage", "AddDescriptor", "AddVideo", "AddBlob", "AddFrame"]:

[Copilot]

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

Suppressed comments (3)

aperturedb/Images.py:1025

• Defining __all__ is a behavioral change for consumers that use from aperturedb.Images import *: Python will now try to import exactly these names, and any missing/renamed symbol will cause the star import to fail. Unless there’s a strong need to strictly curate star-imports, consider removing __all__ entirely, or reduce it to a minimal, guaranteed-stable set (and ensure every listed name is always defined at module import time—aside from Frames, which you already handle via __getattr__).

__all__ = [
    "Any", "BytesIO", "Constraints", "DataFrame", "Dict", "Entities",
    "Frames", "HTML", "Image", "Images", "Iterable", "List",
    "ObjectType", "QueryBuilder", "TYPE_CHECKING", "Tuple", "Union",
    "Utils", "base64", "class_entity", "cv2", "display",
    "execute_query", "image_to_bytes", "logger", "logging", "math",
    "np", "np_arr_img_to_bytes", "plt", "resolve", "rotate", "widgets"
]

aperturedb/Images.py:1029

• __dir__() can return duplicate entries once Frames has been resolved and cached into globals(). Consider de-duplicating (e.g., using a set) so dir(aperturedb.Images) remains stable/clean after the first attribute access.

def __dir__():
    return sorted(list(globals().keys()) + ["Frames"])

aperturedb/transformers/transformer.py:71

• The set of blob-bearing add commands is hardcoded (and duplicated) across multiple transformer modules. Now that AddFrame is added, the risk of future drift increases. Consider centralizing these command-name collections (e.g., a shared constant like BLOB_ADD_COMMANDS) and reusing it across transformer.py, image_properties.py, video_properties.py, and the embedding transformers to ensure consistent behavior.

            if command in ["AddImage", "AddDescriptor", "AddVideo", "AddBlob", "AddFrame"]:
                self._blob_index.append(i)

[ad-claw000]

Addressed the latest suppressed Copilot reviewer comments in commit 8abf9b1:

• Images.py: Removed __all__ entirely and deduplicated __dir__ to safely avoid duplicate entries and strictly match the previous behavior of star imports.
• Constants.py: Centralized BLOB_ADD_COMMANDS to avoid drift across transformer models.
• test_tf_connector.py: Added test_findFrame_mocked to ensure FindFrame is correctly executed with decoding support.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (6)

aperturedb/Constants.py:19

• BLOB_ADD_COMMANDS/BLOB_FIND_COMMANDS are mutable lists; using an immutable type (e.g., tuple/frozenset) avoids accidental runtime mutation and communicates intent. Also, BLOB_FIND_COMMANDS appears incomplete relative to other code paths that treat FindBoundingBox as blob-returning; consider adding it (or clearly documenting why it’s excluded) to prevent future divergence.

BLOB_ADD_COMMANDS = [
    "AddImage",
    "AddDescriptor",
    "AddVideo",
    "AddBlob",
    "AddFrame"
]

BLOB_FIND_COMMANDS = [
    "FindImage",
    "FindDescriptor",
    "FindVideo",
    "FindBlob",
    "FindFrame"
]

aperturedb/TensorFlowDataset.py:183

• The class docstring (in this same file) states that only FindImage blobs are decoded via OpenCV, but this change adds identical decoding for FindFrame. Update the docstring to reflect the new behavior so users don’t misinterpret dataset outputs.

            if self.command_name in ("FindImage", "FindFrame"):
                nparr = np.frombuffer(blob, dtype=np.uint8)
                blob = cv2.imdecode(nparr, cv2.IMREAD_COLOR)
                if blob is None:

aperturedb/CommonLibrary.py:388

• This reintroduces an inline “blob-returning commands” list while other modules are moving to shared constants. To prevent drift (e.g., missing/extra commands across files), consider referencing a single shared constant (and ensure that constant includes all commands used here, such as FindBoundingBox).

                    blob_returning_commands = ["FindImage", "FindBlob", "FindVideo",
                                               "FindDescriptor", "FindBoundingBox", "FindFrame"]
                    if k in blob_returning_commands and "blobs" in req[k] and req[k]["blobs"]:

test/run_test_container.sh:69

• check_containers_networks || true will mask failures from docker ps / docker network ls, which can make CI/environment issues harder to diagnose (and can hide “docker not available” errors). A safer approach is to keep the call visible but make the commands inside check_containers_networks best-effort (or gate them on docker availability), so diagnostics remain informative without silently suppressing unexpected failures.

    $(get_sudo) rm -rf "${SCRIPT_DIR}/aperturedb" || true

test/run_test_container.sh:219

• check_containers_networks || true will mask failures from docker ps / docker network ls, which can make CI/environment issues harder to diagnose (and can hide “docker not available” errors). A safer approach is to keep the call visible but make the commands inside check_containers_networks best-effort (or gate them on docker availability), so diagnostics remain informative without silently suppressing unexpected failures.

check_containers_networks || true

aperturedb/PyTorchDataset.py:116

• FindFrame decoding behavior was added here but the new tests only cover the TensorFlow dataset path. Add a unit test that mirrors the TensorFlow mocked test to assert PyTorch dataset returns decoded tensors/arrays (and correct label dtype) for FindFrame.

        if self.command_name in ("FindImage", "FindFrame"):
            nparr = np.frombuffer(blob, dtype=np.uint8)
            blob = cv2.imdecode(nparr, cv2.IMREAD_COLOR)