From 6b9fb27eff342aeebc58e7db85d59bb53ff5603e Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 24 Jul 2026 19:08:11 -0400 Subject: [PATCH 1/2] Unify scoped metadata attachment storage --- server/dive_server/crud_dataset.py | 323 ++++++++-- server/dive_server/crud_rpc.py | 183 +++++- server/dive_server/event.py | 6 +- server/dive_server/views_dataset.py | 14 +- server/dive_tasks/tasks.py | 16 +- server/dive_tasks/utils.py | 75 ++- server/dive_utils/constants.py | 3 +- server/dive_utils/frame_metadata.py | 39 ++ server/tests/test_event_frame_metadata.py | 62 ++ server/tests/test_frame_metadata_naming.py | 21 + server/tests/test_frame_metadata_sources.py | 629 +++++++++++++++++++ server/tests/test_inject_metadata_file.py | 94 +++ server/tests/test_multicam_export_clone.py | 235 ++++++- server/tests/test_multicam_zip_import.py | 225 ++++++- server/tests/test_pipeline_discovery.py | 28 + server/tests/test_pipeline_metadata_scope.py | 184 ++++++ server/tests/test_process_items.py | 453 +++++++++++++ server/tests/test_update_metadata.py | 11 + server/tests/test_validate_files.py | 76 ++- testutils/framemetadata.spec.json | 21 + testutils/multicam.spec.json | 35 ++ 21 files changed, 2627 insertions(+), 106 deletions(-) create mode 100644 server/dive_utils/frame_metadata.py create mode 100644 server/tests/test_event_frame_metadata.py create mode 100644 server/tests/test_frame_metadata_naming.py create mode 100644 server/tests/test_frame_metadata_sources.py create mode 100644 server/tests/test_inject_metadata_file.py create mode 100644 server/tests/test_pipeline_metadata_scope.py create mode 100644 server/tests/test_process_items.py create mode 100644 testutils/framemetadata.spec.json diff --git a/server/dive_server/crud_dataset.py b/server/dive_server/crud_dataset.py index 6d5957c04..a62f1b66c 100644 --- a/server/dive_server/crud_dataset.py +++ b/server/dive_server/crud_dataset.py @@ -7,9 +7,9 @@ import cherrypy from girder.constants import AccessType from girder.exceptions import RestException +from girder.models.file import File from girder.models.folder import Folder from girder.models.item import Item -from girder.models.file import File from girder.models.token import Token from girder.utility import ziputil from pydantic.main import BaseModel @@ -21,6 +21,7 @@ asbool, calibration_format, constants, + frame_metadata, fromMeta, models, multicam_camera_order, @@ -124,6 +125,8 @@ def _create_multicam_soft_clone( creator=owner, ) cloned_folder['meta'] = copy.deepcopy(source_folder['meta']) + media_source_folder = crud.getCloneRoot(owner, source_folder) + cloned_folder[constants.ForeignMediaIdMarker] = str(media_source_folder['_id']) cloned_folder['meta'][constants.PublishedMarker] = False if constants.ConfidenceFiltersMarker not in cloned_folder['meta']: cloned_folder['meta'][constants.ConfidenceFiltersMarker] = {'default': 0.1} @@ -249,6 +252,29 @@ def _multicam_camera_order(multi_cam: dict) -> List[str]: return multicam_camera_order(multi_cam) +def _iter_multicam_camera_folders( + multi_cam: dict, + user: types.GirderUserModel, +) -> Iterable[Tuple[str, types.GirderModel]]: + """Yield (camera_name, camera_folder) in display order, loading each child folder. + + Every camera entry has a folderId (crud.verify_dataset rejects a multicam dataset + without one). Raises a 404 when a camera's folder cannot be loaded, e.g. a clone whose + source was deleted. + """ + cameras_meta = multi_cam.get('cameras') or {} + for camera_name in _multicam_camera_order(multi_cam): + child = Folder().load( + cameras_meta[camera_name]['folderId'], level=AccessType.READ, user=user + ) + if child is None: + raise RestException( + f'Camera folder for "{camera_name}" was not found', + code=404, + ) + yield camera_name, child + + def get_multi_cam_media( dsFolder: types.GirderModel, user: types.GirderUserModel ) -> models.MultiCamMedia: @@ -383,6 +409,178 @@ def get_media( ) +def _clone_root( + folder: types.GirderModel, + user: types.GirderUserModel, +) -> types.GirderModel: + """The folder's media source root, or the folder itself when it is not a clone. + + crud.getCloneRoot verifies the dataset before walking, and attachment resolution runs + mid-import (process_items) on folders that do not yet carry fps. A folder with no + foreign-media marker is its own root, so skipping the walk skips that verification + without changing the answer. + """ + if not folder.get(constants.ForeignMediaIdMarker): + return folder + return crud.getCloneRoot(user, folder) + + +def _metadata_attachment( + folder: types.GirderModel, + user: types.GirderUserModel, +) -> Optional[Dict[str, str]]: + """Resolve one folder's explicit metadata attachment without parsing its contents. + + The item id comes from folder metadata and can name any item in the instance, so + membership in this scope is the guard: the item must live in ``folder`` or its clone + root. Resolution asserts no ACL, because it cannot: a clone root is force-loaded + (``crud.getCloneRoot``) and may be unreadable to ``user``, exactly as ``crud.valid_images`` + already lists that root's media. Access is enforced where the bytes are served -- the + Girder item download route. + """ + item_id = folder.get('meta', {}).get(constants.MetadataFileItemIdMarker) + if not item_id: + return None + original_name = fromMeta( + folder, + constants.MetadataFileOriginalNameMarker, + '', + ) + item = Item().load(str(item_id), force=True) + allowed_folder_ids = { + str(folder['_id']), + str(_clone_root(folder, user)['_id']), + } + if ( + item is None + or str(item.get('folderId')) not in allowed_folder_ids + or not constants.metadataFileRegex.search(item['name']) + ): + return { + 'name': original_name or 'Metadata File', + 'error': 'Metadata attachment is unavailable.', + } + return {'itemId': str(item['_id']), 'name': original_name or item['name']} + + +def _reserved_metadata_attachment(folder: types.GirderModel) -> Optional[Dict[str, str]]: + """Resolve the singular reserved-name fallback in one folder. + + The query is the reserved-basename predicate, so every returned item is an attachment. + Like the explicit path this establishes identity only, and for the same reason: ``folder`` + may be a force-loaded clone root, so there is no ACL for the resolver to assert. + """ + items = list( + Folder().childItems( + folder, + filters=frame_metadata.frame_metadata_source_name_query(), + ) + ) + if len(items) > 1: + return { + 'name': 'Metadata File', + 'error': 'More than one reserved-name metadata attachment is available.', + } + if not items: + return None + item = items[0] + return {'itemId': str(item['_id']), 'name': item['name']} + + +def _first_metadata_attachment( + folders: Iterable[Optional[types.GirderModel]], + user: types.GirderUserModel, + *, + reserved: bool, +) -> Optional[Dict[str, str]]: + """Resolve the first attachment across placement-precedence folders.""" + seen_folder_ids: set[str] = set() + for folder in folders: + if folder is None: + continue + folder_id = str(folder['_id']) + if folder_id in seen_folder_ids: + continue + seen_folder_ids.add(folder_id) + attachment = ( + _reserved_metadata_attachment(folder) + if reserved + else _metadata_attachment(folder, user) + ) + if attachment is not None: + return attachment + return None + + +def resolve_metadata_attachment( + folder: types.GirderModel, + user: types.GirderUserModel, +) -> Optional[Dict[str, str]]: + """Resolve one scope's metadata attachment identity. + + The single owner of "what is this scope's attachment": an explicit marker on the folder + or its clone root first, then the reserved-name file in the same folders. Returns + ``{itemId, name}`` when resolvable, ``{name, error}`` when declared but unavailable, and + None when the scope has no attachment. + """ + folders = [folder, _clone_root(folder, user)] + explicit = _first_metadata_attachment(folders, user, reserved=False) + if explicit is not None: + return explicit + return _first_metadata_attachment(folders, user, reserved=True) + + +def resolve_metadata_attachment_item_id( + folder: types.GirderModel, + user: types.GirderUserModel, +) -> Optional[str]: + """Return the resolved attachment's item id, or None when absent or unavailable.""" + attachment = resolve_metadata_attachment(folder, user) + if attachment is None: + return None + return attachment.get('itemId') + + +def load_frame_metadata_sources( + dsFolder: types.GirderModel, + user: types.GirderUserModel, +) -> dict: + """Return one normalized metadata attachment identity per metadata scope.""" + crud.verify_dataset(dsFolder) + source_type = fromMeta(dsFolder, constants.TypeMarker) + if source_type == constants.MultiType: + return _load_multicam_frame_metadata_sources(dsFolder, user) + if source_type not in (constants.ImageSequenceType, constants.VideoType): + return {'cameras': {}} + shared = resolve_metadata_attachment(dsFolder, user) + return {'shared': shared, 'cameras': {}} if shared is not None else {'cameras': {}} + + +def _load_multicam_frame_metadata_sources( + dsFolder: types.GirderModel, + user: types.GirderUserModel, +) -> dict: + multi_cam = fromMeta(dsFolder, constants.MultiCamMarker) or {} + shared = resolve_metadata_attachment(dsFolder, user) + cameras: Dict[str, Dict[str, str]] = {} + for camera_name, child in _iter_multicam_camera_folders(multi_cam, user): + if fromMeta(child, constants.TypeMarker) not in ( + constants.ImageSequenceType, + constants.VideoType, + ): + continue + # A camera-local attachment replaces the shared one for that camera, whether it was + # declared explicitly or by reserved name. + attachment = resolve_metadata_attachment(child, user) + if attachment is not None: + cameras[camera_name] = attachment + + response: Dict[str, Any] = {'cameras': cameras} + if shared is not None: + response['shared'] = shared + return response + + class MetadataMutableUpdateArgs(models.MetadataMutable): """Update schema for mutable metadata fields""" @@ -641,24 +839,26 @@ def _yield_metadata_file( z: ziputil.ZipGenerator, zip_path: str, folder: types.GirderModel, + user: types.GirderUserModel, ) -> Generator[bytes, None, None]: - """Add the optional per-dataset metadata file when one is attached. - - Mirrors calibration inclusion in full exports so pipeline sidecars - (e.g. flight logs) survive dataset zip round-trips. + """Add the folder's attachment at its archive-relative metadata path. + + ``metadata/`` is the archive's only record of the attachment; importers + discover it by directory, so meta.json carries no locator. Resolution already confined + the item to this dataset's scope, and the export walks a clone's media source the same + way, so the item is loaded without a second access check -- one that would raise + AccessException past the ``except RestException`` guard in export_datasets_zipstream and + abort the whole stream instead of naming the dataset in failed_datasets.txt. """ - item_id = resolve_metadata_file_item_id(folder) - if not item_id: + attachment = resolve_metadata_attachment(folder, user) + if attachment is None or 'itemId' not in attachment: return - md_item = Item().findOne({'_id': _mongo_id(item_id)}) + md_item = Item().load(attachment['itemId'], force=True) if md_item is None: return - # Prefer the preserved original filename when present so re-import can - # match the name users attached at import time. - original_name = folder.get('meta', {}).get(constants.MetadataFileOriginalNameMarker) - for path, file in Item().fileList(md_item): - out_name = original_name or path - for data in z.addFile(file, Path(f'{zip_path}{out_name}')): + original_name = Path(attachment['name']).name + for _path, file in Item().fileList(md_item): + for data in z.addFile(file, Path(f'{zip_path}metadata/{original_name}')): yield data break @@ -695,13 +895,17 @@ def makeMetajson(): """Include dataset metadata file with full export""" meta = get_dataset(dsFolder, user) media = get_media(dsFolder, user) - yield json.dumps( - { - **meta.dict(exclude_none=True), - **media.dict(exclude_none=True), - }, - indent=2, - ) + output = { + **meta.dict(exclude_none=True), + **media.dict(exclude_none=True), + } + # Attachment locators never travel in an archive: a server-local item id means + # nothing there, and the name is already carried by metadata/, which + # is where both importers look. Mirrors withoutMetadataAttachment in the desktop + # exporter (client/platform/desktop/backend/native/multicamExport.ts). + output.pop(constants.MetadataFileItemIdMarker, None) + output.pop(constants.MetadataFileOriginalNameMarker, None) + yield json.dumps(output, indent=2) def makeDiveJson(): """Include DIVE JSON output annotation file""" @@ -749,7 +953,7 @@ def makeDiveJson(): yield data if includeMedia: - for data in _yield_metadata_file(z, zip_path, dsFolder): + for data in _yield_metadata_file(z, zip_path, dsFolder, user): yield data @@ -772,12 +976,13 @@ def makeMultiCamJson(): for data in z.addFile(makeMultiCamJson, Path(f'{zip_path}multiCam.json')): yield data + # The parent has no media of its own; includeMedia governs only its shared attachment. for data in _yield_single_dataset_export( z, zip_path, dsFolder, user, - False, + includeMedia, includeDetections, excludeBelowThreshold, typeFilter, @@ -787,11 +992,6 @@ def makeMultiCamJson(): if includeMedia: for data in _yield_calibration_files(z, zip_path, str(dsFolder['_id'])): yield data - # Parent folder holds the optional metadata sidecar (not camera children). - # Parent _yield_single_dataset_export runs with includeMedia=False, so - # yield it here alongside calibration. - for data in _yield_metadata_file(z, zip_path, dsFolder): - yield data for cam_name in _multicam_camera_order(multi_cam): cam_info = multi_cam['cameras'][cam_name] @@ -1114,16 +1314,12 @@ def _mark_calibration_source_and_json_item(cal_item: dict) -> None: ) -def _mark_metadata_file_item(md_item: dict) -> None: - Item().setMetadata(md_item, {constants.MetadataFileMarker: 'true'}) - - def _validate_metadata_file_item( user: types.GirderUserModel, folder: types.GirderModel, item_id: str, ) -> types.GirderModel: - """Validate and mark a Girder item as the dataset's optional metadata file.""" + """Validate a Girder item as the dataset's optional metadata file.""" md_item = Item().load(item_id, level=AccessType.WRITE, user=user) if md_item is None: raise RestException('Metadata file was not found', code=404) @@ -1131,7 +1327,6 @@ def _validate_metadata_file_item( raise RestException('Metadata file must be stored in the dataset folder', code=400) if not constants.metadataFileRegex.search(md_item['name']): raise RestException('Metadata file must be .json, .txt, or .csv', code=400) - _mark_metadata_file_item(md_item) return md_item @@ -1463,22 +1658,30 @@ def validate_files(files: List[str]): ``type`` is present only when ``ok``: a rejected selection has no single media type. """ - videos = [f for f in files if constants.videoRegex.search(f)] - images = [f for f in files if constants.imageRegex.search(f)] - large_images = [f for f in files if constants.largeImageRegEx.search(f)] + # Partition frame-metadata sidecars first so they never trip the + # generic csv/txt classification below. + frame_meta = [f for f in files if frame_metadata.is_frame_metadata_source_name(f)] + frame_meta_set = set(frame_meta) + + videos = [f for f in files if constants.videoRegex.search(f) and f not in frame_meta_set] + images = [f for f in files if constants.imageRegex.search(f) and f not in frame_meta_set] + large_images = [ + f for f in files if constants.largeImageRegEx.search(f) and f not in frame_meta_set + ] media = images + videos + large_images - # Dataset config JSON follows the same meta/config filename contract as the client's - # JsonMetaRegEx; annotation JSON is every other .json. + # Keep dataset config filename detection aligned with the client's JsonMetaRegEx. dataset_config = [ f for f in files if constants.jsonRegex.search(f) and constants.metaRegex.search(f) ] dataset_config_set = set(dataset_config) - annotation_csvs = [f for f in files if constants.csvRegex.search(f)] + annotation_csvs = [f for f in files if constants.csvRegex.search(f) and f not in frame_meta_set] annotation_ymls = [f for f in files if constants.ymlRegex.search(f)] annotation_jsons = [ - f for f in files if constants.jsonRegex.search(f) and f not in dataset_config_set + f + for f in files + if constants.jsonRegex.search(f) and f not in dataset_config_set and f not in frame_meta_set ] annotations = annotation_csvs + annotation_ymls + annotation_jsons @@ -1502,6 +1705,9 @@ def validate_files(files: List[str]): elif len(annotation_csvs) > 1: ok = False message = "Can only upload a single CSV Annotation per import" + elif len(frame_meta) > 1: + ok = False + message = "More than one metadata file was selected. Choose one file and try again." elif len(dataset_config) > 1: ok = False message = "Can only upload a single configuration JSON per import" @@ -1526,7 +1732,9 @@ def validate_files(files: List[str]): ok = False message = "No supported media-type files found" - accepted = set(media) | set(annotations) | set(dataset_config) + # A metadata attachment is stored for every dataset type; read time decides what to do + # with it, so there is no media-type gate here. + accepted = set(media) | set(annotations) | set(dataset_config) | frame_meta_set ignored = [f for f in files if f not in accepted] return { @@ -1538,6 +1746,7 @@ def validate_files(files: List[str]): "media": media, "annotations": annotations, "datasetConfig": dataset_config, + "frameMetadata": frame_meta, "ignored": ignored, }, "reasons": {f: UNSUPPORTED_SIDE_FILE_REASON for f in ignored}, @@ -1692,17 +1901,35 @@ def set_metadata_file( folder root. Handed to opt-in pipelines at run time (see the pipe `# Metadata File:` header). """ + # Supersede cleanup removes only an attachment DIVE itself stored. A reserved-name file + # is the user's own folder content, uploaded with the media and merely discovered by + # resolution -- and process_items records what it discovers in this same marker, so the + # marker alone cannot tell the two apart. The name can: replacing the attachment shadows + # a reserved-name file, and deleting it would destroy a file the user uploaded. + declared_item_id = folder.get('meta', {}).get(constants.MetadataFileItemIdMarker) + previous_item_id = str(declared_item_id) if declared_item_id else None md_item = _validate_metadata_file_item(user, folder, item_id) + previous_item = None + if previous_item_id and previous_item_id != str(md_item['_id']): + candidate = Item().load(previous_item_id, level=AccessType.WRITE, user=user) + if ( + candidate is not None + and str(candidate.get('folderId')) == str(folder['_id']) + and not frame_metadata.is_frame_metadata_source_name(candidate['name']) + ): + previous_item = candidate folder['meta'][constants.MetadataFileItemIdMarker] = str(md_item['_id']) folder['meta'][constants.MetadataFileOriginalNameMarker] = md_item['name'] Folder().save(folder) + if previous_item is not None: + try: + Item().remove(previous_item) + except Exception: + cherrypy.log( + f'Unable to remove superseded metadata attachment {previous_item["_id"]}', + traceback=True, + ) return { 'metadataFileItemId': str(md_item['_id']), 'metadataFileOriginalName': md_item['name'], } - - -def resolve_metadata_file_item_id(folder: types.GirderModel) -> Optional[str]: - """Return the dataset's metadata file item id, if one is attached.""" - item_id = folder.get('meta', {}).get(constants.MetadataFileItemIdMarker) - return str(item_id) if item_id else None diff --git a/server/dive_server/crud_rpc.py b/server/dive_server/crud_rpc.py index 7001cf3c5..cf6eb8ca4 100644 --- a/server/dive_server/crud_rpc.py +++ b/server/dive_server/crud_rpc.py @@ -1,7 +1,7 @@ from datetime import datetime, timedelta import json -from typing import Dict, List, Optional, Tuple, TypedDict from pathlib import Path +from typing import Dict, List, Optional, Tuple, TypedDict from girder.constants import AccessType from girder.exceptions import RestException @@ -20,7 +20,15 @@ from dive_tasks import tasks from dive_tasks.utils import choose_annotation_fps from dive_tasks.multicam_pipeline import is_stereo_or_multicam_pipeline, pipeline_requires_input -from dive_utils import TRUTHY_META_VALUES, asbool, constants, fromMeta, models, types +from dive_utils import ( + TRUTHY_META_VALUES, + asbool, + constants, + frame_metadata, + fromMeta, + models, + types, +) from dive_utils.constants import TrainingModelExtensions from dive_utils.serializers import dive, kpf, kwcoco, viame @@ -299,6 +307,7 @@ def run_pipeline( multicam_cameras: List[types.MulticamCameraJob] = [] multicam_default_display = '' calibration_item_id: Optional[str] = None + default_camera_folder: Optional[types.GirderModel] = None if dataset_type == constants.MultiType: multi_cam = fromMeta(folder, constants.MultiCamMarker, required=True) @@ -311,6 +320,8 @@ def run_pipeline( child = Folder().load(folder_id, level=AccessType.READ, user=user) if child is None: raise RestException(f'Camera folder for "{name}" was not found', code=404) + if name == multicam_default_display: + default_camera_folder = child cam_type = cam_info.get('type') or fromMeta(child, constants.TypeMarker) camera_job: types.MulticamCameraJob = { 'name': name, @@ -340,11 +351,21 @@ def run_pipeline( ) # Resolve the dataset's optional metadata file for pipelines that opt in via a - # `# Metadata File: :` header. Applies to single and multicam. + # `# Metadata File: :` header. Scope precedence mirrors what the metadata + # panel resolves: a single-camera run prefers its own camera's attachment and falls + # back to the multicam parent's shared one, a stereo/multicam run prefers the shared + # attachment and falls back to the default display camera's. Every lookup goes through + # the one resolver that owns "what is this scope's attachment", so a run and the panel + # never disagree. metadata_file_key = (pipeline.get("metadata") or {}).get("metadataFileKey") metadata_file_item_id: Optional[str] = None if metadata_file_key: - metadata_file_item_id = crud_dataset.resolve_metadata_file_item_id(folder) + scope_item_ids = ( + crud_dataset.resolve_metadata_attachment_item_id(scope, user) + for scope in (folder, multicam_parent, default_camera_folder) + if scope is not None + ) + metadata_file_item_id = next((item_id for item_id in scope_item_ids if item_id), None) params: types.MulticamPipelineJob = { "pipeline": pipeline, @@ -373,7 +394,6 @@ def run_pipeline( if metadata_file_key and metadata_file_item_id: params['metadata_file_key'] = metadata_file_key params['metadata_file_item_id'] = metadata_file_item_id - job_title = f"Running {pipeline['name']} on {str(folder['name'])}" if multicam_parent is not None and camera_name: job_title = ( @@ -400,6 +420,17 @@ def run_pipeline( constants.JOBCONST_CREATOR: str(user['_id']), }, ) + if metadata_file_key and not metadata_file_item_id: + # Same shape as the missing-calibration notice the task writes: the run continues + # without the `-s :=` setting, so say so instead of dropping it. + Job().updateJob( + job, + log=( + f'Warning: {pipeline["pipe"]} declares metadata file key {metadata_file_key} ' + 'but this dataset has no metadata attachment; ' + 'running without the metadata file setting\n' + ), + ) # Inform Client of new Job added in inactive state Notification( type='job_status', @@ -552,6 +583,11 @@ def run_training( ) +def _frame_metadata_kept_warning(name: str) -> str: + """Notice that a file was kept as a frame-metadata sidecar rather than imported.""" + return f'{name} was stored as frame metadata, not annotations; it stays in the dataset folder.' + + def _get_data_by_type( file: types.GirderModel, image_map: Optional[Dict[str, int]] = None, @@ -669,6 +705,19 @@ def resolve_imported_dataset_info(existing: types.DatasetInfo, meta: dict, addit return {**meta, 'datasetInfo': resolved} +def _attach_swept_sidecar(folder: types.GirderModel, item: types.GirderModel): + """Record a swept sidecar as the folder's explicit metadata attachment. + + Girder's save is a full-document replace and async jobs (convert_video) write folder + meta while this sweep runs, so refresh first or their keys (annotate, originalFps, + ffprobe_info) are replaced with this stale in-memory copy. + """ + crud.refresh_folder_document(folder) + folder['meta'][constants.MetadataFileItemIdMarker] = str(item['_id']) + folder['meta'][constants.MetadataFileOriginalNameMarker] = item['name'] + Folder().save(folder) + + def process_items( folder: types.GirderModel, user: types.GirderUserModel, @@ -679,45 +728,129 @@ def process_items( """ Discover unprocessed items in a dataset and process them by type in order of creation """ - unprocessed_items = Folder().childItems( - folder, - filters={ - "$or": [ - {"lowerName": {"$regex": constants.csvRegex}}, - {"lowerName": {"$regex": constants.jsonRegex}}, - {"lowerName": {"$regex": constants.ymlRegex}}, - ] - }, - # Processing order: oldest to newest - sort=[("created", pymongo.ASCENDING)], - ) - auxiliary = crud.get_or_create_auxiliary_folder( - folder, - user, + attachment_item_id = crud_dataset.resolve_metadata_attachment_item_id(folder, user) + + def is_declared_sidecar(item: types.GirderModel) -> bool: + return str(item['_id']) == attachment_item_id or ( + frame_metadata.is_frame_metadata_source_name(item['name']) + ) + + unprocessed_items = list( + Folder().childItems( + folder, + filters={ + "$and": [ + { + "$or": [ + {"lowerName": {"$regex": constants.csvRegex}}, + {"lowerName": {"$regex": constants.jsonRegex}}, + {"lowerName": {"$regex": constants.ymlRegex}}, + # Reserved sidecar names include .txt, which no annotation + # extension covers. Sweeping them keeps all six behaving + # identically instead of leaving .txt undiscovered. + frame_metadata.frame_metadata_source_name_query(), + ] + }, + # Frame-metadata sidecars are marked processed but stay in the dataset + # folder; excluding them here keeps a left-in-place sidecar from being + # re-swept on a later postprocess. + {f'meta.{constants.ProcessedMarker}': {'$ne': True}}, + ] + }, + # Processing order: oldest to newest + sort=[("created", pymongo.ASCENDING)], + ) ) - aggregate_warnings = [] + aggregate_warnings: List[str] = [] + + # This sweep is also the convergence point for headless writers (assetstore/S3 import), + # where nobody picked these files and nothing can be corrected pre-upload. Attachment + # ambiguity degrades to a warning rather than raising: a raise strips the folder's retry + # marker and leaves fps at -1, so the folder would never converge. + reserved_items = [ + item + for item in unprocessed_items + if frame_metadata.is_frame_metadata_source_name(item['name']) + ] + if attachment_item_id is None and len(reserved_items) > 1: + # The extras stay in the folder, so the remedy is to delete all but one and + # re-import; nothing is marked, so there is nothing to undo. + reserved_names = ', '.join(item['name'] for item in reserved_items) + aggregate_warnings.append( + f'More than one metadata file was found in the dataset folder: {reserved_names}. ' + 'None was attached; keep one and re-import.' + ) + + # Import the oldest annotation CSV and name the rest in a warning rather than failing. + annotation_csv_items = [ + item + for item in unprocessed_items + if constants.csvRegex.search(item['name']) and not is_declared_sidecar(item) + ] + skipped_csv_items = annotation_csv_items[1:] + if skipped_csv_items: + skipped_names = ', '.join(item['name'] for item in skipped_csv_items) + aggregate_warnings.append( + f'Imported annotations from {annotation_csv_items[0]["name"]} only; ' + f'skipped {skipped_names}. A dataset imports one annotation CSV at a time.' + ) + skipped_ids = {str(item['_id']) for item in skipped_csv_items} + unprocessed_items = [ + item for item in unprocessed_items if str(item['_id']) not in skipped_ids + ] + + # image_map (extension-stripped media stems) feeds the VIAME parser and only exists for + # image-sequence folders. Skip the media walk entirely when only sidecars are present: + # a declared sidecar is classified by name and never joined here. + image_map = None + if fromMeta(folder, constants.TypeMarker) == constants.ImageSequenceType and any( + not is_declared_sidecar(item) for item in unprocessed_items + ): + image_map = crud.valid_image_names_dict(crud.valid_images(folder, user)) + + auxiliary = None for item in unprocessed_items: file: Optional[types.GirderModel] = next(Item().childFiles(item), None) if file is None: raise RestException('Item had no associated files') + # The single classification point: a declared sidecar is identified by attachment + # identity or reserved name and never reaches the annotation classifier. Keep it in + # the dataset folder for read-time discovery and mark it processed so it is not + # re-swept, but never import it as annotations, move it, or remove it. + if is_declared_sidecar(item): + if str(item['_id']) == attachment_item_id and not fromMeta( + folder, constants.MetadataFileItemIdMarker + ): + _attach_swept_sidecar(folder, item) + item['meta'][constants.ProcessedMarker] = True + Item().save(item) + aggregate_warnings.append(_frame_metadata_kept_warning(file['name'])) + continue + try: - image_map = None - if fromMeta(folder, constants.TypeMarker) == 'image-sequence': - image_map = crud.valid_image_names_dict(crud.valid_images(folder, user)) results, warnings = _get_data_by_type(file, image_map=image_map) if warnings: aggregate_warnings += warnings except Exception as e: Item().remove(item) if isinstance(e, ValueError): - raise RestException(f'Failed to import {file["name"]}: {e}') from e + hint = ( + ' If this file is frame metadata rather than annotations, upload it ' + 'in the "Metadata File (Optional)" field on the upload page, or rename ' + 'it to frame-metadata.csv and re-upload.' + if constants.csvRegex.search(file['name']) + else '' + ) + raise RestException(f'Failed to import {file["name"]}: {e}{hint}') from e raise RestException(f'{file["name"]} was not a supported file type: {e}') from e if results is None: Item().remove(item) raise RestException(f'Unknown file type for {file["name"]}') + if auxiliary is None: + auxiliary = crud.get_or_create_auxiliary_folder(folder, user) item['meta'][constants.ProcessedMarker] = True Item().move(item, auxiliary) if results['annotations']: diff --git a/server/dive_server/event.py b/server/dive_server/event.py index c2b26dd3b..084459727 100644 --- a/server/dive_server/event.py +++ b/server/dive_server/event.py @@ -16,7 +16,7 @@ from girder_plugin_worker.utils import getWorkerApiUrl from dive_tasks.dive_batch_postprocess import DIVEBatchPostprocessTaskParams -from dive_utils import asbool, fromMeta +from dive_utils import asbool, frame_metadata, fromMeta from dive_utils.constants import ( AnnotationFileFutureProcessMarker, AssetstoreSourceMarker, @@ -106,6 +106,10 @@ def process_assetstore_import(event, meta: dict): # Set the dataset to Video Type dataset_type = VideoType elif possibleAnnotationRegex.search(importPath): + if frame_metadata.is_frame_metadata_source_name(item['name']): + # Declared frame metadata sidecars are plain files: leave them in place for + # read-time discovery, no marker and no relocation. + return # Look for parent folder with same name parentFolder = Folder().findOne({"_id": item["folderId"]}) userId = parentFolder['creatorId'] or parentFolder['baseParentId'] diff --git a/server/dive_server/views_dataset.py b/server/dive_server/views_dataset.py index 5190cfd61..bc62513b0 100644 --- a/server/dive_server/views_dataset.py +++ b/server/dive_server/views_dataset.py @@ -41,6 +41,7 @@ def __init__(self, resourceName): self.route("POST", (":id", "calibration"), self.set_dataset_calibration) self.route("POST", (":id", "metadata_file"), self.set_dataset_metadata_file) self.route("GET", (":id", "media"), self.get_media) + self.route("GET", (":id", "frame_metadata_sources"), self.get_frame_metadata_sources) self.route("GET", ("export",), self.export) self.route("GET", (":id", "configuration"), self.get_configuration) self.route("GET", (":id", "media", ":mediaId", "download"), self.download_media) @@ -203,7 +204,7 @@ def list_datasets( ) def get_meta(self, folder): return crud_dataset.get_dataset(folder, self.getCurrentUser()).dict(exclude_none=True) - + @access.user @autoDescribeRoute( Description("Get calibration information of dataset") @@ -276,6 +277,17 @@ def get_configuration(self, folder): def get_media(self, folder): return crud_dataset.get_media(folder, self.getCurrentUser()).dict(exclude_none=True) + @access.user + @autoDescribeRoute( + Description( + "Load normalized frame-metadata attachment identities for the panel. " + "The server classifies sidecars by name only and never parses them; the client " + "downloads and parses the bytes." + ).modelParam("id", level=AccessType.READ, **DatasetModelParam) + ) + def get_frame_metadata_sources(self, folder): + return crud_dataset.load_frame_metadata_sources(folder, self.getCurrentUser()) + @access.public(scope=TokenScope.DATA_READ, cookie=True) @autoDescribeRoute( Description("Export all selected datasets") diff --git a/server/dive_tasks/tasks.py b/server/dive_tasks/tasks.py index b9966fab9..1a1930166 100644 --- a/server/dive_tasks/tasks.py +++ b/server/dive_tasks/tasks.py @@ -313,9 +313,15 @@ def _inject_dataset_metadata_file(command, gc, working_dir: Path, params, manage md_item = gc.getItem(metadata_file_item_id) md_dir = utils.make_directory(working_dir / 'metadata_file') gc.downloadItem(metadata_file_item_id, str(md_dir), name=md_item.get('name')) - md_path = md_dir / md_item.get('name') - if md_path.exists(): - append_metadata_file_kwiver_settings(command, md_path, metadata_file_key) + # Locate what actually landed rather than reconstructing md_dir/: girder_client + # nests the download under a directory of that name when the item's file is named differently + # from the item (a sidecar renamed after upload), and it sanitizes the name with + # transformFilename first. Both make the reconstructed path wrong -- and in the nested case it + # is a directory, so an exists() check passes and binds a directory into the KWIVER setting. + # md_dir is created fresh for this item, so anything under it is its content. + downloaded = next((path for path in sorted(md_dir.rglob('*')) if path.is_file()), None) + if downloaded is not None: + append_metadata_file_kwiver_settings(command, downloaded, metadata_file_key) else: manager.write( f'Warning: metadata item {metadata_file_item_id} ' @@ -958,7 +964,9 @@ def convert_calibration(self: Task, itemId: str): folder = gc.getFolder(folder_id) multi_cam = (folder.get('meta') or {}).get(constants.MultiCamMarker) or {} if str(multi_cam.get(constants.CalibrationItemIdMarker)) != str(itemId): - manager.write('Calibration source was replaced before linking JSON; discarding output.\n') + manager.write( + 'Calibration source was replaced before linking JSON; discarding output.\n' + ) gc.delete(f'item/{json_item_id}') return diff --git a/server/dive_tasks/utils.py b/server/dive_tasks/utils.py index 72e67a237..f8aed45cf 100644 --- a/server/dive_tasks/utils.py +++ b/server/dive_tasks/utils.py @@ -344,7 +344,13 @@ def upload_zipped_flat_media_files( working_directory: Path, create_subfolder=False, ): - """Takes a flat folder of media files and/or annotation and generates a dataset from it""" + """ + Takes a flat folder of media files and/or annotation and generates a dataset from it. + + validate_files is a gate on the whole zip here, not a per-file filter: the type and the + roles it reports are used, then every extracted file is uploaded, including anything the + response gives the ``ignored`` role. Only the interactive browser upload drops those. + """ listOfFileNames = os.listdir(working_directory) validation = gc.sendRestRequest('POST', '/dive_dataset/validate_files', json=listOfFileNames) root_folderId = folderId @@ -393,6 +399,54 @@ def _load_exported_dataset_meta(working_directory: Path) -> dict: return meta +def _archive_metadata_attachment(scope_directory: Path) -> Optional[Path]: + """ + The metadata attachment an export wrote for one dataset scope. + + Attachments are discovered by directory alone: the single file anywhere under + ``/metadata/`` (a camera scope is the camera's own directory). meta.json + carries no locator, so any metadata key an older export left there is ignored. + The walk is recursive so an archive rewritten by a tool that nested the file is + still imported, matching ``archiveMetadataAttachment`` in + client/platform/desktop/backend/native/common.ts -- same directory rule, same two + error strings. Only the scoping differs: this function is reached solely through + ``_load_exported_dataset_meta`` callers, so an exported scope is already proven, + while the desktop twin runs on any picked folder and proves it itself with + ``isExportedDatasetDirectory``. Keep both gates when changing either side, or an + ordinary media folder that happens to hold a ``metadata/`` subdirectory starts + failing import on one platform only. + """ + metadata_dir = scope_directory / 'metadata' + if not metadata_dir.is_dir(): + return None + attachments = sorted(path for path in metadata_dir.rglob('*') if path.is_file()) + if not attachments: + return None + if len(attachments) > 1: + raise ValueError( + 'More than one metadata file was found in the archive metadata directory. ' + 'Keep one and try again.' + ) + attachment = attachments[0] + if not constants.metadataFileRegex.search(attachment.name): + raise ValueError('Archive metadata attachment must be a JSON, TXT, or CSV file') + return attachment + + +def _upload_archive_metadata_attachment( + gc: GirderClient, + dest_folder_id: str, + attachment: Optional[Path], +) -> Optional[str]: + if attachment is None: + return None + gc.upload(str(attachment), dest_folder_id) + items = list(gc.listItem(dest_folder_id, name=attachment.name)) + if len(items) != 1: + raise ValueError(f'Could not resolve uploaded metadata attachment {attachment.name}') + return str(items[0]['_id']) + + def _import_exported_dataset_directory( gc: GirderClient, manager: JobManager, @@ -403,6 +457,7 @@ def _import_exported_dataset_directory( working_directory = Path(working_directory) list_of_names = os.listdir(working_directory) meta = _load_exported_dataset_meta(working_directory) + metadata_attachment = _archive_metadata_attachment(working_directory) dataset_type = meta[constants.TypeMarker] if dataset_type == constants.MultiType: raise ValueError( @@ -425,7 +480,11 @@ def _import_exported_dataset_directory( shutil.rmtree(aux_path) manager.updateStatus(JobStatus.PUSHING_OUTPUT) - gc.upload(f'{working_directory}/*', dest_folder_id) + for entry in working_directory.iterdir(): + if entry.name == 'metadata': + continue + gc.upload(str(entry), dest_folder_id) + metadata_item_id = _upload_archive_metadata_attachment(gc, dest_folder_id, metadata_attachment) all_files = list(gc.listItem(dest_folder_id)) root_meta = { 'type': dataset_type, @@ -437,6 +496,9 @@ def _import_exported_dataset_directory( 'fps': meta['fps'], 'version': meta['version'], } + if metadata_item_id and metadata_attachment: + root_meta[constants.MetadataFileItemIdMarker] = metadata_item_id + root_meta[constants.MetadataFileOriginalNameMarker] = metadata_attachment.name if dataset_type == constants.VideoType: video = meta['video'] transcoded_video = list(gc.listItem(dest_folder_id, name=video['filename'])) @@ -557,6 +619,10 @@ def upload_exported_multicam_zipped_dataset( with open(multi_cam_path) as f: multi_cam = json.load(f) parent_meta = _load_exported_dataset_meta(working_directory) + # Discovered before anything is created so a bad shared attachment fails the import + # without leaving half a dataset behind. Camera attachments live in each camera's own + # metadata/ directory and are restored by the per-camera import below. + parent_metadata_attachment = _archive_metadata_attachment(working_directory) default_display = multi_cam.get('defaultDisplay') cameras_meta = multi_cam.get('cameras') or {} @@ -608,6 +674,9 @@ def upload_exported_multicam_zipped_dataset( calibration_file_id = _upload_stereo_calibration_files( gc, manager, parent_folder_id, working_directory ) + metadata_file_id = _upload_archive_metadata_attachment( + gc, parent_folder_id, parent_metadata_attachment + ) create_body = { 'name': dataset_name, @@ -620,6 +689,8 @@ def upload_exported_multicam_zipped_dataset( } if calibration_file_id: create_body['calibrationFileId'] = calibration_file_id + if metadata_file_id: + create_body['metadataFileId'] = metadata_file_id manager.write('Finalizing multicamera dataset…\n') gc.sendRestRequest( diff --git a/server/dive_utils/constants.py b/server/dive_utils/constants.py index 048fa68b2..91ef2f263 100644 --- a/server/dive_utils/constants.py +++ b/server/dive_utils/constants.py @@ -122,10 +122,9 @@ # Girder item meta: original stereoscopic calibration upload (npz, yml, etc.) CalibrationFileMarker = "calibrationFile" # Optional per-dataset metadata file (folder marker points at a Girder item id; -# the item itself carries MetadataFileMarker). Applies to single and multicam. +# the owning folder carries the locator). Applies to single and multicam. MetadataFileItemIdMarker = "metadataFileItemId" MetadataFileOriginalNameMarker = "metadataFileOriginalName" -MetadataFileMarker = "metadataFile" # Girder item meta: JSON camera-rig used for calibration display JsonCalibrationFileMarker = "jsonCalibrationFile" AssetstoreSourceMarker = "import_source" diff --git a/server/dive_utils/frame_metadata.py b/server/dive_utils/frame_metadata.py new file mode 100644 index 000000000..6c5a41194 --- /dev/null +++ b/server/dive_utils/frame_metadata.py @@ -0,0 +1,39 @@ +"""Reserved-name predicate for a dataset's frame metadata attachment. + +Keep this mirrored with ``client/dive-common/frameMetadata/naming.ts``, which implements the same +basename predicate over the same six names for the desktop and web importers. A name in this set is +read as a frame metadata attachment instead of being imported as annotations, so the two sides +disagreeing means one platform imports a file the other attaches. The shared fixture +``testutils/framemetadata.spec.json`` pins the truth table for both. + +One term throughout: the file is an *attachment*, and a name in this set *declares* it. +""" + +import re + +FRAME_METADATA_SOURCE_NAMES = { + 'frame-metadata.csv', + 'frame-metadata.json', + 'frame-metadata.txt', + 'frame_metadata.csv', + 'frame_metadata.json', + 'frame_metadata.txt', +} +PATH_SPLIT_RE = re.compile(r'[/\\]') + + +def is_frame_metadata_source_name(name: str) -> bool: + """A frame metadata sidecar is declared by basename.""" + basename = PATH_SPLIT_RE.split(name)[-1] + return basename.lower() in FRAME_METADATA_SOURCE_NAMES + + +def frame_metadata_source_name_query() -> dict: + """Mongo predicate matching a declared frame-metadata sidecar by reserved basename. + + The query-side mirror of is_frame_metadata_source_name: Girder item names are basenames + (no path separators) and ``lowerName`` is the lowercased name, so an exact ``$in`` over the + reserved set is exactly that predicate -- and, unlike a regex, it can use the ``lowerName`` + index. Deriving it from the constant keeps the query from drifting as the reserved set changes. + """ + return {'lowerName': {'$in': sorted(FRAME_METADATA_SOURCE_NAMES)}} diff --git a/server/tests/test_event_frame_metadata.py b/server/tests/test_event_frame_metadata.py new file mode 100644 index 000000000..e5f6ab92f --- /dev/null +++ b/server/tests/test_event_frame_metadata.py @@ -0,0 +1,62 @@ +import types as pytypes +from unittest.mock import patch + +from dive_server import event +from dive_utils.constants import AnnotationFileFutureProcessMarker + + +def _event(info): + return pytypes.SimpleNamespace(info=info) + + +# A real 24-hex ObjectId is required because process_assetstore_import wraps creatorId in ObjectId. +_OWNER_ID = '000000000000000000000000' + + +@patch('dive_server.event.User') +@patch('dive_server.event.Folder') +@patch('dive_server.event.Item') +def test_assetstore_import_leaves_meta_sidecar_unmarked(item_cls, folder_cls, user_cls): + item = {'_id': 'i1', 'name': 'frame_metadata.csv', 'meta': {}, 'folderId': 'f1'} + item_model = item_cls.return_value + item_model.findOne.return_value = item + + event.process_assetstore_import( + _event({'type': 'item', 'importPath': '/data/frame_metadata.csv', 'id': 'i1'}), + {}, + ) + + # Declared sidecar: no future-process marker, no save, no relocation, folder untouched. + assert AnnotationFileFutureProcessMarker not in item['meta'] + item_model.save.assert_not_called() + item_model.move.assert_not_called() + folder_cls.return_value.findOne.assert_not_called() + + +@patch('dive_server.event.User') +@patch('dive_server.event.Folder') +@patch('dive_server.event.Item') +def test_assetstore_import_marks_plain_annotation_csv(item_cls, folder_cls, user_cls): + item = {'_id': 'i1', 'name': 'dets.csv', 'meta': {}, 'folderId': 'f1'} + item_model = item_cls.return_value + item_model.findOne.return_value = item + parent_folder = {'_id': 'f1', 'creatorId': _OWNER_ID, 'baseParentId': None, 'meta': {}} + folder_model = folder_cls.return_value + + def find_one(query): + if query == {'_id': 'f1'}: + return parent_folder + return None # no co-named video folder exists + + folder_model.findOne.side_effect = find_one + user_cls.return_value.findOne.return_value = {'_id': _OWNER_ID} + + event.process_assetstore_import( + _event({'type': 'item', 'importPath': '/data/dets.csv', 'id': 'i1'}), + {}, + ) + + # A plain annotation CSV is still marked for future processing, as on main. + assert item['meta'][AnnotationFileFutureProcessMarker] is True + item_model.save.assert_called_once() + item_model.move.assert_not_called() diff --git a/server/tests/test_frame_metadata_naming.py b/server/tests/test_frame_metadata_naming.py new file mode 100644 index 000000000..95602ef2d --- /dev/null +++ b/server/tests/test_frame_metadata_naming.py @@ -0,0 +1,21 @@ +"""Python parity check for frame-metadata source names. + +``frame_metadata`` / ``frame-metadata`` with a ``.json``, ``.csv``, or ``.txt`` extension is the +single declared-by-name flag that travels every ingestion path. The predicate is mirrored +in Python (``dive_utils.frame_metadata``) and in the shared TypeScript module; both +harnesses assert the same accepted/rejected truth table so the two mirrors stay aligned. +""" + +import json +from pathlib import Path + +from dive_utils.frame_metadata import is_frame_metadata_source_name + +SOURCE_NAMES_FIXTURE = Path(__file__).parents[2] / 'testutils' / 'framemetadata.spec.json' + + +def test_source_names_predicate_matches_shared_truth_table(): + truth_table = json.loads(SOURCE_NAMES_FIXTURE.read_text(encoding='utf-8')) + assert truth_table, 'source_names truth table fixture is empty' + for name, expected in truth_table.items(): + assert is_frame_metadata_source_name(name) is expected, name diff --git a/server/tests/test_frame_metadata_sources.py b/server/tests/test_frame_metadata_sources.py new file mode 100644 index 000000000..a1224fe1d --- /dev/null +++ b/server/tests/test_frame_metadata_sources.py @@ -0,0 +1,629 @@ +from unittest.mock import patch + +from girder.exceptions import AccessException, RestException +import pytest + +from dive_server import crud_dataset +from dive_server.views_dataset import DatasetResource +from dive_utils import constants + + +def _dataset_folder(dataset_type=constants.ImageSequenceType, clone_of=None): + folder = { + '_id': 'dataset-id', + 'name': 'single-camera', + 'meta': {'annotate': True, 'type': dataset_type, 'fps': 5}, + } + if clone_of is not None: + # Only a clone has a media source folder distinct from itself. + folder[constants.ForeignMediaIdMarker] = clone_of + return folder + + +def _root_folder(folder_id: str): + return {'_id': folder_id, 'name': folder_id, 'meta': {}} + + +def _camera_folder( + folder_id: str, + name: str, + dataset_type=constants.ImageSequenceType, + clone_of=None, +): + folder = { + '_id': folder_id, + 'name': name, + 'meta': {'annotate': True, 'type': dataset_type, 'fps': 5}, + } + if clone_of is not None: + folder[constants.ForeignMediaIdMarker] = clone_of + return folder + + +def _multicam_parent_folder(clone_of=None): + folder = { + '_id': 'parent-id', + 'name': 'stereo-camera', + 'meta': { + 'annotate': True, + 'type': constants.MultiType, + 'fps': 5, + 'multiCam': { + 'defaultDisplay': 'port', + 'cameraOrder': ['port', 'starboard'], + 'cameras': { + 'port': {'folderId': 'port-id', 'type': constants.ImageSequenceType}, + 'starboard': { + 'folderId': 'starboard-id', + 'type': constants.ImageSequenceType, + }, + }, + }, + }, + } + if clone_of is not None: + folder[constants.ForeignMediaIdMarker] = clone_of + return folder + + +def _source_item(name: str): + return {'_id': f'{name}-id', 'name': name} + + +def _descriptor(name: str): + return {'itemId': f'{name}-id', 'name': name} + + +def _child_items_by_folder(folder_model, items_by_folder_id): + # Apply the reserved-name filter the way Mongo does, so the mock can never hand the + # resolver an item the real query would have excluded. + def child_items(folder, filters=None): + items = items_by_folder_id.get(folder['_id'], []) + allowed = ((filters or {}).get('lowerName') or {}).get('$in') + if allowed is None: + return items + return [item for item in items if item['name'].lower() in allowed] + + folder_model.childItems.side_effect = child_items + + +def _wire_multicam_folders(folder_model, children): + def load_folder(folder_id, level=None, user=None): + return children.get(folder_id) + + folder_model.load.side_effect = load_folder + + +def _wire_clone_roots(get_clone_root, roots_by_folder_id): + def clone_root(user, folder): + return roots_by_folder_id[folder['_id']] + + get_clone_root.side_effect = clone_root + + +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +@pytest.mark.parametrize( + 'dataset_type', + [constants.ImageSequenceType, constants.VideoType], +) +def test_sources_single_camera_uses_one_reserved_name_fallback( + get_clone_root, + folder_cls, + dataset_type, +): + dataset = _dataset_folder(dataset_type) + get_clone_root.return_value = dataset + folder_cls.return_value.childItems.return_value = [_source_item('frame_metadata.csv')] + + assert crud_dataset.load_frame_metadata_sources(dataset, {'_id': 'user-id'}) == { + 'shared': _descriptor('frame_metadata.csv'), + 'cameras': {}, + } + # The query is the only reserved-name filter on this path: nothing it returns is + # re-checked in Python, so it is pinned literally here. + folder_cls.return_value.childItems.assert_called_once_with( + dataset, + filters={ + 'lowerName': { + '$in': [ + 'frame-metadata.csv', + 'frame-metadata.json', + 'frame-metadata.txt', + 'frame_metadata.csv', + 'frame_metadata.json', + 'frame_metadata.txt', + ] + } + }, + ) + + +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_report_ambiguous_reserved_name_fallback(get_clone_root, folder_cls): + dataset = _dataset_folder() + get_clone_root.return_value = dataset + folder_cls.return_value.childItems.return_value = [ + _source_item('frame_metadata.csv'), + _source_item('frame-metadata.txt'), + ] + + assert crud_dataset.load_frame_metadata_sources(dataset, {'_id': 'user-id'}) == { + 'shared': { + 'name': 'Metadata File', + 'error': 'More than one reserved-name metadata attachment is available.', + }, + 'cameras': {}, + } + + +@patch('dive_server.crud_dataset.Item') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +@pytest.mark.parametrize( + 'dataset_type', + [constants.ImageSequenceType, constants.VideoType], +) +def test_sources_single_camera_reads_explicit_attachment_from_clone_root( + get_clone_root, + item_cls, + dataset_type, +): + dataset = _dataset_folder(dataset_type, clone_of='source-root-id') + source_root = _root_folder('source-root-id') + source_root['meta'].update( + { + constants.MetadataFileItemIdMarker: 'source-item-id', + constants.MetadataFileOriginalNameMarker: 'flight.json', + } + ) + get_clone_root.return_value = source_root + item_cls.return_value.load.return_value = { + '_id': 'source-item-id', + 'folderId': 'source-root-id', + 'name': 'stored.json', + } + + assert crud_dataset.load_frame_metadata_sources(dataset, {'_id': 'user-id'}) == { + 'shared': {'itemId': 'source-item-id', 'name': 'flight.json'}, + 'cameras': {}, + } + + +@patch('dive_server.crud_dataset.Item') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_clone_root_attachment_does_not_need_the_callers_acl( + get_clone_root, + item_cls, +): + """Scope membership is the guard, not the caller's access to the clone root. + + getCloneRoot force-loads the source folder, so the caller may have no READ on it; an + access-checked item load would raise AccessException and 403 the panel for a clone whose + source was unshared, even though crud.valid_images already lists that root's media. + """ + dataset = _dataset_folder(clone_of='source-root-id') + source_root = _root_folder('source-root-id') + source_root['meta'].update( + { + constants.MetadataFileItemIdMarker: 'source-item-id', + constants.MetadataFileOriginalNameMarker: 'flight.json', + } + ) + get_clone_root.return_value = source_root + + def load_item(item_id, level=None, user=None, force=False): + if not force: + raise AccessException('Read access denied for folder source-root-id.') + return {'_id': 'source-item-id', 'folderId': 'source-root-id', 'name': 'stored.json'} + + item_cls.return_value.load.side_effect = load_item + + assert crud_dataset.load_frame_metadata_sources(dataset, {'_id': 'user-id'}) == { + 'shared': {'itemId': 'source-item-id', 'name': 'flight.json'}, + 'cameras': {}, + } + + +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_single_camera_no_attachment_returns_empty(get_clone_root, folder_cls): + dataset = _dataset_folder() + get_clone_root.return_value = dataset + folder_cls.return_value.childItems.return_value = [] + + assert crud_dataset.load_frame_metadata_sources(dataset, {'_id': 'user-id'}) == {'cameras': {}} + + +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_large_image_returns_empty(get_clone_root, folder_cls): + dataset = { + '_id': 'ds', + 'name': 'x', + 'meta': {'annotate': True, 'type': constants.LargeImageType, 'fps': 5}, + } + + result = crud_dataset.load_frame_metadata_sources(dataset, {'_id': 'user-id'}) + + # Non-image-sequence media types never expose sidecars; no folder is even walked. + assert result == {'cameras': {}} + folder_cls.return_value.childItems.assert_not_called() + get_clone_root.assert_not_called() + + +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_multicam_returns_shared_once_and_camera_local_once(get_clone_root, folder_cls): + # A cloned multicam: every folder's attachment lives in its media source root. + parent = _multicam_parent_folder(clone_of='parent-root-id') + port = _camera_folder('port-id', 'port', clone_of='port-root-id') + starboard = _camera_folder('starboard-id', 'starboard', clone_of='starboard-root-id') + parent_root = _root_folder('parent-root-id') + port_root = _root_folder('port-root-id') + starboard_root = _root_folder('starboard-root-id') + user = {'_id': 'user-id'} + + folder_model = folder_cls.return_value + _wire_multicam_folders(folder_model, {'port-id': port, 'starboard-id': starboard}) + _child_items_by_folder( + folder_model, + { + 'port-id': [_source_item('frame_metadata.csv')], + 'port-root-id': [], + 'starboard-id': [], + 'starboard-root-id': [], + 'parent-id': [], + 'parent-root-id': [_source_item('frame-metadata.txt')], + }, + ) + _wire_clone_roots( + get_clone_root, + { + 'parent-id': parent_root, + 'port-id': port_root, + 'starboard-id': starboard_root, + }, + ) + + result = crud_dataset.load_frame_metadata_sources(parent, user) + + assert result == { + 'shared': _descriptor('frame-metadata.txt'), + 'cameras': { + 'port': _descriptor('frame_metadata.csv'), + }, + } + + +@patch('dive_server.crud_dataset.Item') +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_camera_reserved_name_replaces_shared_explicit_attachment( + get_clone_root, + folder_cls, + item_cls, +): + """A camera-local attachment replaces the shared one for that camera, however declared.""" + parent = _multicam_parent_folder() + parent['meta'].update( + { + constants.MetadataFileItemIdMarker: 'shared-id', + constants.MetadataFileOriginalNameMarker: 'shared.csv', + } + ) + port = _camera_folder('port-id', 'port') + starboard = _camera_folder('starboard-id', 'starboard') + parent_root = _root_folder('parent-root-id') + folder_model = folder_cls.return_value + _wire_multicam_folders(folder_model, {'port-id': port, 'starboard-id': starboard}) + _child_items_by_folder( + folder_model, + { + 'port-id': [_source_item('frame_metadata.csv')], + 'port-root-id': [], + 'starboard-id': [], + 'starboard-root-id': [], + }, + ) + item_cls.return_value.load.return_value = { + '_id': 'shared-id', + 'folderId': 'parent-id', + 'name': 'stored.csv', + } + _wire_clone_roots( + get_clone_root, + { + 'parent-id': parent_root, + 'port-id': _root_folder('port-root-id'), + 'starboard-id': _root_folder('starboard-root-id'), + }, + ) + + assert crud_dataset.load_frame_metadata_sources(parent, {'_id': 'user-id'}) == { + 'shared': {'itemId': 'shared-id', 'name': 'shared.csv'}, + 'cameras': {'port': _descriptor('frame_metadata.csv')}, + } + + +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_multicam_skips_large_image_camera(get_clone_root, folder_cls): + parent = _multicam_parent_folder() + port = _camera_folder('port-id', 'port') + starboard = { + '_id': 'starboard-id', + 'name': 'starboard', + 'meta': {'type': constants.LargeImageType}, + } + parent_root = _root_folder('parent-root-id') + user = {'_id': 'user-id'} + + folder_model = folder_cls.return_value + _wire_multicam_folders(folder_model, {'port-id': port, 'starboard-id': starboard}) + _child_items_by_folder( + folder_model, + { + 'port-id': [_source_item('frame_metadata.csv')], + 'port-root-id': [], + 'parent-id': [], + 'parent-root-id': [], + }, + ) + _wire_clone_roots( + get_clone_root, + {'parent-id': parent_root, 'port-id': _root_folder('port-root-id')}, + ) + + result = crud_dataset.load_frame_metadata_sources(parent, user) + + assert result == {'cameras': {'port': _descriptor('frame_metadata.csv')}} + + +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_multicam_includes_video_camera(get_clone_root, folder_cls): + parent = _multicam_parent_folder() + port = _camera_folder('port-id', 'port', constants.VideoType) + starboard = _camera_folder('starboard-id', 'starboard', constants.LargeImageType) + parent_root = _root_folder('parent-root-id') + port_root = _root_folder('port-root-id') + user = {'_id': 'user-id'} + + folder_model = folder_cls.return_value + _wire_multicam_folders(folder_model, {'port-id': port, 'starboard-id': starboard}) + _child_items_by_folder( + folder_model, + { + 'port-id': [_source_item('frame_metadata.csv')], + 'port-root-id': [], + 'parent-id': [], + 'parent-root-id': [], + }, + ) + _wire_clone_roots( + get_clone_root, + {'parent-id': parent_root, 'port-id': port_root}, + ) + + result = crud_dataset.load_frame_metadata_sources(parent, user) + + assert result == {'cameras': {'port': _descriptor('frame_metadata.csv')}} + + +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_multicam_missing_camera_folder_raises_404(get_clone_root, folder_cls): + parent = _multicam_parent_folder() + user = {'_id': 'user-id'} + get_clone_root.return_value = _root_folder('parent-root-id') + _wire_multicam_folders(folder_cls.return_value, {}) + + with pytest.raises(RestException, match='Camera folder for "port" was not found') as exc_info: + crud_dataset.load_frame_metadata_sources(parent, user) + + assert exc_info.value.code == 404 + + +@patch('dive_server.crud_dataset.Item') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_expose_selected_metadata_attachment_once_as_shared(get_clone_root, item_cls): + dataset = _dataset_folder() + dataset['meta'][constants.MetadataFileItemIdMarker] = 'flight_log.csv-id' + dataset['meta'][constants.MetadataFileOriginalNameMarker] = 'flight_log.csv' + user = {'_id': 'user-id'} + get_clone_root.return_value = dataset + item_cls.return_value.load.return_value = { + '_id': 'flight_log.csv-id', + 'folderId': 'dataset-id', + 'name': 'stored.csv', + } + + # The selected metadata attachment is a frame metadata source without another marker or + # association. A pipeline still receives this exact item even if its rows do not match. + assert crud_dataset.load_frame_metadata_sources(dataset, user) == { + 'shared': { + 'itemId': 'flight_log.csv-id', + 'name': 'flight_log.csv', + }, + 'cameras': {}, + } + + +@patch('dive_server.crud_dataset.Item') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_fall_back_to_item_name_when_original_name_is_absent( + get_clone_root, + item_cls, +): + dataset = _dataset_folder() + dataset['meta'][constants.MetadataFileItemIdMarker] = 'flight_log.csv-id' + user = {'_id': 'user-id'} + get_clone_root.return_value = dataset + item_cls.return_value.load.return_value = { + '_id': 'flight_log.csv-id', + 'folderId': 'dataset-id', + 'name': 'flight_log.csv', + } + + assert crud_dataset.load_frame_metadata_sources(dataset, user) == { + 'shared': { + 'itemId': 'flight_log.csv-id', + 'name': 'flight_log.csv', + }, + 'cameras': {}, + } + + +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.Item') +@patch('dive_server.crud_dataset.crud.getCloneRoot') +def test_sources_keep_missing_explicit_attachment_visible_without_reserved_fallback( + get_clone_root, + item_cls, + folder_cls, +): + dataset = _dataset_folder() + dataset['meta'][constants.MetadataFileItemIdMarker] = 'missing-id' + dataset['meta'][constants.MetadataFileOriginalNameMarker] = 'missing.csv' + get_clone_root.return_value = dataset + item_cls.return_value.load.return_value = None + + assert crud_dataset.load_frame_metadata_sources(dataset, {'_id': 'user-id'}) == { + 'shared': { + 'name': 'missing.csv', + 'error': 'Metadata attachment is unavailable.', + }, + 'cameras': {}, + } + folder_cls.return_value.childItems.assert_not_called() + + +@patch('dive_server.crud_dataset.crud.getCloneRoot') +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.Item') +def test_replace_attachment_removes_owned_previous_item_after_record_save( + item_cls, + folder_cls, + get_clone_root, +): + dataset = _dataset_folder() + dataset['meta'][constants.MetadataFileItemIdMarker] = 'old-id' + get_clone_root.return_value = dataset + new_item = {'_id': 'new-id', 'folderId': 'dataset-id', 'name': 'new.csv'} + old_item = {'_id': 'old-id', 'folderId': 'dataset-id', 'name': 'old.csv'} + # The superseded id is read straight off the marker, so the loads are: validate the + # replacement, then load the superseded item for removal. + item_cls.return_value.load.side_effect = [new_item, old_item] + + result = crud_dataset.set_metadata_file({'_id': 'user-id'}, dataset, 'new-id') + + assert result == { + 'metadataFileItemId': 'new-id', + 'metadataFileOriginalName': 'new.csv', + } + folder_cls.return_value.save.assert_called_once_with(dataset) + item_cls.return_value.remove.assert_called_once_with(old_item) + + +@patch('dive_server.crud_dataset.crud.getCloneRoot') +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.Item') +def test_replace_reserved_name_attachment_keeps_it(item_cls, folder_cls, get_clone_root): + """A reserved-name file is the user's own content: it is shadowed, never deleted.""" + dataset = _dataset_folder() + get_clone_root.return_value = dataset + reserved_item = { + '_id': 'frame_metadata.csv-id', + 'folderId': 'dataset-id', + 'name': 'frame_metadata.csv', + } + new_item = {'_id': 'new-id', 'folderId': 'dataset-id', 'name': 'new.csv'} + folder_cls.return_value.childItems.return_value = [reserved_item] + item_cls.return_value.load.side_effect = [new_item] + + crud_dataset.set_metadata_file({'_id': 'user-id'}, dataset, 'new-id') + + item_cls.return_value.remove.assert_not_called() + assert dataset['meta'][constants.MetadataFileItemIdMarker] == 'new-id' + + +@patch('dive_server.crud_dataset.crud.getCloneRoot') +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.Item') +def test_replace_marked_reserved_name_attachment_keeps_it(item_cls, folder_cls, get_clone_root): + """The marker cannot vouch for ownership: process_items records what it discovers in it. + + A reserved-name file uploaded alongside the media is swept, marked, and left in place. + Deleting it here as a superseded attachment would destroy a file the user uploaded, so + ownership is decided by the reserved-name predicate, not by the marker's presence. + """ + dataset = _dataset_folder() + dataset['meta'][constants.MetadataFileItemIdMarker] = 'swept-id' + get_clone_root.return_value = dataset + new_item = {'_id': 'new-id', 'folderId': 'dataset-id', 'name': 'new.csv'} + swept_item = {'_id': 'swept-id', 'folderId': 'dataset-id', 'name': 'frame_metadata.csv'} + item_cls.return_value.load.side_effect = [new_item, swept_item] + + crud_dataset.set_metadata_file({'_id': 'user-id'}, dataset, 'new-id') + + item_cls.return_value.remove.assert_not_called() + assert dataset['meta'][constants.MetadataFileItemIdMarker] == 'new-id' + + +@patch('dive_server.crud_dataset.crud.getCloneRoot') +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.Item') +def test_replace_clone_attachment_does_not_remove_source_item( + item_cls, + folder_cls, + get_clone_root, +): + dataset = _dataset_folder() + dataset[constants.ForeignMediaIdMarker] = 'source-root-id' + dataset['meta'][constants.MetadataFileItemIdMarker] = 'source-id' + get_clone_root.return_value = _root_folder('source-root-id') + new_item = {'_id': 'new-id', 'folderId': 'dataset-id', 'name': 'new.csv'} + source_item = {'_id': 'source-id', 'folderId': 'source-root-id', 'name': 'source.csv'} + item_cls.return_value.load.side_effect = [new_item, source_item] + + crud_dataset.set_metadata_file({'_id': 'user-id'}, dataset, 'new-id') + + folder_cls.return_value.save.assert_called_once_with(dataset) + item_cls.return_value.remove.assert_not_called() + + +@patch('dive_server.crud_dataset.crud.getCloneRoot') +@patch('dive_server.crud_dataset.cherrypy.log') +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.Item') +def test_replace_attachment_stays_successful_when_old_item_cleanup_fails( + item_cls, + folder_cls, + log, + get_clone_root, +): + dataset = _dataset_folder() + dataset['meta'][constants.MetadataFileItemIdMarker] = 'old-id' + get_clone_root.return_value = dataset + new_item = {'_id': 'new-id', 'folderId': 'dataset-id', 'name': 'new.csv'} + old_item = {'_id': 'old-id', 'folderId': 'dataset-id', 'name': 'old.csv'} + item_cls.return_value.load.side_effect = [new_item, old_item] + item_cls.return_value.remove.side_effect = RuntimeError('cleanup failed') + + result = crud_dataset.set_metadata_file({'_id': 'user-id'}, dataset, 'new-id') + + assert result['metadataFileItemId'] == 'new-id' + folder_cls.return_value.save.assert_called_once_with(dataset) + log.assert_called_once() + + +@patch('girder.api.rest.Resource.route') +def test_dataset_resource_registers_frame_metadata_sources_route(route): + with patch('dive_server.views_dataset.Folder'): + resource = DatasetResource('dive_dataset') + + assert any( + call.args == ("GET", (":id", "frame_metadata_sources"), resource.get_frame_metadata_sources) + for call in route.call_args_list + ) diff --git a/server/tests/test_inject_metadata_file.py b/server/tests/test_inject_metadata_file.py new file mode 100644 index 000000000..0b3e0d557 --- /dev/null +++ b/server/tests/test_inject_metadata_file.py @@ -0,0 +1,94 @@ +"""Locating the downloaded frame metadata sidecar for an opt-in pipeline. + +girder_client.downloadItem writes `dest/` only when the item holds exactly one file +whose name equals the item name. Otherwise it creates `dest//` and writes the files +inside. These tests pin that both shapes bind a real file to the KWIVER setting. +""" + +from pathlib import Path + +from dive_tasks.tasks import _inject_dataset_metadata_file + + +class FakeGirderClient: + """Reproduces downloadItem's nest-or-flatten rule over a fake item.""" + + def __init__(self, item_name, file_names): + self.item_name = item_name + self.file_names = file_names + + def getItem(self, _item_id): + return {'name': self.item_name} + + def downloadItem(self, _item_id, dest, name=None): + dest_path = Path(dest) + if len(self.file_names) == 1 and self.file_names[0] == self.item_name: + (dest_path / self.item_name).write_text('frame,lat\n0,-124.6\n') + return + nested = dest_path / self.item_name + nested.mkdir(parents=True, exist_ok=True) + for file_name in self.file_names: + (nested / file_name).write_text('frame,lat\n0,-124.6\n') + + +class RecordingManager: + def __init__(self): + self.messages = [] + + def write(self, message): + self.messages.append(message) + + +def _params(): + return {'metadata_file_item_id': 'item-id', 'metadata_file_key': 'stabilizer:flight_log'} + + +def _setting_path(command): + """The path bound by the single `-s key=path` entry the injection appends.""" + assert len(command) == 1 + return Path(command[0].split('=', 1)[1]) + + +def test_binds_the_file_when_girder_client_writes_it_flat(tmp_path): + gc = FakeGirderClient('frame-metadata.csv', ['frame-metadata.csv']) + command = [] + + _inject_dataset_metadata_file(command, gc, tmp_path, _params(), RecordingManager()) + + bound = _setting_path(command) + assert bound.is_file() + assert bound.name == 'frame-metadata.csv' + + +def test_binds_the_file_when_girder_client_nests_it_under_the_item_name(tmp_path): + # Item renamed after upload: the item is 'flight-log-v2.csv' but its file is still + # 'frame-metadata.csv', so girder_client creates a directory. Reconstructing + # md_dir/ would yield that directory, which exists() accepts. + gc = FakeGirderClient('flight-log-v2.csv', ['frame-metadata.csv']) + command = [] + + _inject_dataset_metadata_file(command, gc, tmp_path, _params(), RecordingManager()) + + bound = _setting_path(command) + assert bound.is_file(), 'must bind the contained file, never the directory' + assert bound.name == 'frame-metadata.csv' + + +def test_warns_when_the_item_downloads_nothing(tmp_path): + gc = FakeGirderClient('nav.csv', []) + command = [] + manager = RecordingManager() + + _inject_dataset_metadata_file(command, gc, tmp_path, _params(), manager) + + assert command == [] + assert 'no downloadable file' in manager.messages[0] + + +def test_is_a_no_op_when_the_pipeline_did_not_opt_in(tmp_path): + gc = FakeGirderClient('nav.csv', ['nav.csv']) + command = [] + + _inject_dataset_metadata_file(command, gc, tmp_path, {}, RecordingManager()) + + assert command == [] diff --git a/server/tests/test_multicam_export_clone.py b/server/tests/test_multicam_export_clone.py index d91e5952d..493a95fad 100644 --- a/server/tests/test_multicam_export_clone.py +++ b/server/tests/test_multicam_export_clone.py @@ -2,6 +2,8 @@ import json from unittest.mock import MagicMock, patch +from girder.exceptions import AccessException + from dive_server import crud_dataset from dive_utils import constants @@ -48,6 +50,14 @@ def _child_folder(folder_id: str, name: str): } +def _source_item(name: str): + return {'_id': f'{name}-id', 'name': name} + + +def _descriptor(name: str): + return {'itemId': f'{name}-id', 'name': name} + + @patch('dive_server.crud_dataset.find_json_calibration_item_id', return_value=None) @patch('dive_server.crud_dataset.find_calibration_item_id', return_value=None) @patch('dive_server.crud_dataset.crud_annotation.clone_annotations') @@ -129,11 +139,90 @@ def test_create_multicam_soft_clone_copies_calibration( clone_cals_mock.assert_called_once() saved_meta = folder_cls.return_value.save.call_args_list[-1][0][0]['meta'] assert saved_meta[constants.MultiCamMarker][constants.CalibrationItemIdMarker] == 'new-cal-src' - assert saved_meta[constants.MultiCamMarker][constants.JsonCalibrationItemIdMarker] == 'new-cal-json' + assert ( + saved_meta[constants.MultiCamMarker][constants.JsonCalibrationItemIdMarker] + == 'new-cal-json' + ) + + +@patch('dive_server.crud_dataset.find_json_calibration_item_id', return_value=None) +@patch('dive_server.crud_dataset.find_calibration_item_id', return_value=None) +@patch('dive_server.crud_dataset.crud_annotation.clone_annotations') +@patch('dive_server.crud_dataset.crud.get_or_create_auxiliary_folder') +@patch('dive_server.crud_dataset._create_single_camera_soft_clone') +@patch('dive_server.crud_dataset.Item') +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud.Folder') +def test_multicam_soft_clone_preserves_shared_parent_frame_metadata_source( + crud_folder_cls, + folder_cls, + item_cls, + create_soft_clone_mock, + _aux, + _clone_ann, + _find_cal, + _find_json_cal, +): + owner = {'login': 'tester'} + source = _multi_parent_folder() + source['meta'][constants.MetadataFileItemIdMarker] = 'shared-id' + source['meta'][constants.MetadataFileOriginalNameMarker] = 'flight-log.csv' + parent = {'_id': 'dest-parent'} + left = _child_folder('left-id', 'left') + right = _child_folder('right-id', 'right') + cloned_left = { + **copy.deepcopy(left), + '_id': 'clone-left-id', + constants.ForeignMediaIdMarker: 'left-id', + } + cloned_right = { + **copy.deepcopy(right), + '_id': 'clone-right-id', + constants.ForeignMediaIdMarker: 'right-id', + } + cloned_parent = copy.deepcopy(source) + cloned_parent['_id'] = 'clone-parent-id' + + folder_model = folder_cls.return_value + folder_model.createFolder.return_value = cloned_parent + folders_by_id = { + 'parent-id': source, + 'left-id': left, + 'right-id': right, + 'clone-left-id': cloned_left, + 'clone-right-id': cloned_right, + } + folder_model.load.side_effect = lambda folder_id, **kwargs: folders_by_id.get(folder_id) + crud_folder_cls.return_value.load.side_effect = lambda folder_id, **kwargs: folders_by_id.get( + folder_id + ) + item_cls.return_value.load.return_value = { + '_id': 'shared-id', + 'folderId': 'parent-id', + 'name': 'stored.csv', + } + # filters is the server-side sidecar pre-filter; the mock ignores it and returns the + # full per-folder list so the is_declared post-filter still decides membership. + folder_model.childItems.side_effect = lambda folder, filters=None: { + 'parent-id': [], + 'clone-parent-id': [], + 'left-id': [], + 'right-id': [], + 'clone-left-id': [], + 'clone-right-id': [], + }.get(folder['_id'], []) + create_soft_clone_mock.side_effect = [cloned_left, cloned_right] + + result = crud_dataset.createSoftClone(owner, source, parent, 'Clone stereo', None) + sources = crud_dataset.load_frame_metadata_sources(result, owner) + + assert sources == { + 'shared': {'itemId': 'shared-id', 'name': 'flight-log.csv'}, + 'cameras': {}, + } @patch('dive_server.crud_dataset._yield_single_dataset_export') -@patch('dive_server.crud_dataset._yield_metadata_file') @patch('dive_server.crud_dataset._yield_calibration_files') @patch('dive_server.crud_dataset.get_multi_cam_media') @patch('dive_server.crud_dataset.Folder') @@ -143,7 +232,6 @@ def test_export_multicam_zip_includes_multicam_json_and_cameras( folder_cls, get_multi_cam_media_mock, yield_cal_mock, - yield_metadata_mock, yield_single_mock, ): parent = _multi_parent_folder() @@ -163,7 +251,6 @@ def add_file_side_effect(_maker, path): zip_gen_cls.return_value = z yield_single_mock.return_value = iter([b'camera-chunk']) yield_cal_mock.return_value = iter([b'cal-chunk']) - yield_metadata_mock.return_value = iter([b'metadata-chunk']) get_multi_cam_media_mock.return_value = MagicMock() stream = crud_dataset.export_datasets_zipstream( @@ -183,12 +270,17 @@ def add_file_side_effect(_maker, path): assert './stereo-dataset/left/' in camera_paths assert './stereo-dataset/right/' in camera_paths yield_cal_mock.assert_called_once() - yield_metadata_mock.assert_called_once_with(z, './stereo-dataset/', parent) - assert b'metadata-chunk' in chunks + # The parent exports no media of its own, but includeMedia still reaches it so its + # shared attachment is emitted by the one owner of that decision. + parent_call = next( + call for call in yield_single_mock.call_args_list if call.args[1] == './stereo-dataset/' + ) + assert parent_call.args[4] is True +@patch('dive_server.crud_dataset.crud.getCloneRoot') @patch('dive_server.crud_dataset.Item') -def test_yield_metadata_file_uses_original_name(item_cls): +def test_yield_metadata_file_uses_original_name(item_cls, get_clone_root): z = MagicMock() def add_file_side_effect(_maker, path): @@ -202,22 +294,109 @@ def add_file_side_effect(_maker, path): constants.MetadataFileOriginalNameMarker: 'flight_log.csv', }, } - item_cls.return_value.findOne.return_value = {'_id': 'md-item-id', 'name': 'renamed.csv'} + get_clone_root.return_value = folder + item_cls.return_value.load.return_value = { + '_id': 'md-item-id', + 'folderId': 'parent-id', + 'name': 'renamed.csv', + } item_cls.return_value.fileList.return_value = [('renamed.csv', MagicMock())] - chunks = list(crud_dataset._yield_metadata_file(z, './stereo-dataset/', folder)) + chunks = list( + crud_dataset._yield_metadata_file(z, './stereo-dataset/', folder, {'login': 'tester'}) + ) z.addFile.assert_called_once() - assert str(z.addFile.call_args.args[1]) == 'stereo-dataset/flight_log.csv' + assert str(z.addFile.call_args.args[1]) == 'stereo-dataset/metadata/flight_log.csv' assert any(b'flight_log.csv' in chunk for chunk in chunks) +@patch('dive_server.crud_dataset.crud.getCloneRoot') +@patch('dive_server.crud_dataset.Folder') @patch('dive_server.crud_dataset.Item') -def test_yield_metadata_file_skips_when_unset(item_cls): +def test_yield_metadata_file_exports_reserved_name_attachment( + item_cls, + folder_cls, + get_clone_root, +): + """An attachment discovered by reserved name survives an export/re-import round trip.""" + z = MagicMock() + + def add_file_side_effect(_maker, path): + yield str(path).encode('utf-8') + + z.addFile.side_effect = add_file_side_effect + folder = {'_id': 'ds-id', 'meta': {}} + get_clone_root.return_value = folder + reserved_item = { + '_id': 'reserved-id', + 'folderId': 'ds-id', + 'name': 'frame_metadata.csv', + } + folder_cls.return_value.childItems.return_value = [reserved_item] + item_cls.return_value.load.return_value = reserved_item + item_cls.return_value.fileList.return_value = [('frame_metadata.csv', MagicMock())] + + list(crud_dataset._yield_metadata_file(z, './ds/', folder, {'login': 'tester'})) + + assert str(z.addFile.call_args.args[1]) == 'ds/metadata/frame_metadata.csv' + + +@patch('dive_server.crud_dataset.crud.getCloneRoot') +@patch('dive_server.crud_dataset.Item') +def test_yield_metadata_file_exports_attachment_on_an_unreadable_clone_root( + item_cls, + get_clone_root, +): + """A clone root the exporter cannot read still contributes its attachment. + + getCloneRoot force-loads the source, so an access-checked item load there raises + AccessException -- which is not a RestException, so it would escape the guard in + export_datasets_zipstream and abort the whole stream instead of listing the dataset in + failed_datasets.txt. The export already streams that root's media unchecked. + """ + z = MagicMock() + + def add_file_side_effect(_maker, path): + yield str(path).encode('utf-8') + + z.addFile.side_effect = add_file_side_effect + folder = {'_id': 'clone-id', 'meta': {}, constants.ForeignMediaIdMarker: 'source-id'} + source_root = { + '_id': 'source-id', + 'meta': { + constants.MetadataFileItemIdMarker: 'md-item-id', + constants.MetadataFileOriginalNameMarker: 'flight_log.csv', + }, + } + get_clone_root.return_value = source_root + + def load_item(item_id, level=None, user=None, force=False): + if not force: + raise AccessException('Read access denied for folder source-id.') + return {'_id': 'md-item-id', 'folderId': 'source-id', 'name': 'renamed.csv'} + + item_cls.return_value.load.side_effect = load_item + item_cls.return_value.fileList.return_value = [('renamed.csv', MagicMock())] + + list(crud_dataset._yield_metadata_file(z, './ds/', folder, {'login': 'tester'})) + + assert str(z.addFile.call_args.args[1]) == 'ds/metadata/flight_log.csv' + + +@patch('dive_server.crud_dataset.crud.getCloneRoot') +@patch('dive_server.crud_dataset.Folder') +@patch('dive_server.crud_dataset.Item') +def test_yield_metadata_file_skips_when_unset(item_cls, folder_cls, get_clone_root): z = MagicMock() - chunks = list(crud_dataset._yield_metadata_file(z, './ds/', {'_id': 'id', 'meta': {}})) + folder = {'_id': 'id', 'meta': {}} + get_clone_root.return_value = folder + folder_cls.return_value.childItems.return_value = [] + + chunks = list(crud_dataset._yield_metadata_file(z, './ds/', folder, {'login': 'tester'})) + assert chunks == [] - item_cls.return_value.findOne.assert_not_called() + item_cls.return_value.load.assert_not_called() z.addFile.assert_not_called() @@ -243,6 +422,8 @@ def test_export_multicam_integration_zip_paths( ): """Build a minimal zip and assert multicam layout from export helpers.""" parent = _multi_parent_folder() + parent['meta'][constants.MetadataFileItemIdMarker] = 'metadata-id' + parent['meta'][constants.MetadataFileOriginalNameMarker] = 'flight_log.csv' left = _child_folder('left-id', 'left') right = _child_folder('right-id', 'right') user = {'login': 'tester'} @@ -265,7 +446,12 @@ def footer(self): zip_gen_cls.return_value = z get_dataset_mock.return_value = MagicMock( - dict=lambda exclude_none=True: {'id': 'parent-id', 'type': constants.MultiType} + dict=lambda exclude_none=True: { + 'id': 'parent-id', + 'type': constants.MultiType, + constants.MetadataFileItemIdMarker: 'metadata-id', + constants.MetadataFileOriginalNameMarker: 'flight_log.csv', + } ) get_media_mock.return_value = MagicMock( dict=lambda exclude_none=True: {'imageData': [], 'video': None} @@ -275,12 +461,24 @@ def footer(self): get_clone_root_mock.side_effect = lambda _user, folder: folder valid_images_mock.return_value = [{'_id': 'img1', 'name': 'left.png'}] item_cls.return_value.fileList.return_value = [('left.png', MagicMock())] + item_cls.return_value.load.return_value = { + '_id': 'metadata-id', + 'folderId': 'parent-id', + 'name': 'renamed.csv', + } def load_folder(folder_id, level=None, user=None): return {'left-id': left, 'right-id': right}.get(folder_id) folder_cls.return_value.load.side_effect = load_folder - folder_cls.return_value.childItems.return_value = [{'name': 'left.png'}] + + def child_items(folder, filters=None, **kwargs): + # The reserved-name query is an $in over basenames; the media walk is a $regex. + if '$in' in (filters or {}).get('lowerName', {}): + return [] + return [{'_id': 'img-item', 'name': 'left.png'}] + + folder_cls.return_value.childItems.side_effect = child_items with patch('dive_server.crud_dataset.get_multi_cam_media') as get_mcm: get_mcm.return_value = MagicMock() @@ -300,6 +498,13 @@ def load_folder(folder_id, level=None, user=None): assert 'stereo-dataset/right/meta.json' in zip_entries multi_cam = json.loads(zip_entries['stereo-dataset/multiCam.json'].decode()) assert multi_cam['defaultDisplay'] == 'left' + parent_meta = json.loads(zip_entries['stereo-dataset/meta.json'].decode()) + # The archive carries no attachment locator at all -- neither the server-local item id + # nor the name -- because it is discovered at metadata/. Same key set the + # desktop exporter writes (withoutMetadataAttachment in multicamExport.ts). + assert constants.MetadataFileItemIdMarker not in parent_meta + assert constants.MetadataFileOriginalNameMarker not in parent_meta + assert 'stereo-dataset/metadata/flight_log.csv' in zip_entries @patch('dive_server.crud_dataset.Folder') diff --git a/server/tests/test_multicam_zip_import.py b/server/tests/test_multicam_zip_import.py index 06f7cc27e..d39744b83 100644 --- a/server/tests/test_multicam_zip_import.py +++ b/server/tests/test_multicam_zip_import.py @@ -16,7 +16,18 @@ from dive_utils import constants -def _write_image_sequence_export(target: Path, images: list[str], fps: float = 5.0): +def _write_image_sequence_export( + target: Path, + images: list[str], + fps: float = 5.0, + metadata_name: str | None = None, + extra_meta: dict | None = None, +): + """Write an exported image-sequence dataset directory. + + The archive declares its attachment by directory alone, so meta.json carries no + locator: the attachment is whatever single file sits in ``metadata/``. + """ target.mkdir(parents=True, exist_ok=True) (target / 'frame0.png').write_bytes(b'png') meta = { @@ -24,14 +35,27 @@ def _write_image_sequence_export(target: Path, images: list[str], fps: float = 5 'fps': fps, 'version': 1, 'imageData': [{'filename': name} for name in images], + **(extra_meta or {}), } + if metadata_name: + metadata_dir = target / 'metadata' + metadata_dir.mkdir() + (metadata_dir / metadata_name).write_text('filename,depth\nframe0.png,10\n') (target / 'meta.json').write_text(json.dumps(meta)) def _write_multicam_export_tree( - root: Path, *, sub_type: str = 'stereo', with_calibration: bool = True + root: Path, + *, + sub_type: str = 'stereo', + with_calibration: bool = True, + with_metadata: bool = False, ): - _write_image_sequence_export(root / 'left', ['frame0.png']) + _write_image_sequence_export( + root / 'left', + ['frame0.png'], + metadata_name='left.csv' if with_metadata else None, + ) _write_image_sequence_export(root / 'right', ['frame0.png']) multi_cam = { 'defaultDisplay': 'left', @@ -49,6 +73,10 @@ def _write_multicam_export_tree( 'version': 1, 'name': 'stereo-import', } + if with_metadata: + metadata_dir = root / 'metadata' + metadata_dir.mkdir() + (metadata_dir / 'shared.csv').write_text('filename,depth\nframe0.png,20\n') (root / 'meta.json').write_text(json.dumps(parent_meta)) if with_calibration and sub_type == 'stereo': (root / 'calibration.npz').write_bytes(b'npz') @@ -136,3 +164,194 @@ def test_upload_exported_zipped_dataset_redirects_when_multicam_json_present( utils.upload_exported_zipped_dataset(mock_gc, mock_manager, 'parent-id', root, '') multicam_mock.assert_called_once_with(mock_gc, mock_manager, 'parent-id', root, '') + + +def _list_items_by_name(folder_id, name=None): + """Resolve an uploaded item the way girder does, with an id derived from its folder.""" + if name: + return [{'_id': f'{folder_id}-{name}', 'name': name}] + return [] + + +def test_import_exported_dataset_uploads_and_links_the_metadata_directory_attachment( + tmp_path, + mock_gc, + mock_manager, +): + root = tmp_path / 'dataset' + _write_image_sequence_export(root, ['frame0.png'], metadata_name='nav.csv') + mock_gc.listItem.side_effect = _list_items_by_name + + utils._import_exported_dataset_directory(mock_gc, mock_manager, 'dest', root) + + uploaded = [call_args.args[0] for call_args in mock_gc.upload.call_args_list] + assert str(root / 'metadata' / 'nav.csv') in uploaded + assert str(root / 'metadata') not in uploaded + folder_meta = mock_gc.addMetadataToFolder.call_args.args[1] + assert folder_meta[constants.MetadataFileItemIdMarker] == 'dest-nav.csv' + assert folder_meta[constants.MetadataFileOriginalNameMarker] == 'nav.csv' + + +def test_import_exported_dataset_ignores_a_legacy_item_id_in_meta_json( + tmp_path, + mock_gc, + mock_manager, +): + """Archives from today's upstream carry a bare item id and no metadata/ directory.""" + root = tmp_path / 'dataset' + _write_image_sequence_export( + root, + ['frame0.png'], + extra_meta={ + constants.MetadataFileItemIdMarker: '65a140e8a4c218785d408b42', + constants.MetadataFileOriginalNameMarker: 'nav.csv', + }, + ) + (root / 'nav.csv').write_text('filename,depth\nframe0.png,10\n') + mock_gc.listItem.side_effect = _list_items_by_name + + utils._import_exported_dataset_directory(mock_gc, mock_manager, 'dest', root) + + uploaded = [call_args.args[0] for call_args in mock_gc.upload.call_args_list] + assert str(root / 'nav.csv') in uploaded + folder_meta = mock_gc.addMetadataToFolder.call_args.args[1] + assert constants.MetadataFileItemIdMarker not in folder_meta + assert constants.MetadataFileOriginalNameMarker not in folder_meta + + +def test_import_exported_dataset_finds_a_nested_metadata_directory_attachment( + tmp_path, + mock_gc, + mock_manager, +): + """Discovery walks metadata/ recursively, matching desktop's archiveMetadataAttachment.""" + root = tmp_path / 'dataset' + _write_image_sequence_export(root, ['frame0.png']) + nested = root / 'metadata' / 'sub' + nested.mkdir(parents=True) + (nested / 'nav.csv').write_text('filename,depth\nframe0.png,10\n') + mock_gc.listItem.side_effect = _list_items_by_name + + utils._import_exported_dataset_directory(mock_gc, mock_manager, 'dest', root) + + uploaded = [call_args.args[0] for call_args in mock_gc.upload.call_args_list] + assert str(nested / 'nav.csv') in uploaded + folder_meta = mock_gc.addMetadataToFolder.call_args.args[1] + assert folder_meta[constants.MetadataFileItemIdMarker] == 'dest-nav.csv' + assert folder_meta[constants.MetadataFileOriginalNameMarker] == 'nav.csv' + + +@pytest.mark.parametrize('second_attachment', ['extra.csv', 'sub/extra.csv']) +def test_import_exported_dataset_rejects_more_than_one_metadata_file( + tmp_path, + mock_gc, + mock_manager, + second_attachment, +): + """Ambiguity counts across the whole metadata/ tree, flat or nested, as desktop does.""" + root = tmp_path / 'dataset' + _write_image_sequence_export(root, ['frame0.png'], metadata_name='nav.csv') + extra = root / 'metadata' / second_attachment + extra.parent.mkdir(parents=True, exist_ok=True) + extra.write_text('filename,depth\nframe0.png,30\n') + + with pytest.raises(ValueError, match='More than one metadata file was found'): + utils._import_exported_dataset_directory(mock_gc, mock_manager, 'dest', root) + + mock_gc.upload.assert_not_called() + + +def test_import_exported_dataset_rejects_an_unreadable_metadata_extension( + tmp_path, + mock_gc, + mock_manager, +): + root = tmp_path / 'dataset' + _write_image_sequence_export(root, ['frame0.png'], metadata_name='notes.md') + + with pytest.raises(ValueError, match='must be a JSON, TXT, or CSV file'): + utils._import_exported_dataset_directory(mock_gc, mock_manager, 'dest', root) + + mock_gc.upload.assert_not_called() + + +def test_upload_exported_multicam_ignores_a_legacy_item_id_in_meta_json( + tmp_path, + mock_gc, + mock_manager, +): + """A multicam zip from today's upstream declares a bare item id and writes the attachment + at the archive root instead of in metadata/. + + The import must not fail on the legacy key, and the root-level file is dropped: the + multicam path only uploads the calibration file and the discovered metadata/ attachment + from the parent directory. Pinned so the loss is a known cost of discovery-by-directory + rather than an accident. + """ + root = tmp_path / 'multicam-dataset' + _write_multicam_export_tree(root, sub_type='multicam', with_calibration=False) + parent_meta = json.loads((root / 'meta.json').read_text()) + parent_meta[constants.MetadataFileItemIdMarker] = '65a140e8a4c218785d408b42' + (root / 'meta.json').write_text(json.dumps(parent_meta)) + (root / 'shared.csv').write_text('filename,depth\nframe0.png,20\n') + + utils.upload_exported_multicam_zipped_dataset(mock_gc, mock_manager, 'parent-id', root, '') + + (_, _), kwargs = mock_gc.sendRestRequest.call_args + assert 'metadataFileId' not in kwargs['json'] + uploaded = [call_args.args[0] for call_args in mock_gc.upload.call_args_list] + assert str(root / 'shared.csv') not in uploaded + + +def test_upload_exported_multicam_rejects_multiple_attachments_before_mutation( + tmp_path, + mock_gc, + mock_manager, +): + root = tmp_path / 'multicam-dataset' + _write_multicam_export_tree( + root, + sub_type='multicam', + with_calibration=False, + with_metadata=True, + ) + (root / 'metadata' / 'extra.csv').write_text('filename,depth\nframe0.png,30\n') + + with pytest.raises(ValueError, match='More than one metadata file was found'): + utils.upload_exported_multicam_zipped_dataset(mock_gc, mock_manager, 'parent-id', root, '') + + mock_gc.createFolder.assert_not_called() + mock_gc.upload.assert_not_called() + + +def test_upload_exported_multicam_restores_shared_and_camera_metadata( + tmp_path, + mock_gc, + mock_manager, +): + root = tmp_path / 'multicam-dataset' + _write_multicam_export_tree( + root, + sub_type='multicam', + with_calibration=False, + with_metadata=True, + ) + mock_gc.listItem.side_effect = _list_items_by_name + + utils.upload_exported_multicam_zipped_dataset(mock_gc, mock_manager, 'parent-id', root, '') + + (_, _), kwargs = mock_gc.sendRestRequest.call_args + assert kwargs['json']['metadataFileId'] == 'parent-id-shared.csv' + left_meta = next( + call_args.args[1] + for call_args in mock_gc.addMetadataToFolder.call_args_list + if call_args.args[0] == 'left-id' + ) + assert left_meta[constants.MetadataFileItemIdMarker] == 'left-id-left.csv' + assert left_meta[constants.MetadataFileOriginalNameMarker] == 'left.csv' + right_meta = next( + call_args.args[1] + for call_args in mock_gc.addMetadataToFolder.call_args_list + if call_args.args[0] == 'right-id' + ) + assert constants.MetadataFileItemIdMarker not in right_meta diff --git a/server/tests/test_pipeline_discovery.py b/server/tests/test_pipeline_discovery.py index 09c33f761..15df3661a 100644 --- a/server/tests/test_pipeline_discovery.py +++ b/server/tests/test_pipeline_discovery.py @@ -94,3 +94,31 @@ def test_extract_pipe_metadata_requires_calibration(tmp_path: Path): assert metadata['description'] == 'stereo measurement' assert metadata['inputType'] == 'TRACK' assert metadata['outputType'] == 'TRACK' + + +def test_extract_pipe_metadata_parses_metadata_file_key(tmp_path: Path): + pipe = tmp_path / 'detector_stabilize.pipe' + pipe.write_text( + '\n'.join( + [ + '# Description: stabilized detector', + '# Metadata File: stabilizer:flight_log', + '# Input: TRACK', + ] + ) + ) + + metadata = extract_pipe_metadata(pipe) + + # The opt-in header binds the dataset's selected metadata attachment to this KWIVER key. + assert metadata['metadataFileKey'] == 'stabilizer:flight_log' + assert metadata['description'] == 'stabilized detector' + assert metadata['inputType'] == 'TRACK' + + +def test_extract_pipe_metadata_absent_metadata_file_key(tmp_path: Path): + pipe = tmp_path / 'detector_plain.pipe' + pipe.write_text('# Description: plain detector\n# Input: TRACK\n') + + # A pipe that does not opt in leaves the key unset, so no file is injected. + assert 'metadataFileKey' not in extract_pipe_metadata(pipe) diff --git a/server/tests/test_pipeline_metadata_scope.py b/server/tests/test_pipeline_metadata_scope.py new file mode 100644 index 000000000..16ec0a798 --- /dev/null +++ b/server/tests/test_pipeline_metadata_scope.py @@ -0,0 +1,184 @@ +from unittest.mock import patch + +import pytest + +from dive_server.crud_rpc import run_pipeline +from dive_utils import constants + +USER = {'_id': 'user-id', 'login': 'someone'} +DETECTOR_PIPELINE = { + 'name': 'stabilizer', + 'type': 'detector', + 'pipe': 'detector_stabilizer.pipe', + 'folderId': None, + 'metadata': {'metadataFileKey': 'stabilizer:flight_log'}, +} +MULTICAM_PIPELINE = { + 'name': 'stereo stabilizer', + 'type': '2-cam', + 'pipe': 'detector_stereo.pipe', + 'folderId': None, + 'metadata': {'metadataFileKey': 'stabilizer:flight_log'}, +} + + +def _image_sequence_folder(folder_id='ds'): + return { + '_id': folder_id, + 'name': folder_id, + 'meta': {'type': constants.ImageSequenceType, 'fps': 5}, + } + + +def _multicam_folder(camera_folder_ids): + return { + '_id': 'parent', + 'name': 'parent', + 'meta': { + 'type': constants.MultiType, + 'fps': 5, + constants.MultiCamMarker: { + 'defaultDisplay': 'left', + 'cameras': { + name: {'folderId': folder_id, 'type': constants.ImageSequenceType} + for name, folder_id in camera_folder_ids.items() + }, + }, + }, + } + + +@pytest.fixture +def pipeline_run_env(): + """Patch everything run_pipeline touches except the metadata attachment resolution.""" + with ( + patch('dive_server.crud_rpc.verify_pipe'), + patch('dive_server.crud_rpc.crud.getCloneRoot'), + patch('dive_server.crud_rpc.crud.get_multicam_parent_folder') as multicam_parent, + patch('dive_server.crud_rpc.crud.get_multicam_camera_name') as camera_name, + patch('dive_server.crud_rpc.crud_dataset.resolve_stereo_calibration_item_id') as cal_id, + patch('dive_server.crud_rpc.crud_dataset.pipeline_requires_calibration') as needs_cal, + patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') as resolve, + patch('dive_server.crud_rpc.Folder') as folder_cls, + patch('dive_server.crud_rpc.Job') as job_cls, + patch('dive_server.crud_rpc.Token'), + patch('dive_server.crud_rpc.Notification'), + patch('dive_server.crud_rpc.tasks') as tasks_module, + patch('dive_server.crud_rpc._persist_async_job_metadata') as persist_job, + ): + multicam_parent.return_value = None + camera_name.return_value = None + cal_id.return_value = None + needs_cal.return_value = False + job_cls.return_value.findOne.return_value = None # no outstanding job + persist_job.return_value = {'_id': 'job-id'} + yield { + 'resolve': resolve, + 'multicam_parent': multicam_parent, + 'camera_name': camera_name, + 'folder_cls': folder_cls, + 'job_cls': job_cls, + 'tasks': tasks_module, + } + + +def _job_params(env): + return env['tasks'].run_pipeline.apply_async.call_args.kwargs['kwargs']['params'] + + +def test_single_dataset_run_binds_its_own_attachment(pipeline_run_env): + folder = _image_sequence_folder() + pipeline_run_env['resolve'].return_value = 'nav-item' + + run_pipeline(USER, folder, DETECTOR_PIPELINE) + + params = _job_params(pipeline_run_env) + assert params['metadata_file_key'] == 'stabilizer:flight_log' + assert params['metadata_file_item_id'] == 'nav-item' + pipeline_run_env['resolve'].assert_called_once_with(folder, USER) + + +def test_camera_run_falls_back_to_the_shared_parent_attachment(pipeline_run_env): + # A single-camera run on a camera folder must still see the attachment the parent owns: + # the panel shows it for that camera, so the pipeline gets the same file. + camera = _image_sequence_folder('left') + parent = _multicam_folder({'left': 'left', 'right': 'right'}) + pipeline_run_env['multicam_parent'].return_value = parent + pipeline_run_env['camera_name'].return_value = 'left' + pipeline_run_env['resolve'].side_effect = lambda scope, user: ( + 'shared-item' if scope is parent else None + ) + + run_pipeline(USER, camera, DETECTOR_PIPELINE) + + assert _job_params(pipeline_run_env)['metadata_file_item_id'] == 'shared-item' + # Camera-local first, shared parent second. + assert [call.args[0] for call in pipeline_run_env['resolve'].call_args_list] == [ + camera, + parent, + ] + + +def test_camera_run_prefers_its_own_camera_attachment(pipeline_run_env): + camera = _image_sequence_folder('left') + parent = _multicam_folder({'left': 'left', 'right': 'right'}) + pipeline_run_env['multicam_parent'].return_value = parent + pipeline_run_env['camera_name'].return_value = 'left' + pipeline_run_env['resolve'].side_effect = lambda scope, user: ( + 'camera-item' if scope is camera else 'shared-item' + ) + + run_pipeline(USER, camera, DETECTOR_PIPELINE) + + assert _job_params(pipeline_run_env)['metadata_file_item_id'] == 'camera-item' + + +def test_multicam_run_falls_back_to_the_default_display_camera_attachment(pipeline_run_env): + # A stereo/multicam run reads the shared attachment first, but a dataset whose only + # attachment sits on the default display camera must not run without it. + parent = _multicam_folder({'left': 'left-id', 'right': 'right-id'}) + camera_folders = { + 'left-id': _image_sequence_folder('left-id'), + 'right-id': _image_sequence_folder('right-id'), + } + pipeline_run_env['folder_cls'].return_value.load.side_effect = ( + lambda folder_id, level=None, user=None: camera_folders[folder_id] + ) + pipeline_run_env['resolve'].side_effect = lambda scope, user: ( + 'camera-item' if scope is camera_folders['left-id'] else None + ) + + run_pipeline(USER, parent, MULTICAM_PIPELINE) + + assert _job_params(pipeline_run_env)['metadata_file_item_id'] == 'camera-item' + assert [call.args[0] for call in pipeline_run_env['resolve'].call_args_list] == [ + parent, + camera_folders['left-id'], + ] + + +def test_declared_key_without_an_attachment_is_reported_in_the_job_log(pipeline_run_env): + # The setting is dropped, so say so in the job log the way the missing-calibration + # notice does, instead of running silently without it. + folder = _image_sequence_folder() + pipeline_run_env['resolve'].return_value = None + + run_pipeline(USER, folder, DETECTOR_PIPELINE) + + params = _job_params(pipeline_run_env) + assert 'metadata_file_item_id' not in params + assert 'metadata_file_key' not in params + log = pipeline_run_env['job_cls'].return_value.updateJob.call_args.kwargs['log'] + assert 'stabilizer:flight_log' in log + assert 'no metadata attachment' in log + + +def test_pipeline_without_a_metadata_key_resolves_nothing(pipeline_run_env): + folder = _image_sequence_folder() + pipeline = {**DETECTOR_PIPELINE, 'metadata': {}} + + run_pipeline(USER, folder, pipeline) + + assert 'metadata_file_key' not in _job_params(pipeline_run_env) + pipeline_run_env['resolve'].assert_not_called() + pipeline_run_env['job_cls'].return_value.updateJob.assert_not_called() diff --git a/server/tests/test_process_items.py b/server/tests/test_process_items.py new file mode 100644 index 000000000..dc49d5de6 --- /dev/null +++ b/server/tests/test_process_items.py @@ -0,0 +1,453 @@ +from unittest.mock import patch + +from girder.exceptions import RestException +import pytest + +from dive_server.crud_rpc import process_items +from dive_utils import constants, frame_metadata + +VIAME_HEADER = ( + '# 1: Detection or Track-id, 2: Video or Image Identifier, 3: Unique Frame Identifier, ' + '4-7: Img-bbox(TL_x,TL_y,BR_x,BR_y), 8: Detection or Length Confidence, ' + '9: Fish Length, 10-11+: Repeated Species' +) + + +def _viame_csv(filename='image_0001.jpg'): + """A minimal, well-formed VIAME annotation CSV with the DIVE comment header.""" + return f'{VIAME_HEADER}\n0,{filename},0,10,10,50,50,1.0,-1,fish,0.9\n' + + +def _download_side_effect(bytes_by_file_id): + def download(file, headers=False): + return lambda: [bytes_by_file_id[file['_id']]] + + return download + + +def _childfiles_side_effect(file_by_item_id): + def child_files(item): + return iter([file_by_item_id[item['_id']]]) + + return child_files + + +@pytest.mark.parametrize( + 'dataset_type', + [constants.VideoType, constants.LargeImageType, constants.ImageSequenceType], +) +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') +@patch('dive_server.crud_rpc.crud.refresh_folder_document') +@patch('dive_server.crud_rpc.crud_annotation.save_annotations') +@patch('dive_server.crud_rpc.crud.get_or_create_auxiliary_folder') +@patch('dive_server.crud_rpc.File') +@patch('dive_server.crud_rpc.Item') +@patch('dive_server.crud_rpc.Folder') +def test_frame_metadata_csv_is_kept_in_place_for_every_media_type( + folder_cls, + item_cls, + file_cls, + get_auxiliary_folder, + save_annotations, + refresh_folder_document, + resolve_attachment_item_id, + dataset_type, +): + folder = {'_id': 'ds', 'meta': {'type': dataset_type, 'fps': 5}} + item = {'_id': 'item-id', 'name': 'frame_metadata.csv', 'meta': {}} + file = {'_id': 'file-id', 'name': 'frame_metadata.csv', 'exts': ['csv']} + + resolve_attachment_item_id.return_value = 'item-id' + folder_cls.return_value.childItems.return_value = [item] + item_cls.return_value.childFiles.side_effect = _childfiles_side_effect({'item-id': file}) + + warnings = process_items(folder, {'_id': 'user-id'}) + + # Declared by name: marked processed and left in the dataset folder, never imported as + # annotations, moved, or removed. The bytes are never even downloaded. + assert len(warnings) == 1 + assert 'frame_metadata.csv' in warnings[0] + assert 'stays in the dataset folder' in warnings[0] + assert item['meta'][constants.ProcessedMarker] is True + item_cls.return_value.save.assert_called_once_with(item) + item_cls.return_value.move.assert_not_called() + item_cls.return_value.remove.assert_not_called() + save_annotations.assert_not_called() + file_cls.return_value.download.assert_not_called() + # The resolved attachment is recorded on the folder so later consumers need no rescan. + assert folder['meta'][constants.MetadataFileItemIdMarker] == 'item-id' + assert folder['meta'][constants.MetadataFileOriginalNameMarker] == 'frame_metadata.csv' + + +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') +@patch('dive_server.crud_rpc.crud.refresh_folder_document') +@patch('dive_server.crud_rpc.Item') +@patch('dive_server.crud_rpc.Folder') +def test_reserved_txt_sidecar_is_swept_like_the_other_reserved_names( + folder_cls, item_cls, refresh_folder_document, resolve_attachment_item_id +): + folder = {'_id': 'ds', 'meta': {'type': constants.ImageSequenceType, 'fps': 5}} + item = {'_id': 'item-id', 'name': 'frame_metadata.txt', 'meta': {}} + file = {'_id': 'file-id', 'name': 'frame_metadata.txt', 'exts': ['txt']} + + resolve_attachment_item_id.return_value = 'item-id' + folder_cls.return_value.childItems.return_value = [item] + item_cls.return_value.childFiles.side_effect = _childfiles_side_effect({'item-id': file}) + + warnings = process_items(folder, {'_id': 'user-id'}) + + # .txt matches none of the annotation extension regexes, so the reserved-name predicate + # has to be part of the sweep query or a reserved .txt attachment is never discovered + # (AUV flight logs are .txt). + filters = folder_cls.return_value.childItems.call_args.kwargs['filters'] + assert frame_metadata.frame_metadata_source_name_query() in filters['$and'][0]['$or'] + assert len(warnings) == 1 + assert 'frame_metadata.txt' in warnings[0] + assert item['meta'][constants.ProcessedMarker] is True + assert folder['meta'][constants.MetadataFileItemIdMarker] == 'item-id' + + +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') +@patch('dive_server.crud_rpc.crud.refresh_folder_document') +@patch('dive_server.crud_rpc.Item') +@patch('dive_server.crud_rpc.Folder') +def test_swept_sidecar_write_keeps_concurrently_written_folder_keys( + folder_cls, item_cls, refresh_folder_document, resolve_attachment_item_id +): + # An async convert_video job writes annotate / originalFps / ffprobe_info onto the folder + # while this sweep runs. Girder's save is a full-document replace, so the attachment write + # must refresh first or it replaces the folder with its pre-dispatch copy. + folder = {'_id': 'ds', 'meta': {'type': constants.VideoType, 'fps': 5}} + item = {'_id': 'item-id', 'name': 'frame_metadata.csv', 'meta': {}} + file = {'_id': 'file-id', 'name': 'frame_metadata.csv', 'exts': ['csv']} + + def concurrent_job_write(target): + target['meta']['annotate'] = True + target['meta']['originalFps'] = 30 + + resolve_attachment_item_id.return_value = 'item-id' + refresh_folder_document.side_effect = concurrent_job_write + folder_cls.return_value.childItems.return_value = [item] + item_cls.return_value.childFiles.side_effect = _childfiles_side_effect({'item-id': file}) + + process_items(folder, {'_id': 'user-id'}) + + refresh_folder_document.assert_called_once_with(folder) + saved = folder_cls.return_value.save.call_args.args[0] + assert saved['meta']['annotate'] is True + assert saved['meta']['originalFps'] == 30 + assert saved['meta'][constants.MetadataFileItemIdMarker] == 'item-id' + + +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') +@patch('dive_server.crud_rpc.crud.refresh_folder_document') +@patch('dive_server.crud_rpc.crud.get_or_create_auxiliary_folder') +@patch('dive_server.crud_rpc.File') +@patch('dive_server.crud_rpc.Item') +@patch('dive_server.crud_rpc.Folder') +def test_frame_metadata_csv_marked_processed_is_not_reswept( + folder_cls, + item_cls, + file_cls, + get_auxiliary_folder, + refresh_folder_document, + resolve_attachment_item_id, +): + folder = {'_id': 'ds', 'meta': {'type': constants.ImageSequenceType, 'fps': 5}} + item = {'_id': 'item-id', 'name': 'frame_metadata.csv', 'meta': {}} + file = {'_id': 'file-id', 'name': 'frame_metadata.csv', 'exts': ['csv']} + + # Emulate the ProcessedMarker "$ne: True" query filter: a marked sidecar is no longer + # listed, so a later process_items call never re-adjudicates it. + def child_items(_folder, filters=None, sort=None): + if item['meta'].get(constants.ProcessedMarker) is True: + return [] + return [item] + + resolve_attachment_item_id.return_value = 'item-id' + folder_cls.return_value.childItems.side_effect = child_items + item_cls.return_value.childFiles.side_effect = _childfiles_side_effect({'item-id': file}) + + warnings = process_items(folder, {'_id': 'user-id'}) + assert len(warnings) == 1 + assert item['meta'][constants.ProcessedMarker] is True + item_cls.return_value.save.assert_called_once_with(item) + + # Second pass: the marked sidecar is excluded, so it is not re-saved or re-warned. + second_warnings = process_items(folder, {'_id': 'user-id'}) + assert second_warnings == [] + item_cls.return_value.save.assert_called_once() + + +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') +@patch('dive_server.crud_rpc.crud_annotation.save_annotations') +@patch('dive_server.crud_rpc.crud.valid_images') +@patch('dive_server.crud_rpc.crud.get_or_create_auxiliary_folder') +@patch('dive_server.crud_rpc.File') +@patch('dive_server.crud_rpc.Item') +@patch('dive_server.crud_rpc.Folder') +def test_plain_annotation_csv_still_imports( + folder_cls, + item_cls, + file_cls, + get_auxiliary_folder, + valid_images, + save_annotations, + resolve_attachment_item_id, +): + # The keep-in-place guard must not intercept an ordinary annotation CSV: it is still + # moved to the auxiliary folder and its tracks saved. + folder = {'_id': 'ds', 'meta': {'type': constants.ImageSequenceType, 'fps': 5}} + item = {'_id': 'item-id', 'name': 'annotations.csv', 'meta': {}} + file = {'_id': 'file-id', 'name': 'annotations.csv', 'exts': ['csv']} + + resolve_attachment_item_id.return_value = None + folder_cls.return_value.childItems.return_value = [item] + item_cls.return_value.childFiles.side_effect = _childfiles_side_effect({'item-id': file}) + file_cls.return_value.download.side_effect = _download_side_effect( + {'file-id': _viame_csv().encode()} + ) + get_auxiliary_folder.return_value = {'_id': 'aux-id'} + valid_images.return_value = [{'name': 'image_0001.jpg'}, {'name': 'image_0002.jpg'}] + + warnings = process_items(folder, {'_id': 'user-id'}) + + assert warnings == [] + item_cls.return_value.move.assert_called_once_with(item, {'_id': 'aux-id'}) + save_annotations.assert_called_once() + # An imported annotation is not tagged as a kept-in-place sidecar. + item_cls.return_value.save.assert_not_called() + + +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') +@patch('dive_server.crud_rpc.crud_annotation.save_annotations') +@patch('dive_server.crud_rpc.crud.valid_images') +@patch('dive_server.crud_rpc.crud.get_or_create_auxiliary_folder') +@patch('dive_server.crud_rpc.File') +@patch('dive_server.crud_rpc.Item') +@patch('dive_server.crud_rpc.Folder') +def test_two_plain_csvs_import_the_oldest_and_warn_about_the_rest( + folder_cls, + item_cls, + file_cls, + get_auxiliary_folder, + valid_images, + save_annotations, + resolve_attachment_item_id, +): + # This sweep is the convergence point for headless writers (assetstore/S3), where nobody + # picked these files: two annotation CSVs must not fail the folder. The oldest imports and + # the rest are named in a warning, left untouched for a later sweep. + folder = {'_id': 'ds', 'meta': {'type': constants.ImageSequenceType, 'fps': 5}} + oldest = {'_id': 'a', 'name': 'detections.csv', 'meta': {}} + newer = {'_id': 'b', 'name': 'tracks.csv', 'meta': {}} + file_a = {'_id': 'fa', 'name': 'detections.csv', 'exts': ['csv']} + file_b = {'_id': 'fb', 'name': 'tracks.csv', 'exts': ['csv']} + + resolve_attachment_item_id.return_value = None + folder_cls.return_value.childItems.return_value = [oldest, newer] + item_cls.return_value.childFiles.side_effect = _childfiles_side_effect( + {'a': file_a, 'b': file_b} + ) + file_cls.return_value.download.side_effect = _download_side_effect( + {'fa': _viame_csv('image_0001.jpg').encode(), 'fb': _viame_csv('image_0002.jpg').encode()} + ) + get_auxiliary_folder.return_value = {'_id': 'aux-id'} + valid_images.return_value = [{'name': 'image_0001.jpg'}, {'name': 'image_0002.jpg'}] + + warnings = process_items(folder, {'_id': 'user-id'}) + + assert len(warnings) == 1 + assert 'detections.csv' in warnings[0] + assert 'tracks.csv' in warnings[0] + save_annotations.assert_called_once() + item_cls.return_value.move.assert_called_once_with(oldest, {'_id': 'aux-id'}) + item_cls.return_value.remove.assert_not_called() + # The skipped CSV is untouched: unmarked, unmoved, and still available to import. + assert newer['meta'] == {} + + +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') +@patch('dive_server.crud_rpc.crud_annotation.save_annotations') +@patch('dive_server.crud_rpc.Item') +@patch('dive_server.crud_rpc.Folder') +def test_more_than_one_reserved_metadata_item_warns_and_attaches_none( + folder_cls, + item_cls, + save_annotations, + resolve_attachment_item_id, +): + folder = {'_id': 'ds', 'meta': {'type': constants.ImageSequenceType, 'fps': 5}} + items = [ + {'_id': 'a', 'name': 'frame_metadata.csv', 'meta': {}}, + {'_id': 'b', 'name': 'frame-metadata.json', 'meta': {}}, + ] + files = { + 'a': {'_id': 'f-a', 'name': 'frame_metadata.csv', 'exts': ['csv']}, + 'b': {'_id': 'f-b', 'name': 'frame-metadata.json', 'exts': ['json']}, + } + resolve_attachment_item_id.return_value = None + folder_cls.return_value.childItems.return_value = items + item_cls.return_value.childFiles.side_effect = _childfiles_side_effect(files) + + warnings = process_items(folder, {'_id': 'user-id'}) + + # The assetstore/S3 sweep has no file picker and nothing to correct pre-upload, so + # ambiguity degrades to a warning: raising here would strip the folder's retry marker + # and skip the fps finalize. Both files stay put, so deleting one and re-importing lets + # the reserved-name fallback resolve the survivor. + ambiguity = next(w for w in warnings if 'More than one metadata file' in w) + assert 'frame_metadata.csv' in ambiguity + assert 'frame-metadata.json' in ambiguity + assert 'None was attached' in ambiguity + assert constants.MetadataFileItemIdMarker not in folder['meta'] + # Neither is imported as annotations, moved, or removed. + save_annotations.assert_not_called() + item_cls.return_value.move.assert_not_called() + item_cls.return_value.remove.assert_not_called() + folder_cls.return_value.save.assert_not_called() + assert all(item['meta'][constants.ProcessedMarker] is True for item in items) + + +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') +@patch('dive_server.crud_rpc.crud.refresh_folder_document') +@patch('dive_server.crud_rpc.crud_annotation.save_annotations') +@patch('dive_server.crud_rpc.crud.get_or_create_auxiliary_folder') +@patch('dive_server.crud_rpc.File') +@patch('dive_server.crud_rpc.Item') +@patch('dive_server.crud_rpc.Folder') +def test_attached_item_short_circuits_sweep( + folder_cls, + item_cls, + file_cls, + get_auxiliary_folder, + save_annotations, + refresh_folder_document, + resolve_attachment_item_id, +): + folder = { + '_id': 'ds', + 'meta': { + 'type': constants.ImageSequenceType, + 'fps': 5, + constants.MetadataFileItemIdMarker: 'item-id', + }, + } + item = { + '_id': 'item-id', + 'name': 'nav_2024.csv', + 'meta': {}, + } + file = {'_id': 'file-id', 'name': 'nav_2024.csv', 'exts': ['csv']} + + resolve_attachment_item_id.return_value = 'item-id' + folder_cls.return_value.childItems.return_value = [item] + item_cls.return_value.childFiles.side_effect = _childfiles_side_effect({'item-id': file}) + + warnings = process_items(folder, {'_id': 'user-id'}) + + assert len(warnings) == 1 + assert 'stays in the dataset folder' in warnings[0] + assert item['meta'][constants.ProcessedMarker] is True + item_cls.return_value.save.assert_called_once_with(item) + item_cls.return_value.move.assert_not_called() + item_cls.return_value.remove.assert_not_called() + file_cls.return_value.download.assert_not_called() + save_annotations.assert_not_called() + # The attachment is already recorded, so the sweep writes nothing to the folder. + refresh_folder_document.assert_not_called() + folder_cls.return_value.save.assert_not_called() + + +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') +@patch('dive_server.crud_rpc.crud_annotation.save_annotations') +@patch('dive_server.crud_rpc.crud.valid_images') +@patch('dive_server.crud_rpc.crud.get_or_create_auxiliary_folder') +@patch('dive_server.crud_rpc.File') +@patch('dive_server.crud_rpc.Item') +@patch('dive_server.crud_rpc.Folder') +def test_attached_csv_does_not_trip_two_csv_guard( + folder_cls, + item_cls, + file_cls, + get_auxiliary_folder, + valid_images, + save_annotations, + resolve_attachment_item_id, +): + # An attached CSV alongside one plain annotation CSV is not "two + # annotation CSVs": the annotation imports and the sidecar stays put. + folder = { + '_id': 'ds', + 'meta': { + 'type': constants.ImageSequenceType, + 'fps': 5, + constants.MetadataFileItemIdMarker: 'nav', + }, + } + marked = { + '_id': 'nav', + 'name': 'nav_2024.csv', + 'meta': {}, + } + plain = {'_id': 'ann', 'name': 'annotations.csv', 'meta': {}} + file_marked = {'_id': 'f-nav', 'name': 'nav_2024.csv', 'exts': ['csv']} + file_plain = {'_id': 'f-ann', 'name': 'annotations.csv', 'exts': ['csv']} + + resolve_attachment_item_id.return_value = 'nav' + folder_cls.return_value.childItems.return_value = [marked, plain] + item_cls.return_value.childFiles.side_effect = _childfiles_side_effect( + {'nav': file_marked, 'ann': file_plain} + ) + file_cls.return_value.download.side_effect = _download_side_effect( + {'f-ann': _viame_csv().encode()} + ) + get_auxiliary_folder.return_value = {'_id': 'aux-id'} + valid_images.return_value = [{'name': 'image_0001.jpg'}] + + warnings = process_items(folder, {'_id': 'user-id'}) + + assert len(warnings) == 1 + assert 'nav_2024.csv' in warnings[0] + item_cls.return_value.move.assert_called_once_with(plain, {'_id': 'aux-id'}) + save_annotations.assert_called_once() + assert marked['meta'][constants.ProcessedMarker] is True + + +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') +@patch('dive_server.crud_rpc.crud_annotation.save_annotations') +@patch('dive_server.crud_rpc.crud.valid_images') +@patch('dive_server.crud_rpc.crud.get_or_create_auxiliary_folder') +@patch('dive_server.crud_rpc.File') +@patch('dive_server.crud_rpc.Item') +@patch('dive_server.crud_rpc.Folder') +def test_undecodable_plain_csv_fails_loudly_with_rename_hint( + folder_cls, + item_cls, + file_cls, + get_auxiliary_folder, + valid_images, + save_annotations, + resolve_attachment_item_id, +): + # A plain .csv whose bytes are not valid UTF-8 fails the annotation decode; the loud + # failure carries the hint pointing frame metadata users at the upload page field and + # the reserved name. + folder = {'_id': 'ds', 'meta': {'type': constants.ImageSequenceType, 'fps': 5}} + item = {'_id': 'item-id', 'name': 'annotations.csv', 'meta': {}} + file = {'_id': 'file-id', 'name': 'annotations.csv', 'exts': ['csv']} + raw = 'filename,species\nimage_0001.jpg,poisson-\xe9p\xe9e\n'.encode('latin-1') + + resolve_attachment_item_id.return_value = None + folder_cls.return_value.childItems.return_value = [item] + item_cls.return_value.childFiles.side_effect = _childfiles_side_effect({'item-id': file}) + file_cls.return_value.download.side_effect = _download_side_effect({'file-id': raw}) + valid_images.return_value = [{'name': 'image_0001.jpg'}, {'name': 'image_0002.jpg'}] + + with pytest.raises(RestException, match='Failed to import annotations.csv') as excinfo: + process_items(folder, {'_id': 'user-id'}) + + assert 'Metadata File (Optional)' in str(excinfo.value) + assert 'frame-metadata.csv' in str(excinfo.value) + item_cls.return_value.remove.assert_called_once_with(item) + save_annotations.assert_not_called() diff --git a/server/tests/test_update_metadata.py b/server/tests/test_update_metadata.py index 29a7a8ea3..8c8e166f0 100644 --- a/server/tests/test_update_metadata.py +++ b/server/tests/test_update_metadata.py @@ -202,6 +202,7 @@ def test_resolve_imported_dataset_info_does_not_mutate_inputs(): ), ], ) +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') @patch('dive_server.crud_rpc.crud_dataset.update_metadata') @patch('dive_server.crud_rpc.crud.get_or_create_auxiliary_folder') @patch('dive_server.crud_rpc.File') @@ -213,6 +214,7 @@ def test_process_items_resolves_dataset_info_from_dive_configuration_import( file_cls, get_auxiliary_folder, update_metadata, + resolve_attachment_item_id, additive, expected, ): @@ -233,6 +235,9 @@ def test_process_items_resolves_dataset_info_from_dive_configuration_import( } } + # The attachment resolver reaches Mongo through crud_dataset for its reserved-name + # fallback, so it is stubbed here like every other process_items unit test. + resolve_attachment_item_id.return_value = None folder_cls.return_value.childItems.return_value = [item] item_cls.return_value.childFiles.return_value = iter([file]) file_cls.return_value.download.return_value = lambda: [json.dumps(payload).encode()] @@ -250,6 +255,7 @@ def test_process_items_resolves_dataset_info_from_dive_configuration_import( assert verify is False +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') @patch('dive_server.crud_rpc.crud_dataset.update_metadata') @patch('dive_server.crud_rpc.crud.saveImportAttributes') @patch('dive_server.crud_rpc.crud.get_multicam_parent_folder') @@ -265,6 +271,7 @@ def test_process_items_syncs_mutable_config_to_multicam_parent( get_multicam_parent, save_import_attributes, update_metadata, + resolve_attachment_item_id, ): """Camera-targeted DIVE config also updates the multicam parent the viewer reads.""" # Use video so process_items skips valid_images (needs Mongo). @@ -300,6 +307,7 @@ def test_process_items_syncs_mutable_config_to_multicam_parent( 'cameraRegistrationSource': {'model': 'from-import'}, } + resolve_attachment_item_id.return_value = None folder_cls.return_value.childItems.return_value = [item] item_cls.return_value.childFiles.return_value = iter([file]) file_cls.return_value.download.return_value = lambda: [json.dumps(payload).encode()] @@ -335,6 +343,7 @@ def test_process_items_syncs_mutable_config_to_multicam_parent( assert parent_call.args[2] is False +@patch('dive_server.crud_rpc.crud_dataset.resolve_metadata_attachment_item_id') @patch('dive_server.crud_rpc.crud_annotation.save_annotations') @patch('dive_server.crud_rpc.dive.migrate') @patch('dive_server.crud_rpc.viame.load_json_as_track_and_attributes') @@ -356,6 +365,7 @@ def test_process_items_syncs_track_attributes_to_multicam_parent( load_json, migrate, save_annotations, + resolve_attachment_item_id, ): """Annotation imports that derive attributes also union them onto the parent.""" camera_folder = { @@ -392,6 +402,7 @@ def test_process_items_syncs_track_attributes_to_multicam_parent( } } + resolve_attachment_item_id.return_value = None folder_cls.return_value.childItems.return_value = [item] item_cls.return_value.childFiles.return_value = iter([file]) file_cls.return_value.download.return_value = lambda: [json.dumps(payload).encode()] diff --git a/server/tests/test_validate_files.py b/server/tests/test_validate_files.py index dc68fef15..f452ce3bf 100644 --- a/server/tests/test_validate_files.py +++ b/server/tests/test_validate_files.py @@ -8,7 +8,51 @@ def test_response_shape(): result = validate_files(['image_0001.jpg', 'tracks.csv']) assert set(result) == {"ok", "message", "type", "roles", "reasons"} - assert set(result["roles"]) == {"media", "annotations", "datasetConfig", "ignored"} + assert set(result["roles"]) == { + "media", + "annotations", + "datasetConfig", + "frameMetadata", + "ignored", + } + + +def test_image_sequence_with_annotation_csv_and_frame_metadata_csv(): + result = validate_files(['image_0001.jpg', 'tracks.csv', 'frame_metadata.csv']) + + assert result['ok'] is True + assert result['type'] == constants.ImageSequenceType + assert 'tracks.csv' in result['roles']['annotations'] + assert 'frame_metadata.csv' in result['roles']['frameMetadata'] + # The annotation CSV is not misclassified as a sidecar, and the sidecar is not an annotation. + assert 'frame_metadata.csv' not in result['roles']['annotations'] + assert 'tracks.csv' not in result['roles']['frameMetadata'] + assert result['roles']['ignored'] == [] + + +def test_image_sequence_rejects_multiple_reserved_metadata_attachments(): + result = validate_files( + [ + 'image_0001.jpg', + 'frame_metadata.csv', + 'frame_metadata.txt', + 'frame-metadata.csv', + 'frame-metadata.txt', + ] + ) + + assert result['ok'] is False + assert result['message'] == ( + 'More than one metadata file was selected. Choose one file and try again.' + ) + + +def test_image_sequence_classifies_reserved_json_only_as_metadata(): + result = validate_files(['image_0001.jpg', 'frame_metadata.json']) + + assert result['ok'] is True + assert result['roles']['frameMetadata'] == ['frame_metadata.json'] + assert result['roles']['annotations'] == [] def test_image_sequence_with_yaml_annotation(): @@ -63,14 +107,33 @@ def test_annotation_json_and_config_json_are_distinguished(): assert 'config.json' not in result['roles']['annotations'] -def test_every_role_is_populated_for_a_full_selection(): - result = validate_files(['image_0001.jpg', 'tracks.csv', 'meta.json']) +def test_video_with_frame_metadata_csv_accepts_sidecar(): + result = validate_files(['movie.mp4', 'frame_metadata.csv']) + + assert result['ok'] is True + assert result['type'] == constants.VideoType + assert result['roles']['frameMetadata'] == ['frame_metadata.csv'] + assert 'frame_metadata.csv' not in result['roles']['ignored'] + + +def test_large_image_with_frame_metadata_csv_accepts_sidecar(): + # The attachment is stored for every dataset type; read time decides what to do with it. + result = validate_files(['mosaic.tif', 'frame_metadata.csv']) assert result['ok'] is True + assert result['type'] == constants.LargeImageType + assert result['roles']['frameMetadata'] == ['frame_metadata.csv'] + assert result['roles']['ignored'] == [] + + +def test_every_role_is_populated_for_a_full_selection(): + result = validate_files(['image_0001.jpg', 'tracks.csv', 'meta.json', 'frame_metadata.csv']) + assert result['roles'] == { 'media': ['image_0001.jpg'], 'annotations': ['tracks.csv'], 'datasetConfig': ['meta.json'], + 'frameMetadata': ['frame_metadata.csv'], 'ignored': [], } assert result['reasons'] == {} @@ -112,7 +175,9 @@ def test_multiple_videos_with_config_json_is_rejected(): result = validate_files(['a.mp4', 'b.mp4', 'config.json']) assert result['ok'] is False - assert result['message'] == "Annotation upload is not supported when multiple videos are uploaded" + assert ( + result['message'] == "Annotation upload is not supported when multiple videos are uploaded" + ) def test_multiple_videos_without_annotations_is_allowed(): @@ -160,12 +225,13 @@ def test_validate_files_nitf_remains_large_image(): def test_ignored_is_the_exact_complement_of_the_accepted_roles(): # The web client uploads the selection minus `ignored`, so a file that is neither given a # role nor listed as ignored would silently not upload. Pin the partition over a selection - # that reaches every branch: media, annotations, dataset config, and two side files. + # that reaches every branch: media, annotations, dataset config, sidecar, and two side files. files = [ 'image_0001.jpg', 'image_0002.jpg', 'tracks.csv', 'config.json', + 'frame_metadata.csv', 'notes.md', 'thumbnail.png.bak', ] diff --git a/testutils/framemetadata.spec.json b/testutils/framemetadata.spec.json new file mode 100644 index 000000000..647e5b274 --- /dev/null +++ b/testutils/framemetadata.spec.json @@ -0,0 +1,21 @@ +{ + "frame-metadata.txt": true, + "frame-metadata.csv": true, + "frame-metadata.json": true, + "FRAME-METADATA.TXT": true, + "frame_metadata.csv": true, + "frame_metadata.json": true, + "frame_metadata.txt": true, + "FRAME_METADATA.CSV": true, + "left/frame-metadata.txt": true, + "left/frame-metadata.csv": true, + "right\\frame_metadata.csv": true, + "right\\frame_metadata.txt": true, + "nav.meta.csv": false, + "a.b.meta.txt": false, + "nav.csv": false, + "nav.txt": false, + "prefix-frame-metadata.txt": false, + "frame_metadata.csv.bak": false, + ".meta.csv": false +} diff --git a/testutils/multicam.spec.json b/testutils/multicam.spec.json index 95fccfe52..ce35aa4a1 100644 --- a/testutils/multicam.spec.json +++ b/testutils/multicam.spec.json @@ -9,6 +9,41 @@ } }, "/home/user/data": { + "local.csv": "filename,depth\nleft.png,10\n", + "sharedVideos": { + "left.mp4": "", + "right.mp4": "", + "frame_metadata.csv": "filename,depth\nleft.mp4,10\nright.mp4,20\n" + }, + "archiveMulticam": { + "meta.json": "{\"metadataFile\":\"metadata/shared.csv\",\"metadataOriginalName\":\"shared.csv\"}", + "metadata": { + "shared.csv": "filename,depth\nleft1.png,20\n" + }, + "left": { + "left1.png": "", + "meta.json": "{\"metadataFile\":\"metadata/local.csv\",\"metadataOriginalName\":\"local.csv\"}", + "metadata": { + "local.csv": "filename,depth\nleft1.png,10\n" + } + }, + "right": { + "right1.png": "", + "meta.json": "{}" + } + }, + "upstreamArchiveMulticam": { + "meta.json": "{\"metadataFileItemId\": \"64b8f0c2e1d3a40001abcdef\", \"metadataFileOriginalName\": \"flight_log.csv\"}", + "flight_log.csv": "filename,depth\nleft1.png,10\n", + "left": { + "left1.png": "", + "meta.json": "{\"metadataFileItemId\": \"64b8f0c2e1d3a40001abcdee\"}" + }, + "right": { + "right1.png": "", + "meta.json": "{}" + } + }, "stereoLeftRightCombinedImages": { "left1.png": "", "left2.png": "", From 6b2be16266e57e3a70a016947fbcbf90df58a73e Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 24 Jul 2026 19:08:11 -0400 Subject: [PATCH 2/2] Add metadata attachments to web and desktop imports --- client/dive-common/apispec.ts | 38 +- .../components/ImportAnnotations.vue | 17 + .../ImportMultiCamChooseMetadata.vue | 51 ++ .../ImportMultiCamMetadata.vue | 10 +- .../ImportMultiCamMultiFolder.vue | 11 + .../ImportMultiCamSubfolders.vue | 11 + .../useImportMultiCamDialog.ts | 62 +- .../components/importAnnotationsSets.spec.ts | 94 ++- client/dive-common/constants.ts | 3 - client/dive-common/frameMetadata/naming.ts | 17 + .../frameMetadata/readability.spec.ts | 19 + .../dive-common/frameMetadata/readability.ts | 20 + client/platform/desktop/backend/ipcService.ts | 5 + .../desktop/backend/native/common.spec.ts | 671 ++++++++++++++++++ .../platform/desktop/backend/native/common.ts | 408 ++++++++++- .../desktop/backend/native/multiCam.spec.ts | 182 +++++ .../desktop/backend/native/multiCamImport.ts | 59 +- .../backend/native/multicamExport.spec.ts | 207 ++++++ .../desktop/backend/native/multicamExport.ts | 52 +- .../desktop/backend/serializers/viame.ts | 14 +- client/platform/desktop/constants.ts | 8 +- client/platform/desktop/frontend/api.ts | 7 +- .../frontend/components/ImportDialog.vue | 4 +- client/platform/web-girder/App.vue | 2 + .../web-girder/api/dataset.service.spec.ts | 49 ++ .../web-girder/api/dataset.service.ts | 101 +-- .../api/frameMetadata.service.spec.ts | 88 +++ .../web-girder/api/frameMetadata.service.ts | 80 +++ client/platform/web-girder/api/index.ts | 1 + .../web-girder/multicamFileRegistry.spec.ts | 53 +- .../web-girder/multicamFileRegistry.ts | 32 +- .../platform/web-girder/uploadSlots.spec.ts | 24 +- client/platform/web-girder/uploadSlots.ts | 24 +- .../platform/web-girder/views/Upload.spec.ts | 24 +- client/platform/web-girder/views/Upload.vue | 56 +- .../web-girder/views/UploadGirder.vue | 20 +- 36 files changed, 2368 insertions(+), 156 deletions(-) create mode 100644 client/dive-common/components/ImportMultiCamDialog/ImportMultiCamChooseMetadata.vue create mode 100644 client/dive-common/frameMetadata/naming.ts create mode 100644 client/dive-common/frameMetadata/readability.spec.ts create mode 100644 client/dive-common/frameMetadata/readability.ts create mode 100644 client/platform/desktop/backend/native/multicamExport.spec.ts create mode 100644 client/platform/web-girder/api/dataset.service.spec.ts create mode 100644 client/platform/web-girder/api/frameMetadata.service.spec.ts create mode 100644 client/platform/web-girder/api/frameMetadata.service.ts diff --git a/client/dive-common/apispec.ts b/client/dive-common/apispec.ts index 9ac26db83..d5b8ef232 100644 --- a/client/dive-common/apispec.ts +++ b/client/dive-common/apispec.ts @@ -149,6 +149,24 @@ interface FrameImage { timestamp?: number; } +/** One metadata attachment loaded by a platform implementation. */ +interface FrameMetadataAttachmentText { + /** Preserved original name, falling back to the resolved item/path basename. */ + name: string; + /** Present for a readable TXT/CSV attachment. */ + text?: string; + /** Present when the selected locator could not be read. */ + error?: string; +} + +/** Complete normalized attachment response for one dataset. */ +interface FrameMetadataSourcesResponse { + /** Single-camera dataset attachment or multicamera parent attachment. */ + shared?: FrameMetadataAttachmentText; + /** Camera-local attachments only, keyed by camera name. */ + cameras: Record; +} + export interface MultiCamImportFolderArgs { datasetName?: string; // Girder parent folder name (required on web) defaultDisplay: string; // In multicam the default camera to display @@ -163,6 +181,8 @@ export interface MultiCamImportFolderArgs { * dataset's saved camera registration. */ transformFile?: string; + /** Optional camera-local metadata attachment. */ + metadataFile?: string; /** Per-camera media type when cameras differ (e.g. EO JPG + IR TIFF on web). */ type?: 'image-sequence' | 'video' | 'large-image'; /** @@ -208,6 +228,10 @@ interface MediaImportResponse { globPattern: string; mediaConvertList: string[]; } + +/** User-editable datasetInfo stored on the dataset's backing metadata object. */ +type DatasetInfoFields = Record; + /** * The parts of metadata a user should be able to modify. */ @@ -219,7 +243,7 @@ interface DatasetMetaMutable { imageEnhancements?: ImageEnhancements; attributes?: Readonly>; attributeTrackFilters?: Readonly>; - datasetInfo?: Record; + datasetInfo?: DatasetInfoFields; cameraHomographies?: CameraHomographies; cameraCorrespondences?: CameraCorrespondences; cameraTransformTypes?: CameraTransformTypes; @@ -331,6 +355,7 @@ interface Api { loadMetadata(datasetId: string): Promise; loadDetections(datasetId: string, revision?: number, set?: string): Promise; + loadFrameMetadata(datasetId: string): Promise; saveDetections(datasetId: string, args: SaveDetectionsArgs): Promise; saveMetadata(datasetId: string, metadata: DatasetMetaMutable): Promise; @@ -339,7 +364,13 @@ interface Api { args: SaveAttributeTrackFilterArgs): Promise; // Non-Endpoint shared functions openFromDisk(datasetType: DatasetType | 'bulk' | 'calibration' | 'annotation' | 'text' | 'zip' | 'transform' | 'metadata', directory?: boolean): - Promise<{canceled?: boolean; filePaths: string[]; fileList?: File[]; root?: string}>; + Promise<{ + canceled?: boolean; + filePaths: string[]; + fileList?: File[]; + root?: string; + selectionId?: string; + }>; /** Desktop: immediate child directory names under a parent folder (multicam subfolder import). */ listImmediateSubfolders?(parentPath: string): Promise; /** Desktop: subfolders or root-level video files under a parent folder (multicam import). */ @@ -591,6 +622,7 @@ export { AnnotationSchema, Api, DatasetMeta, + DatasetInfoFields, DatasetMetaMutable, DatasetMetaMutableKeys, MulticamSharedMutableKeys, @@ -602,6 +634,8 @@ export { SubType, PipelineParamType, FrameImage, + FrameMetadataAttachmentText, + FrameMetadataSourcesResponse, MultiTrackRecord, MultiGroupRecord, Pipe, diff --git a/client/dive-common/components/ImportAnnotations.vue b/client/dive-common/components/ImportAnnotations.vue index f70721b9d..9424b6cff 100644 --- a/client/dive-common/components/ImportAnnotations.vue +++ b/client/dive-common/components/ImportAnnotations.vue @@ -5,6 +5,7 @@ import { import { useApi } from 'dive-common/apispec'; import { usePrompt } from 'dive-common/vue-utilities/prompt-service'; import { clientSettings, isStereoInteractiveModeEnabled } from 'dive-common/store/settings'; +import isFrameMetadataSourceName from 'dive-common/frameMetadata/naming'; import clearLengthAttributes from 'dive-common/utils/clearLengthAttributes'; import warpAnnotationsAcrossCameras from 'dive-common/utils/warpAnnotationsAcrossCameras'; import { cloneDeep } from 'lodash'; @@ -188,6 +189,22 @@ export default defineComponent({ if (!ret.canceled) { menuOpen.value = false; const path = ret.filePaths[0]; + // A reserved-name sidecar is frame metadata, not annotations. Reject before + // the import call: web uploads the file and postprocesses afterwards, so a + // later check would leave it sitting in the dataset folder. + const name = ret.fileList?.[0]?.name ?? path; + if (isFrameMetadataSourceName(name)) { + await prompt({ + title: 'Not an Annotation File', + text: [ + `"${name.replace(/^.*[\\/]/, '')}" is a frame metadata file, not annotations.`, + 'Attach frame metadata with the "Metadata File (Optional)" field when the dataset is created.', + 'Attached that way, a frame metadata file may have any name.', + ], + positiveButton: 'OK', + }); + return; + } let importFile: boolean | string[] = false; processing.value = true; const set = currentSet.value === 'default' ? undefined : currentSet.value; diff --git a/client/dive-common/components/ImportMultiCamDialog/ImportMultiCamChooseMetadata.vue b/client/dive-common/components/ImportMultiCamDialog/ImportMultiCamChooseMetadata.vue new file mode 100644 index 000000000..99050bddf --- /dev/null +++ b/client/dive-common/components/ImportMultiCamDialog/ImportMultiCamChooseMetadata.vue @@ -0,0 +1,51 @@ + + + diff --git a/client/dive-common/components/ImportMultiCamDialog/ImportMultiCamMetadata.vue b/client/dive-common/components/ImportMultiCamDialog/ImportMultiCamMetadata.vue index f8fbad63f..20de2c793 100644 --- a/client/dive-common/components/ImportMultiCamDialog/ImportMultiCamMetadata.vue +++ b/client/dive-common/components/ImportMultiCamDialog/ImportMultiCamMetadata.vue @@ -1,6 +1,6 @@