From 1395ba874364375705e8cff23016cb59ae20161c Mon Sep 17 00:00:00 2001 From: Owen Date: Thu, 12 Oct 2023 17:22:38 +0100 Subject: [PATCH 01/25] Initial WIP on item uploads --- darwin/future/core/items/uploads.py | 113 ++++++++++++++++++++++++++++ darwin/future/data_objects/item.py | 64 ++++++++++++++++ 2 files changed, 177 insertions(+) create mode 100644 darwin/future/core/items/uploads.py create mode 100644 darwin/future/data_objects/item.py diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py new file mode 100644 index 000000000..d99dfd5a0 --- /dev/null +++ b/darwin/future/core/items/uploads.py @@ -0,0 +1,113 @@ +import asyncio +from pathlib import Path +from typing import List +from uuid import UUID + +from darwin.future.core.client import ClientCore +from darwin.future.data_objects.item import Item + + +async def async_register_upload( + api_client: ClientCore, + team_slug: str, + dataset_slug: str, + item: Item, + path: Path, +) -> None: + """ + Payload example: + { + "dataset_slug": "my-dataset", + "items": [ + { + "layout": { + "slots": [ + "1", + "2", + "3" + ], + "type": "grid", + "version": 1 + }, + "name": "some-item", + "path": "/", + "slots": [ + { + "as_frames": false, + "extract_views": false, + "file_name": "my_image.jpg", + "fps": "native", + "metadata": {}, + "slot_name": "0", + "tags": [ + "tag_class_name1", + "tag_class_name2" + ], + "type": "image" + } + ], + "tags": [ + "tag_class_name1", + "tag_class_name2" + ] + }, + { + "as_frames": false, + "extract_views": false, + "fps": "native", + "metadata": {}, + "name": "some-item", + "path": "/", + "tags": [ + "tag_class_name1", + "tag_class_name2" + ], + "type": "image" + } + ], + "options": { + "force_tiling": false, + "handle_as_slices": false, + "ignore_dicom_layout": false + } + } + """ + # TODO: implement + raise NotImplementedError + + +async def async_create_signed_upload_url( + api_client: ClientCore, +) -> None: + # TODO: implement + raise NotImplementedError + + +async def async_register_and_create_signed_upload_url(api_client: ClientCore) -> None: + # TODO: implement + raise NotImplementedError + + +async def async_confirm_upload(api_client: ClientCore) -> None: + # TODO: implement + raise NotImplementedError + + +def register_upload(api_client: ClientCore) -> None: + # TODO: Implement + raise NotImplementedError + + +def create_signed_upload_url(api_client: ClientCore) -> None: + # TODO: Implement + raise NotImplementedError + + +def register_and_create_signed_upload_url(api_client: ClientCore) -> None: + # TODO: Implement + raise NotImplementedError + + +def confirm_upload(api_client: ClientCore) -> None: + # TODO: Implement + raise NotImplementedError diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py new file mode 100644 index 000000000..7219348f6 --- /dev/null +++ b/darwin/future/data_objects/item.py @@ -0,0 +1,64 @@ +# @see: GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.ExistingItem +from typing import Dict, List, Literal, Optional, Union + +from pydantic import Field, validator + +from darwin.datatypes import NumberLike +from darwin.future.data_objects.pydantic_base import DefaultDarwin +from darwin.future.data_objects.typing import UnknownType + +ItemFrameRate = Union[NumberLike, Literal["native"]] + + +def validate_no_slashes(v: UnknownType) -> str: + assert isinstance(v, str), "Must be a string" + assert len(v) > 0, "cannot be empty" + assert r"^[^/].*$".find(v) != -1, "cannot start with a slash" + + return v + + +class ItemSlot(DefaultDarwin): + # GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.ExistingSlot + + # Required fields + slot_name: str = Field(..., alias="slotName") + storage_key: str = Field(..., alias="storageKey") + + # Optional fields + as_frames: Optional[bool] = Field(None, alias="asFrames") + extract_views: Optional[bool] = Field(None, alias="extractViews") + file_name: str = Field(..., alias="fileName") + fps: Optional[ItemFrameRate] = Field(None, alias="fps") + metadata: Optional[Dict[str, UnknownType]] = Field({}, alias="metadata") + tags: Optional[Union[List[str], Dict[str, str]]] = Field(None, alias="tags") + type: Literal["image", "video", "pdf", "dicom"] = Field(..., alias="type") + + @validator("slot_name") + def validate_slot_name(cls, v: UnknownType) -> str: + assert isinstance(v, str), "slot_name must be a string" + assert len(v) > 0, "slot_name cannot be empty" + return v + + @validator("storage_key") + def validate_storage_key(cls, v: UnknownType) -> str: + return validate_no_slashes(v) + + @validator("fps") + def validate_fps(cls, v: UnknownType) -> ItemFrameRate: + assert isinstance(v, (int, float, str)), "fps must be a number or 'native'" + if isinstance(v, (int, float)): + assert v > 0, "fps must be greater than 0" + if isinstance(v, str): + assert v == "native", "fps must be 'native' or a number greater than 0" + return v + + +class Item(DefaultDarwin): + name: str + path: str + slots: List[ItemSlot] + + @validator("name") + def validate_name(cls, v: UnknownType) -> str: + return validate_no_slashes(v) From 74a5aeef98bb9ffe926fbd3f40b144cebd7d2561 Mon Sep 17 00:00:00 2001 From: Owen Date: Fri, 13 Oct 2023 13:38:56 +0100 Subject: [PATCH 02/25] Further skeletoning of upload endpoint --- darwin/future/core/items/uploads.py | 157 ++++++++++-------- .../tests/core/items/test_upload_items.py | 3 + 2 files changed, 93 insertions(+), 67 deletions(-) create mode 100644 darwin/future/tests/core/items/test_upload_items.py diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index d99dfd5a0..9a4ac23c0 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -1,79 +1,96 @@ import asyncio from pathlib import Path -from typing import List -from uuid import UUID +from typing import Dict, List, Tuple, Union from darwin.future.core.client import ClientCore +from darwin.future.core.types.common import JSONType from darwin.future.data_objects.item import Item +def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List[Dict]: + """ + [ + { + "layout": {"slots": ["1", "2", "3"], "type": "grid", "version": 1}, + "name": "some-item", + "path": "/", + "slots": [ + { + "as_frames": false, + "extract_views": false, + "file_name": "my_image.jpg", + "fps": "native", + "metadata": {}, + "slot_name": "0", + "tags": ["tag_class_name1", "tag_class_name2"], + "type": "image", + } + ], + "tags": ["tag_class_name1", "tag_class_name2"], + }, + { + "as_frames": false, + "extract_views": false, + "fps": "native", + "metadata": {}, + "name": "some-item", + "path": "/", + "tags": ["tag_class_name1", "tag_class_name2"], + "type": "image", + }, + ] + """ + return_list = [] + for item, path in items_and_paths: + base_item = { + "name": getattr(item, "name"), + "path:": str(path), + "tags": getattr(item, "tags", []), + } + + # TODO: Handle slots - not sure how well the Item reflects the needed payload + # It's complex if the item passed is DatasetsV2.ItemRegistration.NewCompositeItem + # and simpler if it's DatasetsV2.ItemRegistration.NewSimpleItem + # TODO: Handle layout + + return_list.append(base_item) + + return return_list + + async def async_register_upload( api_client: ClientCore, team_slug: str, dataset_slug: str, - item: Item, - path: Path, -) -> None: - """ - Payload example: - { - "dataset_slug": "my-dataset", - "items": [ - { - "layout": { - "slots": [ - "1", - "2", - "3" - ], - "type": "grid", - "version": 1 - }, - "name": "some-item", - "path": "/", - "slots": [ - { - "as_frames": false, - "extract_views": false, - "file_name": "my_image.jpg", - "fps": "native", - "metadata": {}, - "slot_name": "0", - "tags": [ - "tag_class_name1", - "tag_class_name2" - ], - "type": "image" - } - ], - "tags": [ - "tag_class_name1", - "tag_class_name2" - ] - }, - { - "as_frames": false, - "extract_views": false, - "fps": "native", - "metadata": {}, - "name": "some-item", - "path": "/", - "tags": [ - "tag_class_name1", - "tag_class_name2" - ], - "type": "image" - } - ], - "options": { - "force_tiling": false, - "handle_as_slices": false, - "ignore_dicom_layout": false - } + items: Union[List[Item], Item], + paths: Union[List[Path], Path], + force_tiling: bool = False, + handle_as_slices: bool = False, + ignore_dicom_layout: bool = False, +) -> JSONType: + if isinstance(items, Item): + items = [items] + if isinstance(paths, Path): + paths = [paths] + + assert len(items) == len(paths), "items and paths must be the same length" + + items_and_paths = list(zip(items, paths)) + payload_items = _build_payload_items(items_and_paths) + + options = { + "force_tiling": force_tiling, + "handle_as_slices": handle_as_slices, + "ignore_dicom_layout": ignore_dicom_layout, } - """ - # TODO: implement - raise NotImplementedError + + payload = { + "dataset_slug": dataset_slug, + "items": payload_items, + "options": options, + } + + return api_client.post(f"/api/v2/teams/{team_slug}/items/register_upload", payload) async def async_create_signed_upload_url( @@ -93,9 +110,15 @@ async def async_confirm_upload(api_client: ClientCore) -> None: raise NotImplementedError -def register_upload(api_client: ClientCore) -> None: - # TODO: Implement - raise NotImplementedError +def register_upload( + api_client: ClientCore, + team_slug: str, + dataset_slug: str, + item: Item, + path: Path, +) -> JSONType: + response = asyncio.run(async_register_upload(api_client, team_slug, dataset_slug, item, path)) + return response def create_signed_upload_url(api_client: ClientCore) -> None: diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py new file mode 100644 index 000000000..601671272 --- /dev/null +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -0,0 +1,3 @@ +from .fixtures import * # noqa: F401,F403 + +... From 4fddf1f5961b92ad68acb111c600f9d8962127e4 Mon Sep 17 00:00:00 2001 From: Owen Date: Mon, 16 Oct 2023 16:19:42 +0100 Subject: [PATCH 03/25] Some implementation of all methods now in place, outline of testing. --- darwin/future/core/items/uploads.py | 223 +++++++++++++----- darwin/future/data_objects/item.py | 2 +- .../tests/core/items/test_upload_items.py | 91 ++++++- 3 files changed, 251 insertions(+), 65 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index 9a4ac23c0..b5bca35cf 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -1,45 +1,70 @@ import asyncio +from concurrent.futures import Future from pathlib import Path -from typing import Dict, List, Tuple, Union +from typing import Coroutine, Dict, List, Tuple, Union +from urllib import response from darwin.future.core.client import ClientCore from darwin.future.core.types.common import JSONType -from darwin.future.data_objects.item import Item +from darwin.future.data_objects.item import Item, ItemSlot +from darwin.future.exceptions import DarwinException +""" + [ + { + "layout": {"slots": ["1", "2", "3"], "type": "grid", "version": 1}, + "name": "some-item", + "path": "/", + "slots": [ + { + "as_frames": false, + "extract_views": false, + "file_name": "my_image.jpg", + "fps": "native", + "metadata": {}, + "slot_name": "0", + "tags": ["tag_class_name1", "tag_class_name2"], + "type": "image", + } + ], + "tags": ["tag_class_name1", "tag_class_name2"], + }, + { + "as_frames": false, + "extract_views": false, + "fps": "native", + "metadata": {}, + "name": "some-item", + "path": "/", + "tags": ["tag_class_name1", "tag_class_name2"], + "type": "image", + }, + ] +""" + +def _build_slots(slots: List[ItemSlot]) -> List[Dict]: + # TODO: implememnt me + return NotImplemented + + +def _build_layout(layout: Dict) -> Dict: + # TODO: implement me + return NotImplemented def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List[Dict]: """ - [ - { - "layout": {"slots": ["1", "2", "3"], "type": "grid", "version": 1}, - "name": "some-item", - "path": "/", - "slots": [ - { - "as_frames": false, - "extract_views": false, - "file_name": "my_image.jpg", - "fps": "native", - "metadata": {}, - "slot_name": "0", - "tags": ["tag_class_name1", "tag_class_name2"], - "type": "image", - } - ], - "tags": ["tag_class_name1", "tag_class_name2"], - }, - { - "as_frames": false, - "extract_views": false, - "fps": "native", - "metadata": {}, - "name": "some-item", - "path": "/", - "tags": ["tag_class_name1", "tag_class_name2"], - "type": "image", - }, - ] + Builds the payload for the items to be registered for upload + + Parameters + ---------- + items_and_paths: List[Tuple[Item, Path]] + + Returns + ------- + List[Dict] + The payload for the items to be registered for upload """ + # TODO: test me return_list = [] for item, path in items_and_paths: base_item = { @@ -53,6 +78,12 @@ def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List[Dict] # and simpler if it's DatasetsV2.ItemRegistration.NewSimpleItem # TODO: Handle layout + if getattr(item, "slots", None): + base_item["slots"] = _build_slots(item) + + if getattr(item, "layout", None): + base_item["layout"] = _build_layout(item) #! FIXME: Type bug here + return_list.append(base_item) return return_list @@ -62,20 +93,38 @@ async def async_register_upload( api_client: ClientCore, team_slug: str, dataset_slug: str, - items: Union[List[Item], Item], - paths: Union[List[Path], Path], + items_and_paths: Union[Tuple[Item, Path], List[Tuple[Item, Path]]], force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, ) -> JSONType: - if isinstance(items, Item): - items = [items] - if isinstance(paths, Path): - paths = [paths] + """ + Registers an upload for a dataset that can then be used to upload files to Darwin + + Parameters + ---------- + api_client: ClientCore + The client to use for the request + team_slug: str + The slug of the team to register the upload for + dataset_slug: str + The slug of the dataset to register the upload for + items_and_paths: Union[Tuple[Item, Path], List[Tuple[Item, Path]]] + A list of tuples of Items and Paths to register for upload + force_tiling: bool + Whether to force tiling for the upload + handle_as_slices: bool + Whether to handle the upload as slices + ignore_dicom_layout: bool + Whether to ignore the dicom layout + """ - assert len(items) == len(paths), "items and paths must be the same length" + if isinstance(items_and_paths, tuple): + items_and_paths = [items_and_paths] + assert all( + (isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths + ), "items must be a list of Items" - items_and_paths = list(zip(items, paths)) payload_items = _build_payload_items(items_and_paths) options = { @@ -95,42 +144,90 @@ async def async_register_upload( async def async_create_signed_upload_url( api_client: ClientCore, -) -> None: - # TODO: implement - raise NotImplementedError + upload_id: str, + team_slug: str, +) -> JSONType: + # TODO: Test me + return api_client.post(f"/api/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={}) + + +async def async_register_and_create_signed_upload_url( + api_client: ClientCore, + team_slug: str, + dataset_slug: str, + items_and_paths: Union[Tuple[Item, Path], List[Tuple[Item, Path]]], + force_tiling: bool = False, + handle_as_slices: bool = False, + ignore_dicom_layout: bool = False, +) -> Coroutine[Future, None, JSONType]: + # TODO: test me + register = ( + await async_register_upload( + api_client, + team_slug, + dataset_slug, + items_and_paths, + force_tiling, + handle_as_slices, + ignore_dicom_layout, + ), + ) + download_id = getattr(register, "id") + if "errors" in register or not download_id: + raise DarwinException(f"Failed to register upload in {__name__}") -async def async_register_and_create_signed_upload_url(api_client: ClientCore) -> None: - # TODO: implement - raise NotImplementedError + return async_create_signed_upload_url(api_client, team_slug, download_id) -async def async_confirm_upload(api_client: ClientCore) -> None: - # TODO: implement - raise NotImplementedError +async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JSONType + return api_client.post(f"/api/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={}) def register_upload( api_client: ClientCore, team_slug: str, dataset_slug: str, - item: Item, - path: Path, + items_and_paths: Union[Tuple[Item, Path], List[Tuple[Item, Path]]], + force_tiling: bool = False, + handle_as_slices: bool = False, + ignore_dicom_layout: bool = False, ) -> JSONType: - response = asyncio.run(async_register_upload(api_client, team_slug, dataset_slug, item, path)) + # TODO: test me + response = asyncio.run(async_register_upload(api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout)) return response -def create_signed_upload_url(api_client: ClientCore) -> None: - # TODO: Implement - raise NotImplementedError - - -def register_and_create_signed_upload_url(api_client: ClientCore) -> None: - # TODO: Implement - raise NotImplementedError +def create_signed_upload_url( + api_client: ClientCore, + upload_id: str, + team_slug: str, +) -> JSONType: + # TODO: test me + return asyncio.run(async_create_signed_upload_url(api_client, upload_id, team_slug)) -def confirm_upload(api_client: ClientCore) -> None: - # TODO: Implement - raise NotImplementedError +def register_and_create_signed_upload_url( + api_client: ClientCore, + team_slug: str, + dataset_slug: str, + item: Item, + path: Path +) -> None: + #TODO: test me + return asyncio.run( + # ! FIXME: Type bug here + async_register_and_create_signed_upload_url( + api_client, + team_slug, + dataset_slug, + item, + path + ) + ) + + +def confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JSONType: + # TODO: test me + response = asyncio.run(async_confirm_upload(api_client, team_slug, upload_id)) + return response diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index 7219348f6..2bfb9cf14 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -13,7 +13,7 @@ def validate_no_slashes(v: UnknownType) -> str: assert isinstance(v, str), "Must be a string" assert len(v) > 0, "cannot be empty" - assert r"^[^/].*$".find(v) != -1, "cannot start with a slash" + assert r"^[^/].*$".find(v) == -1, "cannot start with a slash" return v diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 601671272..6d844a8f1 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -1,3 +1,92 @@ +import asyncio +from typing import Coroutine, List +from unittest.mock import MagicMock, patch + +import pytest +import responses + +import darwin.future.core.items.uploads as uploads +from darwin.future.core.client import ClientCore +from darwin.future.data_objects.item import Item +from darwin.future.meta.objects import base +from darwin.future.tests.core.fixtures import * # noqa: F401,F403 + from .fixtures import * # noqa: F401,F403 -... + +class TestRegisterUpload: + @pytest.fixture + def default_url(self, base_client: ClientCore) -> str: + return f"{base_client.config.base_url}api/v2/teams/{base_client.config.default_team}/items" + + @responses.activate() + @patch.object(uploads, "_build_payload_items") + def test_async_register_uploads_accepts_tuple_or_list_of_tuples( # type: ignore + self, + mock_build_payload_items: MagicMock, + base_client: ClientCore, + default_url: str, + ) -> None: + mock_build_payload_items.return_value = [] + + item = Item(name="name", path="path", slots=[]) + + item_and_path = (item, Path("path")) + items_and_paths = [item_and_path] + + responses.add( + "post", + f"{default_url}/register_upload", + status=200, + json={ + "dataset_slug": "dataset_slug", + "items": [], + "options": {"force_tiling": False, "handle_as_slices": False, "ignore_dicom_layout": False}, + }, + ) + + responses.add( + "post", + f"{default_url}/register_upload", + status=200, + json={ + "dataset_slug": "dataset_slug", + "items": [], + "options": {"force_tiling": False, "handle_as_slices": False, "ignore_dicom_layout": False}, + }, + ) + + tasks: List[Coroutine] = [ + uploads.async_register_upload( + base_client, + "team_slug", + "dataset_slug", + items_and_paths, + ), + uploads.async_register_upload( + base_client, + "team_slug", + "dataset_slug", + item_and_path, + ), + ] + try: + outputs = asyncio.run(tasks[0]), asyncio.run(tasks[1]) + except Exception as e: + print(e) + pytest.fail() + + print(outputs) + + class TestCreateSignedUploadUrl: + ... + + class TestRegisterAndCreateSignedUploadUrl: + ... + + class TestConfirmUpload: + ... + + +if __name__ == "__main__": + pytest.main(["-s", "-v", __file__]) From 8710f8c914b78a2c7ca27c31288dbdbfc8099744 Mon Sep 17 00:00:00 2001 From: Owen Date: Mon, 16 Oct 2023 17:01:35 +0100 Subject: [PATCH 04/25] Linting fixes --- darwin/future/core/items/uploads.py | 50 +++++++------------ .../tests/core/items/test_upload_items.py | 2 +- 2 files changed, 18 insertions(+), 34 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index b5bca35cf..f5ebc786f 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -1,12 +1,10 @@ import asyncio -from concurrent.futures import Future from pathlib import Path -from typing import Coroutine, Dict, List, Tuple, Union -from urllib import response +from typing import Dict, List, Tuple, Union from darwin.future.core.client import ClientCore from darwin.future.core.types.common import JSONType -from darwin.future.data_objects.item import Item, ItemSlot +from darwin.future.data_objects.item import Item from darwin.future.exceptions import DarwinException """ @@ -42,16 +40,18 @@ ] """ -def _build_slots(slots: List[ItemSlot]) -> List[Dict]: + +async def _build_slots(item: Item) -> List[Dict]: # TODO: implememnt me return NotImplemented -def _build_layout(layout: Dict) -> Dict: +async def _build_layout(item: Item) -> Dict: # TODO: implement me return NotImplemented -def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List[Dict]: + +async def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List[Dict]: """ Builds the payload for the items to be registered for upload @@ -79,10 +79,10 @@ def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List[Dict] # TODO: Handle layout if getattr(item, "slots", None): - base_item["slots"] = _build_slots(item) + base_item["slots"] = await _build_slots(item) if getattr(item, "layout", None): - base_item["layout"] = _build_layout(item) #! FIXME: Type bug here + base_item["layout"] = await _build_layout(item) return_list.append(base_item) @@ -121,11 +121,9 @@ async def async_register_upload( if isinstance(items_and_paths, tuple): items_and_paths = [items_and_paths] - assert all( - (isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths - ), "items must be a list of Items" + assert all((isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths), "items must be a list of Items" - payload_items = _build_payload_items(items_and_paths) + payload_items = await _build_payload_items(items_and_paths) options = { "force_tiling": force_tiling, @@ -159,7 +157,7 @@ async def async_register_and_create_signed_upload_url( force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, -) -> Coroutine[Future, None, JSONType]: +) -> JSONType: # TODO: test me register = ( await async_register_upload( @@ -177,10 +175,11 @@ async def async_register_and_create_signed_upload_url( if "errors" in register or not download_id: raise DarwinException(f"Failed to register upload in {__name__}") + # FIXME: Type bug here return async_create_signed_upload_url(api_client, team_slug, download_id) -async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JSONType +async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JSONType: return api_client.post(f"/api/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={}) @@ -207,24 +206,9 @@ def create_signed_upload_url( return asyncio.run(async_create_signed_upload_url(api_client, upload_id, team_slug)) -def register_and_create_signed_upload_url( - api_client: ClientCore, - team_slug: str, - dataset_slug: str, - item: Item, - path: Path -) -> None: - #TODO: test me - return asyncio.run( - # ! FIXME: Type bug here - async_register_and_create_signed_upload_url( - api_client, - team_slug, - dataset_slug, - item, - path - ) - ) +def register_and_create_signed_upload_url(api_client: ClientCore, team_slug: str, dataset_slug: str, item: Item, path: Path) -> JSONType: + # TODO: test me + return asyncio.run(async_register_and_create_signed_upload_url(api_client, team_slug, dataset_slug, item, path)) def confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JSONType: diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 6d844a8f1..9c130ed9b 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -1,4 +1,5 @@ import asyncio +from pathlib import Path from typing import Coroutine, List from unittest.mock import MagicMock, patch @@ -8,7 +9,6 @@ import darwin.future.core.items.uploads as uploads from darwin.future.core.client import ClientCore from darwin.future.data_objects.item import Item -from darwin.future.meta.objects import base from darwin.future.tests.core.fixtures import * # noqa: F401,F403 from .fixtures import * # noqa: F401,F403 From 36f9e6cd9e6e863dcf12da8b2ba2c82c623cd0b1 Mon Sep 17 00:00:00 2001 From: Owen Date: Tue, 17 Oct 2023 17:29:51 +0100 Subject: [PATCH 05/25] All methods implemented, layout helper pending. --- darwin/future/core/items/uploads.py | 255 ++++++++++++++++++++++------ darwin/future/data_objects/item.py | 50 ++++-- 2 files changed, 246 insertions(+), 59 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index f5ebc786f..ae7f4fc5e 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -4,50 +4,65 @@ from darwin.future.core.client import ClientCore from darwin.future.core.types.common import JSONType -from darwin.future.data_objects.item import Item +from darwin.future.data_objects.item import Item, ItemSlot +from darwin.future.data_objects.pydantic_base import DefaultDarwin +from darwin.future.data_objects.typing import UnknownType from darwin.future.exceptions import DarwinException -""" - [ - { - "layout": {"slots": ["1", "2", "3"], "type": "grid", "version": 1}, - "name": "some-item", - "path": "/", - "slots": [ - { - "as_frames": false, - "extract_views": false, - "file_name": "my_image.jpg", - "fps": "native", - "metadata": {}, - "slot_name": "0", - "tags": ["tag_class_name1", "tag_class_name2"], - "type": "image", - } - ], - "tags": ["tag_class_name1", "tag_class_name2"], - }, - { - "as_frames": false, - "extract_views": false, - "fps": "native", - "metadata": {}, - "name": "some-item", - "path": "/", - "tags": ["tag_class_name1", "tag_class_name2"], - "type": "image", - }, - ] -""" - async def _build_slots(item: Item) -> List[Dict]: - # TODO: implememnt me - return NotImplemented + """ + (internal) Builds the slots for an item + + Parameters + ---------- + item: Item + The item to build slots for + + Returns + ------- + List[Dict] + The built slots + """ + # ! testme + + if not item.slots: + return [] + + slots_to_return: List[Dict] = [] + + for slot in item.slots: + slot_dict: Dict[str, UnknownType] = { + "slot_name": slot.slot_name, + "file_name": slot.file_name, + "storage_key": slot.storage_key, + } + + if slot.as_frames: + slot_dict["as_frames"] = slot.as_frames + + if slot.extract_views: + slot_dict["extract_views"] = slot.extract_views + + if slot.fps: + slot_dict["fps"] = slot.fps + + if slot.metadata: + slot_dict["metadata"] = slot.metadata + + if slot.tags: + slot_dict["tags"] = slot.tags + + if slot.type: + slot_dict["type"] = slot.type + + slots_to_return.append(slot_dict) + + return slots_to_return async def _build_layout(item: Item) -> Dict: - # TODO: implement me + # TODO: implement me - START HERE return NotImplemented @@ -73,11 +88,6 @@ async def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List "tags": getattr(item, "tags", []), } - # TODO: Handle slots - not sure how well the Item reflects the needed payload - # It's complex if the item passed is DatasetsV2.ItemRegistration.NewCompositeItem - # and simpler if it's DatasetsV2.ItemRegistration.NewSimpleItem - # TODO: Handle layout - if getattr(item, "slots", None): base_item["slots"] = await _build_slots(item) @@ -121,7 +131,9 @@ async def async_register_upload( if isinstance(items_and_paths, tuple): items_and_paths = [items_and_paths] - assert all((isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths), "items must be a list of Items" + assert all( + (isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths + ), "items must be a list of Items" payload_items = await _build_payload_items(items_and_paths) @@ -145,6 +157,23 @@ async def async_create_signed_upload_url( upload_id: str, team_slug: str, ) -> JSONType: + """ + Asynchronously create a signed upload URL for an upload or uploads + + Parameters + ---------- + api_client: ClientCore + The client to use for the request + team_slug: str + The slug of the team to register the upload for + upload_id: str + The ID of the upload to confirm + + Returns + ------- + JSONType + The response from the API + """ # TODO: Test me return api_client.post(f"/api/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={}) @@ -158,6 +187,31 @@ async def async_register_and_create_signed_upload_url( handle_as_slices: bool = False, ignore_dicom_layout: bool = False, ) -> JSONType: + """ + Asynchronously register and create a signed upload URL for an upload or uploads + + Parameters + ---------- + api_client: ClientCore + The client to use for the request + team_slug: str + The slug of the team to register the upload for + dataset_slug: str + The slug of the dataset to register the upload for + items_and_paths: Union[Tuple[Item, Path], List[Tuple[Item, Path]]] + A list of tuples, or a single tuple of Items and Paths to register for upload + force_tiling: bool + Whether to force tiling for the upload + handle_as_slices: bool + Whether to handle the upload as slices + ignore_dicom_layout: bool + Whether to ignore the dicom layout + + Returns + ------- + JSONType + The response from the API + """ # TODO: test me register = ( await async_register_upload( @@ -175,11 +229,29 @@ async def async_register_and_create_signed_upload_url( if "errors" in register or not download_id: raise DarwinException(f"Failed to register upload in {__name__}") - # FIXME: Type bug here - return async_create_signed_upload_url(api_client, team_slug, download_id) + signed_info = await async_create_signed_upload_url(api_client, team_slug, download_id) + + return signed_info async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JSONType: + """ + Asynchronously confirm an upload/uploads was successful by ID + + Parameters + ---------- + api_client: ClientCore + The client to use for the request + team_slug: str + The slug of the team to confirm the upload for + upload_id: str + The ID of the upload to confirm + + Returns + ------- + JSONType + The response from the API + """ return api_client.post(f"/api/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={}) @@ -192,8 +264,32 @@ def register_upload( handle_as_slices: bool = False, ignore_dicom_layout: bool = False, ) -> JSONType: + """ + Asynchronously register an upload/uploads for a dataset that can then be used to upload files to Darwin + + Parameters + ---------- + api_client: ClientCore + The client to use for the request + team_slug: str + The slug of the team to register the upload for + dataset_slug: str + The slug of the dataset to register the upload for + items_and_paths: Union[Tuple[Item, Path], List[Tuple[Item, Path]]] + A list of tuples, or a single tuple of Items and Paths to register for upload + force_tiling: bool + Whether to force tiling for the upload + handle_as_slices: bool + Whether to handle the upload as slices + ignore_dicom_layout: bool + Whether to ignore the dicom layout + """ # TODO: test me - response = asyncio.run(async_register_upload(api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout)) + response = asyncio.run( + async_register_upload( + api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout + ) + ) return response @@ -202,16 +298,79 @@ def create_signed_upload_url( upload_id: str, team_slug: str, ) -> JSONType: + """ + Create a signed upload URL for an upload or uploads + + Parameters + ---------- + api_client: ClientCore + The client to use for the request + team_slug: str + The slug of the team to register the upload for + upload_id: str + The ID of the upload to confirm + + Returns + ------- + JSONType + The response from the API + """ # TODO: test me return asyncio.run(async_create_signed_upload_url(api_client, upload_id, team_slug)) -def register_and_create_signed_upload_url(api_client: ClientCore, team_slug: str, dataset_slug: str, item: Item, path: Path) -> JSONType: +def register_and_create_signed_upload_url( + api_client: ClientCore, + team_slug: str, + dataset_slug: str, + items_and_paths: Union[List[Tuple[Item, Path]], Tuple[Item, Path]], + force_tiling: bool = False, + handle_as_slices: bool = False, + ignore_dicom_layout: bool = False, +) -> JSONType: + """ + Register and create a signed upload URL for an upload or uploads + + Parameters + ---------- + api_client: ClientCore + The client to use for the request + team_slug: str + The slug of the team to register the upload for + dataset_slug: str + The slug of the dataset to register the upload for + + Returns + ------- + JSONType + The response from the API + """ # TODO: test me - return asyncio.run(async_register_and_create_signed_upload_url(api_client, team_slug, dataset_slug, item, path)) + return asyncio.run( + async_register_and_create_signed_upload_url( + api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout + ) + ) def confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JSONType: + """ + Confirm an upload/uploads was successful by ID + + Parameters + ---------- + api_client: ClientCore + The client to use for the request + team_slug: str + The slug of the team to confirm the upload for + upload_id: str + The ID of the upload to confirm + + Returns + ------- + JSONType + The response from the API + """ # TODO: test me response = asyncio.run(async_confirm_upload(api_client, team_slug, upload_id)) return response diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index 2bfb9cf14..ca89400cf 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -18,21 +18,42 @@ def validate_no_slashes(v: UnknownType) -> str: return v +class ItemLayoutV1(DefaultDarwin): + # GraphotateWeb.Schemas.DatasetsV2.Common.ItemLayoutV1 + + # Required fields + slots: List[str] = Field(...) + type: Literal["grid", "horizontal", "vertical", "simple"] = Field(...) + version: Literal[1] = Field(...) + + +class ItemLayoutV2(DefaultDarwin): + # GraphotateWeb.Schemas.DatasetsV2.Common.ItemLayoutV2 + + # Required fields + slots: List[str] = Field(...) + type: Literal["grid", "horizontal", "vertical", "simple"] = Field(...) + version: Literal[2] = Field(...) + + # Optional fields + layout_shape: List[int] = Field(...) + + class ItemSlot(DefaultDarwin): # GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.ExistingSlot # Required fields - slot_name: str = Field(..., alias="slotName") - storage_key: str = Field(..., alias="storageKey") + slot_name: str = Field(...) + file_name: str = Field(...) + storage_key: str = Field(...) # Optional fields - as_frames: Optional[bool] = Field(None, alias="asFrames") - extract_views: Optional[bool] = Field(None, alias="extractViews") - file_name: str = Field(..., alias="fileName") - fps: Optional[ItemFrameRate] = Field(None, alias="fps") - metadata: Optional[Dict[str, UnknownType]] = Field({}, alias="metadata") - tags: Optional[Union[List[str], Dict[str, str]]] = Field(None, alias="tags") - type: Literal["image", "video", "pdf", "dicom"] = Field(..., alias="type") + as_frames: Optional[bool] = Field(default=False) + extract_views: Optional[bool] = Field(default=False) + fps: Optional[ItemFrameRate] = Field(...) + metadata: Optional[Dict[str, UnknownType]] = Field({}) + tags: Optional[Union[List[str], Dict[str, str]]] = Field([]) + type: Literal["image", "video", "pdf", "dicom"] = Field(...) @validator("slot_name") def validate_slot_name(cls, v: UnknownType) -> str: @@ -40,7 +61,7 @@ def validate_slot_name(cls, v: UnknownType) -> str: assert len(v) > 0, "slot_name cannot be empty" return v - @validator("storage_key") + @validator("file_name") def validate_storage_key(cls, v: UnknownType) -> str: return validate_no_slashes(v) @@ -55,9 +76,16 @@ def validate_fps(cls, v: UnknownType) -> ItemFrameRate: class Item(DefaultDarwin): + # GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.NewItem + + # Required fields name: str + slots: List[ItemSlot] = Field(default=[]) + + # Optional fields path: str - slots: List[ItemSlot] + tags: Optional[Union[List[str], Dict[str, str]]] = Field([]) + layout: Optional[Union[ItemLayoutV1, ItemLayoutV2]] = Field(...) @validator("name") def validate_name(cls, v: UnknownType) -> str: From 4453f1dea7ce68a17d151de7fd0828f2b3ee80e4 Mon Sep 17 00:00:00 2001 From: Owen Date: Tue, 17 Oct 2023 17:34:12 +0100 Subject: [PATCH 06/25] Linting --- darwin/future/core/items/__init__.py | 4 ++-- darwin/future/core/items/uploads.py | 17 ++++------------- darwin/future/meta/objects/base.py | 4 ++-- 3 files changed, 8 insertions(+), 17 deletions(-) diff --git a/darwin/future/core/items/__init__.py b/darwin/future/core/items/__init__.py index 619a680fe..1f8a2e8e1 100644 --- a/darwin/future/core/items/__init__.py +++ b/darwin/future/core/items/__init__.py @@ -1,2 +1,2 @@ -from darwin.future.core.items.get import * -from darwin.future.core.items.move_items import * +from darwin.future.core.items.get import get_item_ids, get_item_ids_stage # noqa: F401 +from darwin.future.core.items.move_items import move_items_to_stage # noqa: F401 diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index ae7f4fc5e..a543d05cb 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -4,8 +4,7 @@ from darwin.future.core.client import ClientCore from darwin.future.core.types.common import JSONType -from darwin.future.data_objects.item import Item, ItemSlot -from darwin.future.data_objects.pydantic_base import DefaultDarwin +from darwin.future.data_objects.item import Item from darwin.future.data_objects.typing import UnknownType from darwin.future.exceptions import DarwinException @@ -131,9 +130,7 @@ async def async_register_upload( if isinstance(items_and_paths, tuple): items_and_paths = [items_and_paths] - assert all( - (isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths - ), "items must be a list of Items" + assert all((isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths), "items must be a list of Items" payload_items = await _build_payload_items(items_and_paths) @@ -285,11 +282,7 @@ def register_upload( Whether to ignore the dicom layout """ # TODO: test me - response = asyncio.run( - async_register_upload( - api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout - ) - ) + response = asyncio.run(async_register_upload(api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout)) return response @@ -347,9 +340,7 @@ def register_and_create_signed_upload_url( """ # TODO: test me return asyncio.run( - async_register_and_create_signed_upload_url( - api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout - ) + async_register_and_create_signed_upload_url(api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout) ) diff --git a/darwin/future/meta/objects/base.py b/darwin/future/meta/objects/base.py index aa51789f1..7a9b84624 100644 --- a/darwin/future/meta/objects/base.py +++ b/darwin/future/meta/objects/base.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Dict, Generic, List, Optional, TypeVar +from typing import Dict, Generic, Optional, TypeVar from darwin.future.core.client import ClientCore from darwin.future.pydantic_base import DefaultDarwin @@ -16,7 +16,7 @@ class MetaBase(Generic[R]): def __init__(self, client: ClientCore, element: R, meta_params: Optional[Param] = None) -> None: self.client = client self._element = element - self.meta_params = meta_params or dict() + self.meta_params = meta_params or {} def __str__(self) -> str: return str(self._element) From 9b5272f1a709d837b08e35574fc3344e65ed4c45 Mon Sep 17 00:00:00 2001 From: Owen Date: Wed, 18 Oct 2023 11:41:50 +0100 Subject: [PATCH 07/25] WIP: Tests for build items --- darwin/future/core/items/uploads.py | 35 ++++++++++++++++--- darwin/future/data_objects/item.py | 8 ++--- .../tests/core/items/test_upload_items.py | 21 ++++++++++- 3 files changed, 54 insertions(+), 10 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index a543d05cb..8a749888d 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -61,8 +61,25 @@ async def _build_slots(item: Item) -> List[Dict]: async def _build_layout(item: Item) -> Dict: - # TODO: implement me - START HERE - return NotImplemented + if not item.layout: + return {} + + if item.layout.version == 1: + return { + "slots": item.layout.slots, + "type": item.layout.type, + "version": item.layout.version, + } + + if item.layout.version == 2: + return { + "slots": item.layout.slots, + "type": item.layout.type, + "version": item.layout.version, + "layout_shape": item.layout.layout_shape, + } + + raise DarwinException(f"Invalid layout version {item.layout.version}") async def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List[Dict]: @@ -130,7 +147,9 @@ async def async_register_upload( if isinstance(items_and_paths, tuple): items_and_paths = [items_and_paths] - assert all((isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths), "items must be a list of Items" + assert all( + (isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths + ), "items must be a list of Items" payload_items = await _build_payload_items(items_and_paths) @@ -282,7 +301,11 @@ def register_upload( Whether to ignore the dicom layout """ # TODO: test me - response = asyncio.run(async_register_upload(api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout)) + response = asyncio.run( + async_register_upload( + api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout + ) + ) return response @@ -340,7 +363,9 @@ def register_and_create_signed_upload_url( """ # TODO: test me return asyncio.run( - async_register_and_create_signed_upload_url(api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout) + async_register_and_create_signed_upload_url( + api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout + ) ) diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index ca89400cf..fe3e06797 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -50,7 +50,7 @@ class ItemSlot(DefaultDarwin): # Optional fields as_frames: Optional[bool] = Field(default=False) extract_views: Optional[bool] = Field(default=False) - fps: Optional[ItemFrameRate] = Field(...) + fps: Optional[ItemFrameRate] = Field(0) metadata: Optional[Dict[str, UnknownType]] = Field({}) tags: Optional[Union[List[str], Dict[str, str]]] = Field([]) type: Literal["image", "video", "pdf", "dicom"] = Field(...) @@ -83,9 +83,9 @@ class Item(DefaultDarwin): slots: List[ItemSlot] = Field(default=[]) # Optional fields - path: str - tags: Optional[Union[List[str], Dict[str, str]]] = Field([]) - layout: Optional[Union[ItemLayoutV1, ItemLayoutV2]] = Field(...) + path: Optional[str] = None + tags: Optional[Union[List[str], Dict[str, str]]] = [] + layout: Optional[Union[ItemLayoutV1, ItemLayoutV2]] = None @validator("name") def validate_name(cls, v: UnknownType) -> str: diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 9c130ed9b..f03afe341 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -1,6 +1,6 @@ import asyncio from pathlib import Path -from typing import Coroutine, List +from typing import Coroutine, Dict, List from unittest.mock import MagicMock, patch import pytest @@ -14,6 +14,25 @@ from .fixtures import * # noqa: F401,F403 +class TestBuildSlots: + items: List[Item] = [ + Item( + name="item with no slots", + slots=[], + ) + ] + expectations: List[List[Dict]] = [[]] + + # Sanity check + assert len(items) == len(expectations) + + items_and_expectations = zip(items, expectations) + + @pytest.mark.parametrize("item,expected", *items_and_expectations) + def test_build_slots(self, item: Item, expected: List[Dict]) -> None: + assert uploads._build_slots(item) == expected + + class TestRegisterUpload: @pytest.fixture def default_url(self, base_client: ClientCore) -> str: From f9d7429889df6ae477601b9b77c8a73b54513bf7 Mon Sep 17 00:00:00 2001 From: Owen Date: Wed, 18 Oct 2023 15:12:31 +0100 Subject: [PATCH 08/25] WIP Tests --- darwin/future/core/items/uploads.py | 12 +- darwin/future/data_objects/item.py | 62 +++- .../tests/core/items/test_upload_items.py | 323 ++++++++++++++++-- 3 files changed, 353 insertions(+), 44 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index 8a749888d..ed0ee0954 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -37,22 +37,22 @@ async def _build_slots(item: Item) -> List[Dict]: "storage_key": slot.storage_key, } - if slot.as_frames: + if slot.as_frames is not None: slot_dict["as_frames"] = slot.as_frames - if slot.extract_views: + if slot.extract_views is not None: slot_dict["extract_views"] = slot.extract_views - if slot.fps: + if slot.fps != 0: slot_dict["fps"] = slot.fps - if slot.metadata: + if slot.metadata is not None: slot_dict["metadata"] = slot.metadata - if slot.tags: + if slot.tags is not None: slot_dict["tags"] = slot.tags - if slot.type: + if slot.type is not None: slot_dict["type"] = slot.type slots_to_return.append(slot_dict) diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index fe3e06797..ab56a7a57 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -1,7 +1,8 @@ # @see: GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.ExistingItem +from pathlib import Path from typing import Dict, List, Literal, Optional, Union -from pydantic import Field, validator +from pydantic import root_validator, validator from darwin.datatypes import NumberLike from darwin.future.data_objects.pydantic_base import DefaultDarwin @@ -22,38 +23,38 @@ class ItemLayoutV1(DefaultDarwin): # GraphotateWeb.Schemas.DatasetsV2.Common.ItemLayoutV1 # Required fields - slots: List[str] = Field(...) - type: Literal["grid", "horizontal", "vertical", "simple"] = Field(...) - version: Literal[1] = Field(...) + slots: List[str] + type: Literal["grid", "horizontal", "vertical", "simple"] + version: Literal[1] class ItemLayoutV2(DefaultDarwin): # GraphotateWeb.Schemas.DatasetsV2.Common.ItemLayoutV2 # Required fields - slots: List[str] = Field(...) - type: Literal["grid", "horizontal", "vertical", "simple"] = Field(...) - version: Literal[2] = Field(...) + slots: List[str] + type: Literal["grid", "horizontal", "vertical", "simple"] + version: Literal[2] # Optional fields - layout_shape: List[int] = Field(...) + layout_shape: Optional[List[int]] = None class ItemSlot(DefaultDarwin): # GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.ExistingSlot # Required fields - slot_name: str = Field(...) - file_name: str = Field(...) - storage_key: str = Field(...) + slot_name: str + file_name: str + storage_key: str # Optional fields - as_frames: Optional[bool] = Field(default=False) - extract_views: Optional[bool] = Field(default=False) - fps: Optional[ItemFrameRate] = Field(0) - metadata: Optional[Dict[str, UnknownType]] = Field({}) - tags: Optional[Union[List[str], Dict[str, str]]] = Field([]) - type: Literal["image", "video", "pdf", "dicom"] = Field(...) + as_frames: Optional[bool] = None + extract_views: Optional[bool] = None + fps: Optional[Union[int, float, Literal["native"]]] = 0 + metadata: Optional[Dict[str, UnknownType]] = None + tags: Optional[Union[List[str], Dict[str, str]]] = None + type: Optional[Literal["image", "video", "pdf", "dicom"]] = None @validator("slot_name") def validate_slot_name(cls, v: UnknownType) -> str: @@ -74,13 +75,38 @@ def validate_fps(cls, v: UnknownType) -> ItemFrameRate: assert v == "native", "fps must be 'native' or a number greater than 0" return v + class Config: + smart_union = True + + @root_validator + def infer_type(cls, values: Dict[str, UnknownType]) -> Dict[str, UnknownType]: + file_name = values.get("file_name") + + if file_name is not None: + suffix = Path(file_name).suffix.lower() + + # TODO - Review types + if suffix in (".jpg", ".jpeg", ".png", ".bmp", ".gif"): + values["type"] = "image" + elif suffix == ".pdf": + values["type"] = "pdf" + elif suffix in [".dcm", ".nii", ".nii.gz"]: + values["type"] = "dicom" + elif suffix in (".mp4", ".avi", ".mov", ".wmv", ".mkv"): + values["type"] = "video" + + if values["type"] is None: + values["type"] = "image" + + return values + class Item(DefaultDarwin): # GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.NewItem # Required fields name: str - slots: List[ItemSlot] = Field(default=[]) + slots: List[ItemSlot] = [] # Optional fields path: Optional[str] = None diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index f03afe341..e85678392 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -1,6 +1,6 @@ import asyncio from pathlib import Path -from typing import Coroutine, Dict, List +from typing import Coroutine, Dict, List, Tuple from unittest.mock import MagicMock, patch import pytest @@ -8,29 +8,309 @@ import darwin.future.core.items.uploads as uploads from darwin.future.core.client import ClientCore -from darwin.future.data_objects.item import Item +from darwin.future.data_objects.item import Item, ItemSlot from darwin.future.tests.core.fixtures import * # noqa: F401,F403 from .fixtures import * # noqa: F401,F403 class TestBuildSlots: - items: List[Item] = [ - Item( - name="item with no slots", - slots=[], + BUILD_SLOT_RETURN_TYPE = List[Dict] + + items_and_expectations: List[Tuple[Item, BUILD_SLOT_RETURN_TYPE]] = [] + + # Test empty slots + items_and_expectations.append((Item(name="name_with_no_slots", slots=[]), [])) + + # Test Simple slot with no non-required fields + items_and_expectations.append( + ( + Item( + name="name_with_simple_slot", + slots=[ + ItemSlot( + slot_name="slot_name_simple", + file_name="file_name", + storage_key="storage_key", + ) + ], + ), + [ + { + "slot_name": "slot_name_simple", + "file_name": "file_name", + "storage_key": "storage_key", + "type": "image", + "fps": 0, + } + ], + ) + ) + + # Test with multiple slots + items_and_expectations.append( + ( + Item( + name="name_with_multiple_slots", + slots=[ + ItemSlot( + slot_name="slot_name1", + file_name="file_name1", + storage_key="storage_key1", + ), + ItemSlot( + slot_name="slot_name2", + file_name="file_name2", + storage_key="storage_key2", + ), + ], + ), + [ + { + "slot_name": "slot_name1", + "file_name": "file_name1", + "storage_key": "storage_key1", + "type": "image", + "fps": 0, + }, + { + "slot_name": "slot_name2", + "file_name": "file_name2", + "storage_key": "storage_key2", + "type": "image", + "fps": 0, + }, + ], + ) + ) + + # Test with `as_frames` optional field + items_and_expectations.append( + ( + Item( + name="name_testing_as_frames", + slots=[ + ItemSlot( + slot_name="slot_name1", + file_name="file_name", + storage_key="storage_key", + as_frames=True, + ), + ItemSlot( + slot_name="slot_name2", + file_name="file_name", + storage_key="storage_key", + as_frames=False, + ), + ItemSlot( + slot_name="slot_name3", + file_name="file_name", + storage_key="storage_key", + ), + ], + ), + [ + { + "slot_name": "slot_name1", + "file_name": "file_name", + "storage_key": "storage_key", + "fps": 0, + "type": "image", + "as_frames": True, + }, + { + "slot_name": "slot_name2", + "file_name": "file_name", + "storage_key": "storage_key", + "fps": 0, + "type": "image", + "as_frames": False, + }, + { + "slot_name": "slot_name3", + "file_name": "file_name", + "storage_key": "storage_key", + "fps": 0, + "type": "image", + }, + ], ) - ] - expectations: List[List[Dict]] = [[]] + ) - # Sanity check - assert len(items) == len(expectations) + # Test with `extract_views` optional field + items_and_expectations.append( + ( + Item( + name="name_testing_extract_views", + slots=[ + ItemSlot( + slot_name="slot_name1", + file_name="file_name", + storage_key="storage_key", + extract_views=True, + ), + ItemSlot( + slot_name="slot_name2", + file_name="file_name", + storage_key="storage_key", + extract_views=False, + ), + ItemSlot( + slot_name="slot_name3", + file_name="file_name", + storage_key="storage_key", + ), + ], + ), + [ + { + "slot_name": "slot_name1", + "file_name": "file_name", + "storage_key": "storage_key", + "fps": 0, + "type": "image", + "extract_views": True, + }, + { + "slot_name": "slot_name2", + "file_name": "file_name", + "storage_key": "storage_key", + "fps": 0, + "type": "image", + "extract_views": False, + }, + { + "slot_name": "slot_name3", + "file_name": "file_name", + "storage_key": "storage_key", + "fps": 0, + "type": "image", + }, + ], + ) + ) + + # Test with `fps` semi-optional field - field defaults to 0 if not provided + items_and_expectations.append( + ( + Item( + name="name_with_simple_slot", + slots=[ + ItemSlot( + slot_name="slot_name", + file_name="file_name", + storage_key="storage_key", + fps=25, # Testing int + ), + ItemSlot( + slot_name="slot_name", + file_name="file_name", + storage_key="storage_key", + # FIXME: this should pass through as a float but doesn't + fps=float(29.997), # Testing float + ), + ItemSlot( + slot_name="slot_name", + file_name="file_name", + storage_key="storage_key", + fps="native", # Testing literal + ), + ItemSlot( + slot_name="slot_name", + file_name="file_name", + storage_key="storage_key", + ), + ], + ), + [ + { + "slot_name": "slot_name25", + "file_name": "file_name", + "storage_key": "storage_key", + "type": "image", + "fps": 25, + }, + { + "slot_name": "slot_name29.997", + "file_name": "file_name", + "storage_key": "storage_key", + "type": "image", + "fps": 29.997, + }, + { + "slot_name": "slot_namenative", + "file_name": "file_name", + "storage_key": "storage_key", + "type": "image", + "fps": "native", + }, + { + "slot_name": "slot_name", + "file_name": "file_name", + "storage_key": "storage_key", + "type": "image", + "fps": 0, + }, + ], + ) + ) - items_and_expectations = zip(items, expectations) + # Test with `metadata` optional field + items_and_expectations.append( + ( + Item( + name="name_with_simple_slot", + slots=[ + ItemSlot( + slot_name="slot_name", + file_name="file_name", + storage_key="storage_key", + metadata={"key": "value"}, + ), + ItemSlot( + slot_name="slot_name", + file_name="file_name", + storage_key="storage_key", + metadata=None, + ), + ItemSlot( + slot_name="slot_name", + file_name="file_name", + storage_key="storage_key", + ), + ], + ), + [ + { + "slot_name": "slot_name", + "file_name": "file_name", + "storage_key": "storage_key", + "fps": 0, + "type": "image", + "metadata": {"key": "value"}, + }, + { + "slot_name": "slot_name", + "file_name": "file_name", + "storage_key": "storage_key", + "fps": 0, + "type": "image", + }, + { + "slot_name": "slot_name", + "file_name": "file_name", + "storage_key": "storage_key", + "fps": 0, + "type": "image", + }, + ], + ) + ) - @pytest.mark.parametrize("item,expected", *items_and_expectations) + @pytest.mark.parametrize("item,expected", [(item, expected) for item, expected in items_and_expectations]) def test_build_slots(self, item: Item, expected: List[Dict]) -> None: - assert uploads._build_slots(item) == expected + result = asyncio.run(uploads._build_slots(item)) + assert result == expected class TestRegisterUpload: @@ -40,7 +320,7 @@ def default_url(self, base_client: ClientCore) -> str: @responses.activate() @patch.object(uploads, "_build_payload_items") - def test_async_register_uploads_accepts_tuple_or_list_of_tuples( # type: ignore + def test_async_register_uploads_accepts_tuple_or_list_of_tuples( self, mock_build_payload_items: MagicMock, base_client: ClientCore, @@ -97,14 +377,17 @@ def test_async_register_uploads_accepts_tuple_or_list_of_tuples( # type: ignore print(outputs) - class TestCreateSignedUploadUrl: - ... - class TestRegisterAndCreateSignedUploadUrl: - ... +class TestCreateSignedUploadUrl: + ... + + +class TestRegisterAndCreateSignedUploadUrl: + ... + - class TestConfirmUpload: - ... +class TestConfirmUpload: + ... if __name__ == "__main__": From 4506fd9ad7fc62178d2ea52a68eff59ac0c5cda5 Mon Sep 17 00:00:00 2001 From: Owen Date: Thu, 19 Oct 2023 10:58:20 +0100 Subject: [PATCH 09/25] Build items fully working --- darwin/future/core/items/uploads.py | 8 ++------ darwin/future/data_objects/item.py | 2 -- darwin/future/tests/core/items/test_upload_items.py | 9 ++++----- 3 files changed, 6 insertions(+), 13 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index ed0ee0954..67ee68dd5 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -35,6 +35,8 @@ async def _build_slots(item: Item) -> List[Dict]: "slot_name": slot.slot_name, "file_name": slot.file_name, "storage_key": slot.storage_key, + "fps": slot.fps, + "type": slot.type, } if slot.as_frames is not None: @@ -43,18 +45,12 @@ async def _build_slots(item: Item) -> List[Dict]: if slot.extract_views is not None: slot_dict["extract_views"] = slot.extract_views - if slot.fps != 0: - slot_dict["fps"] = slot.fps - if slot.metadata is not None: slot_dict["metadata"] = slot.metadata if slot.tags is not None: slot_dict["tags"] = slot.tags - if slot.type is not None: - slot_dict["type"] = slot.type - slots_to_return.append(slot_dict) return slots_to_return diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index ab56a7a57..5559d1984 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -69,8 +69,6 @@ def validate_storage_key(cls, v: UnknownType) -> str: @validator("fps") def validate_fps(cls, v: UnknownType) -> ItemFrameRate: assert isinstance(v, (int, float, str)), "fps must be a number or 'native'" - if isinstance(v, (int, float)): - assert v > 0, "fps must be greater than 0" if isinstance(v, str): assert v == "native", "fps must be 'native' or a number greater than 0" return v diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index e85678392..f9a17ce1a 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -197,20 +197,19 @@ class TestBuildSlots: name="name_with_simple_slot", slots=[ ItemSlot( - slot_name="slot_name", + slot_name="slot_name25", file_name="file_name", storage_key="storage_key", fps=25, # Testing int ), ItemSlot( - slot_name="slot_name", + slot_name="slot_name29.997", file_name="file_name", storage_key="storage_key", - # FIXME: this should pass through as a float but doesn't - fps=float(29.997), # Testing float + fps=29.997, # Testing float ), ItemSlot( - slot_name="slot_name", + slot_name="slot_namenative", file_name="file_name", storage_key="storage_key", fps="native", # Testing literal From 7c011c109e0c040225efc2ebcf9e9d07a74f4cf0 Mon Sep 17 00:00:00 2001 From: Owen Date: Thu, 19 Oct 2023 12:32:58 +0100 Subject: [PATCH 10/25] Tests WIP --- darwin/future/core/items/uploads.py | 2 +- .../tests/core/items/test_upload_items.py | 225 +++++++++++++++++- 2 files changed, 219 insertions(+), 8 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index 67ee68dd5..cdb8041f7 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -91,7 +91,7 @@ async def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List List[Dict] The payload for the items to be registered for upload """ - # TODO: test me + return_list = [] for item, path in items_and_paths: base_item = { diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index f9a17ce1a..94a12c718 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -1,14 +1,15 @@ import asyncio from pathlib import Path from typing import Coroutine, Dict, List, Tuple -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, Mock, patch import pytest import responses import darwin.future.core.items.uploads as uploads from darwin.future.core.client import ClientCore -from darwin.future.data_objects.item import Item, ItemSlot +from darwin.future.data_objects.item import Item, ItemLayoutV1, ItemLayoutV2, ItemSlot +from darwin.future.exceptions import DarwinException from darwin.future.tests.core.fixtures import * # noqa: F401,F403 from .fixtures import * # noqa: F401,F403 @@ -306,17 +307,158 @@ class TestBuildSlots: ) ) + # Test with `tags` optional field + items_and_expectations.append( + ( + Item( + name="name_testing_tags", + slots=[ + ItemSlot( + slot_name="slot_name_with_string_list", + file_name="file_name", + storage_key="storage_key", + tags=["tag1", "tag2"], + ), + ItemSlot( + slot_name="slot_name_with_kv_pairs", + file_name="file_name", + storage_key="storage_key", + tags={"key": "value"}, + ), + ], + ), + [ + { + "slot_name": "slot_name_with_string_list", + "file_name": "file_name", + "storage_key": "storage_key", + "tags": ["tag1", "tag2"], + "fps": 0, + "type": "image", + }, + { + "slot_name": "slot_name_with_kv_pairs", + "file_name": "file_name", + "storage_key": "storage_key", + "tags": {"key": "value"}, + "fps": 0, + "type": "image", + }, + ], + ) + ) + @pytest.mark.parametrize("item,expected", [(item, expected) for item, expected in items_and_expectations]) def test_build_slots(self, item: Item, expected: List[Dict]) -> None: result = asyncio.run(uploads._build_slots(item)) assert result == expected -class TestRegisterUpload: +class TestBuildLayout: + @pytest.mark.parametrize( + "item, expected", + [ + ( + Item( + name="test_item", + layout=ItemLayoutV1(version=1, type="grid", slots=["slot1", "slot2"]), + ), + { + "slots": ["slot1", "slot2"], + "type": "grid", + "version": 1, + }, + ), + ( + Item( + name="test_item", + layout=ItemLayoutV2( + version=2, + type="grid", + slots=["slot1", "slot2"], + layout_shape=[3, 4], + ), + ), + { + "slots": ["slot1", "slot2"], + "type": "grid", + "version": 2, + "layout_shape": [3, 4], + }, + ), + ], + ) + def test_build_layout(self, item: Item, expected: Dict) -> None: + assert asyncio.run(uploads._build_layout(item)) == expected + + +class TestBuildPayloadItems: + @pytest.mark.parametrize( + "items_and_paths, expected", + [ + ( + [ + ( + Item( + name="test_item", + slots=[ + ItemSlot( + slot_name="slot_name_with_string_list", + file_name="file_name", + storage_key="storage_key", + tags=["tag1", "tag2"], + ), + ItemSlot( + slot_name="slot_name_with_kv_pairs", + file_name="file_name", + storage_key="storage_key", + tags={"key": "value"}, + ), + ], + ), + Path("test_path"), + ) + ], + [ + { + "name": "test_item", + "path:": "test_path", + "tags": [], + "slots": [ + { + "slot_name": "slot_name_with_string_list", + "file_name": "file_name", + "storage_key": "storage_key", + "tags": ["tag1", "tag2"], + "fps": 0, + "type": "image", + }, + { + "slot_name": "slot_name_with_kv_pairs", + "file_name": "file_name", + "storage_key": "storage_key", + "tags": {"key": "value"}, + "fps": 0, + "type": "image", + }, + ], + } + ], + ) + ], + ) + def test_build_payload_items(self, items_and_paths: List[Tuple[Item, Path]], expected: List[Dict]) -> None: + result = asyncio.run(uploads._build_payload_items(items_and_paths)) + assert result == expected + + +class SetupTests: @pytest.fixture def default_url(self, base_client: ClientCore) -> str: - return f"{base_client.config.base_url}api/v2/teams/{base_client.config.default_team}/items" + return f"{base_client.config.base_url}api/v2/teams/my-team/items" + +class TestRegisterUpload(SetupTests): @responses.activate() @patch.object(uploads, "_build_payload_items") def test_async_register_uploads_accepts_tuple_or_list_of_tuples( @@ -377,12 +519,81 @@ def test_async_register_uploads_accepts_tuple_or_list_of_tuples( print(outputs) -class TestCreateSignedUploadUrl: - ... +class TestCreateSignedUploadUrl(SetupTests): + def test_async_create_signed_upload_url(self, default_url: str, base_config: DarwinConfig) -> None: + with responses.RequestsMock() as rsps: + # Mock the API response + expected_response = {"upload_url": "https://signed.url"} + rsps.add( + rsps.POST, + f"{default_url}/uploads/1/sign", + json=expected_response, + ) + + # Call the function with mocked arguments + api_client = ClientCore(base_config) + actual_response = asyncio.run(uploads.async_create_signed_upload_url(api_client, "1", "my-team")) + + # Check that the response matches the expected response + if not actual_response: + pytest.fail("Response was None") + + assert actual_response == expected_response class TestRegisterAndCreateSignedUploadUrl: - ... + @pytest.fixture + def mock_async_register_upload(self): + with patch.object(uploads, "async_register_upload") as mock: + yield mock + + @pytest.fixture + def mock_async_create_signed_upload_url(self): + with patch.object(uploads, "async_create_signed_upload_url") as mock: + yield mock + + def test_async_register_and_create_signed_upload_url( + mock_async_register_upload: MagicMock, + mock_async_create_signed_upload_url: MagicMock, + ): + # Set up mock responses + mock_register_response = {"id": "123"} # TODO Check me + mock_signed_url_response = {"upload_url": "https://signed.url"} + + # Set up mock API client + mock_api_client = MagicMock() + + # Call the function with mocked arguments + actual_response = asyncio.run( + uploads.async_register_and_create_signed_upload_url( + mock_api_client, + "my-team", + "my-dataset", + [(Mock(), Mock())], + force_tiling=False, + handle_as_slices=False, + ignore_dicom_layout=False, + ) + ) + + # Check that the function called the correct sub-functions with the correct arguments + mock_async_register_upload.assert_called_once_with( + mock_api_client, + "my-team", + "my-dataset", + [(Mock(), Mock())], + force_tiling=False, + handle_as_slices=False, + ignore_dicom_layout=False, + ) + mock_async_create_signed_upload_url.assert_called_once_with( + mock_api_client, + "my-team", + "123", + ) + + # Check that the response matches the expected response + assert actual_response == mock_signed_url_response class TestConfirmUpload: From 02f7ca4e2b20e839e325e3e43366494489684a74 Mon Sep 17 00:00:00 2001 From: Owen Date: Thu, 19 Oct 2023 14:59:08 +0100 Subject: [PATCH 11/25] WIP all but register endpoint tests --- darwin/future/core/items/uploads.py | 34 ++--- .../tests/core/items/test_upload_items.py | 143 +++++++++++++++--- 2 files changed, 141 insertions(+), 36 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index cdb8041f7..abd18a351 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -224,20 +224,20 @@ async def async_register_and_create_signed_upload_url( JSONType The response from the API """ - # TODO: test me - register = ( - await async_register_upload( - api_client, - team_slug, - dataset_slug, - items_and_paths, - force_tiling, - handle_as_slices, - ignore_dicom_layout, - ), + + register = await async_register_upload( + api_client, + team_slug, + dataset_slug, + items_and_paths, + force_tiling, + handle_as_slices, + ignore_dicom_layout, ) - download_id = getattr(register, "id") + assert isinstance(register, dict), "Unexpected return type from register upload" + + download_id = register["id"] if "errors" in register or not download_id: raise DarwinException(f"Failed to register upload in {__name__}") @@ -264,7 +264,7 @@ async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id JSONType The response from the API """ - return api_client.post(f"/api/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={}) + return api_client.post(f"/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={}) def register_upload( @@ -296,7 +296,7 @@ def register_upload( ignore_dicom_layout: bool Whether to ignore the dicom layout """ - # TODO: test me + response = asyncio.run( async_register_upload( api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout @@ -327,7 +327,7 @@ def create_signed_upload_url( JSONType The response from the API """ - # TODO: test me + return asyncio.run(async_create_signed_upload_url(api_client, upload_id, team_slug)) @@ -357,7 +357,7 @@ def register_and_create_signed_upload_url( JSONType The response from the API """ - # TODO: test me + return asyncio.run( async_register_and_create_signed_upload_url( api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout @@ -383,6 +383,6 @@ def confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JS JSONType The response from the API """ - # TODO: test me + response = asyncio.run(async_confirm_upload(api_client, team_slug, upload_id)) return response diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 94a12c718..e5977f434 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -1,6 +1,6 @@ import asyncio from pathlib import Path -from typing import Coroutine, Dict, List, Tuple +from typing import Coroutine, Dict, Generator, List, Tuple from unittest.mock import MagicMock, Mock, patch import pytest @@ -543,22 +543,24 @@ def test_async_create_signed_upload_url(self, default_url: str, base_config: Dar class TestRegisterAndCreateSignedUploadUrl: @pytest.fixture - def mock_async_register_upload(self): + def mock_async_register_upload(self) -> Generator: with patch.object(uploads, "async_register_upload") as mock: yield mock @pytest.fixture - def mock_async_create_signed_upload_url(self): + def mock_async_create_signed_upload_url(self) -> Generator: with patch.object(uploads, "async_create_signed_upload_url") as mock: yield mock def test_async_register_and_create_signed_upload_url( + self, mock_async_register_upload: MagicMock, mock_async_create_signed_upload_url: MagicMock, - ): + ) -> None: # Set up mock responses - mock_register_response = {"id": "123"} # TODO Check me + mock_async_register_upload.return_value = {"id": "123"} mock_signed_url_response = {"upload_url": "https://signed.url"} + mock_async_create_signed_upload_url.return_value = mock_signed_url_response # Set up mock API client mock_api_client = MagicMock() @@ -570,22 +572,14 @@ def test_async_register_and_create_signed_upload_url( "my-team", "my-dataset", [(Mock(), Mock())], - force_tiling=False, - handle_as_slices=False, - ignore_dicom_layout=False, + False, + False, + False, ) ) # Check that the function called the correct sub-functions with the correct arguments - mock_async_register_upload.assert_called_once_with( - mock_api_client, - "my-team", - "my-dataset", - [(Mock(), Mock())], - force_tiling=False, - handle_as_slices=False, - ignore_dicom_layout=False, - ) + mock_async_register_upload.assert_called_once() mock_async_create_signed_upload_url.assert_called_once_with( mock_api_client, "my-team", @@ -595,9 +589,120 @@ def test_async_register_and_create_signed_upload_url( # Check that the response matches the expected response assert actual_response == mock_signed_url_response + def test_async_register_and_create_signed_upload_url_raises( + self, + mock_async_register_upload: MagicMock, + mock_async_create_signed_upload_url: MagicMock, + ) -> None: + # Set up mock responses + mock_async_register_upload.return_value = {"id": "123", "errors": ["error"]} + mock_signed_url_response = {"upload_url": "https://signed.url"} + mock_async_create_signed_upload_url.return_value = mock_signed_url_response + + # Set up mock API client + mock_api_client = MagicMock() + + # Check that the response matches the expected response + with pytest.raises(DarwinException): + asyncio.run( + uploads.async_register_and_create_signed_upload_url( + mock_api_client, + "my-team", + "my-dataset", + [(Mock(), Mock())], + False, + False, + False, + ) + ) + + +class TestConfirmUpload(SetupTests): + @responses.activate + def test_async_confirm_upload(self, base_client: ClientCore, default_url: str) -> None: + # Call the function with mocked arguments + responses.add( + "POST", + f"{default_url}/uploads/123/confirm", + status=200, + json={}, + ) + + actual_response = asyncio.run( + uploads.async_confirm_upload( + base_client, + "my-team", + "123", + ) + ) + + # Check that the response matches the expected response + assert actual_response == {} + + def test_async_confirm_upload_raises(self, base_client: ClientCore) -> None: + base_client.post = MagicMock() # type: ignore + base_client.post.side_effect = DarwinException("Error") + + with pytest.raises(DarwinException): + asyncio.run(uploads.async_confirm_upload(base_client, "team", "123")) + + +class TestSynchronousMethods: + @pytest.fixture + def mock_async_register_upload(self) -> Generator: + with patch.object(uploads, "async_register_upload") as mock: + yield mock + + @pytest.fixture + def mock_async_create_signed_upload_url(self) -> Generator: + with patch.object(uploads, "async_create_signed_upload_url") as mock: + yield mock + + @pytest.fixture + def mock_async_register_and_create_signed_upload_url(self) -> Generator: + with patch.object(uploads, "async_register_and_create_signed_upload_url") as mock: + yield mock + + @pytest.fixture + def mock_async_confirm_upload(self) -> Generator: + with patch.object(uploads, "async_confirm_upload") as mock: + yield mock + + def test_register_upload( + self, + mock_async_register_upload: MagicMock, + base_client: ClientCore, + ) -> None: + uploads.register_upload(base_client, "team", "dataset", [(Mock(), Mock())]) + + mock_async_register_upload.assert_called_once() + + def test_create_signed_upload_url( + self, + mock_async_create_signed_upload_url: MagicMock, + base_client: ClientCore, + ) -> None: + uploads.create_signed_upload_url(base_client, "team", "123") + + mock_async_create_signed_upload_url.assert_called_once() + + def test_register_and_create_signed_upload_url( + self, + mock_async_register_and_create_signed_upload_url: MagicMock, + base_client: ClientCore, + ) -> None: + uploads.register_and_create_signed_upload_url(base_client, "team", "dataset", [(Mock(), Mock())]) + + mock_async_register_and_create_signed_upload_url.assert_called_once() + + def test_confirm_upload( + self, + mock_async_confirm_upload: MagicMock, + base_client: ClientCore, + ) -> None: + uploads.confirm_upload(base_client, "team", "123") -class TestConfirmUpload: - ... + mock_async_confirm_upload.assert_called_once() if __name__ == "__main__": From cbe813d8c0989c9f9d9a4465e26b17083d7ef2e0 Mon Sep 17 00:00:00 2001 From: Owen Date: Thu, 19 Oct 2023 16:51:28 +0100 Subject: [PATCH 12/25] Unit tests complete for 3-step upload --- darwin/future/core/items/uploads.py | 5 +- .../tests/core/items/test_upload_items.py | 165 +++++++++++++----- 2 files changed, 120 insertions(+), 50 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index abd18a351..ab4a32158 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -161,7 +161,7 @@ async def async_register_upload( "options": options, } - return api_client.post(f"/api/v2/teams/{team_slug}/items/register_upload", payload) + return api_client.post(f"/v2/teams/{team_slug}/items/register_upload", payload) async def async_create_signed_upload_url( @@ -186,8 +186,7 @@ async def async_create_signed_upload_url( JSONType The response from the API """ - # TODO: Test me - return api_client.post(f"/api/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={}) + return api_client.post(f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={}) async def async_register_and_create_signed_upload_url( diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index e5977f434..5400924bc 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -1,6 +1,6 @@ import asyncio from pathlib import Path -from typing import Coroutine, Dict, Generator, List, Tuple +from typing import Dict, Generator, List, Tuple from unittest.mock import MagicMock, Mock, patch import pytest @@ -459,64 +459,128 @@ def default_url(self, base_client: ClientCore) -> str: class TestRegisterUpload(SetupTests): - @responses.activate() + team_slug = "my-team" + dataset_slug = "my-dataset" + + @pytest.fixture + def items_and_paths(self) -> List[Tuple[Item, Path]]: + return [ + ( + Item( + name="test_item", + slots=[ + ItemSlot( + slot_name="slot_name_with_string_list", + file_name="file_name", + storage_key="storage_key", + tags=["tag1", "tag2"], + ), + ItemSlot( + slot_name="slot_name_with_kv_pairs", + file_name="file_name", + storage_key="storage_key", + tags={"key": "value"}, + ), + ], + ), + Path("test_path"), + ) + ] + + @responses.activate @patch.object(uploads, "_build_payload_items") - def test_async_register_uploads_accepts_tuple_or_list_of_tuples( + def test_async_register_upload_happy_path( self, mock_build_payload_items: MagicMock, base_client: ClientCore, default_url: str, + items_and_paths: List[Tuple[Item, Path]], ) -> None: - mock_build_payload_items.return_value = [] + items = [ + { + "name": "test_item", + "path:": "test_path", + "tags": [], + "slots": [ + { + "slot_name": "slot_name_with_string_list", + "file_name": "file_name", + "storage_key": "storage_key", + "tags": ["tag1", "tag2"], + "fps": 0, + "type": "image", + }, + { + "slot_name": "slot_name_with_kv_pairs", + "file_name": "file_name", + "storage_key": "storage_key", + "tags": {"key": "value"}, + "fps": 0, + "type": "image", + }, + ], + } + ] + mock_build_payload_items.return_value = items - item = Item(name="name", path="path", slots=[]) + responses.add(responses.POST, f"{default_url}/register_upload", json={"status": "success"}) + asyncio.run( + uploads.async_register_upload( + api_client=base_client, + team_slug=self.team_slug, + dataset_slug=self.dataset_slug, + items_and_paths=items_and_paths, + force_tiling=True, + handle_as_slices=True, + ignore_dicom_layout=True, + ) + ) - item_and_path = (item, Path("path")) - items_and_paths = [item_and_path] + assert responses.calls[0].request.url == f"{default_url}/register_upload" # type: ignore + received_call = json.loads(responses.calls[0].request.body.decode("utf-8")) # type: ignore - responses.add( - "post", - f"{default_url}/register_upload", - status=200, - json={ - "dataset_slug": "dataset_slug", - "items": [], - "options": {"force_tiling": False, "handle_as_slices": False, "ignore_dicom_layout": False}, - }, - ) + assert received_call == { + "dataset_slug": "my-dataset", + "items": items, + "options": {"force_tiling": True, "handle_as_slices": True, "ignore_dicom_layout": True}, + }, "The request body should be empty" - responses.add( - "post", - f"{default_url}/register_upload", - status=200, - json={ - "dataset_slug": "dataset_slug", - "items": [], - "options": {"force_tiling": False, "handle_as_slices": False, "ignore_dicom_layout": False}, - }, - ) + @patch.object(uploads, "_build_payload_items") + def test_async_register_upload_raises(self, mock_build_payload_items: MagicMock, base_client: ClientCore) -> None: + with pytest.raises(DarwinException) as exc: + mock_build_payload_items.side_effect = DarwinException("Error1") - tasks: List[Coroutine] = [ - uploads.async_register_upload( - base_client, - "team_slug", - "dataset_slug", - items_and_paths, - ), - uploads.async_register_upload( - base_client, - "team_slug", - "dataset_slug", - item_and_path, - ), - ] - try: - outputs = asyncio.run(tasks[0]), asyncio.run(tasks[1]) - except Exception as e: - print(e) - pytest.fail() + asyncio.run( + uploads.async_register_upload( + api_client=base_client, + team_slug=self.team_slug, + dataset_slug=self.dataset_slug, + items_and_paths=[], + force_tiling=True, + handle_as_slices=True, + ignore_dicom_layout=True, + ) + ) + + assert str(exc) == "Error1", "Failed to raise on failed payload build" + + with pytest.raises(DarwinException) as exc: + base_client.post = MagicMock() # type: ignore + base_client.post.side_effect = DarwinException("Error2") - print(outputs) + asyncio.run( + uploads.async_register_upload( + api_client=base_client, + team_slug=self.team_slug, + dataset_slug=self.dataset_slug, + items_and_paths=[], + force_tiling=True, + handle_as_slices=True, + ignore_dicom_layout=True, + ) + ) + + assert str(exc) == "Error2", "Failed to raise on failed API call" class TestCreateSignedUploadUrl(SetupTests): @@ -540,6 +604,13 @@ def test_async_create_signed_upload_url(self, default_url: str, base_config: Dar assert actual_response == expected_response + def test_async_create_signed_upload_url_raises(self, base_client: ClientCore) -> None: + base_client.post = MagicMock() + base_client.post.side_effect = DarwinException("Error") + + with pytest.raises(DarwinException): + asyncio.run(uploads.async_create_signed_upload_url(base_client, "1", "my-team")) + class TestRegisterAndCreateSignedUploadUrl: @pytest.fixture From ae937ea85170ac318f9dd7c78f773ecdda26f1b8 Mon Sep 17 00:00:00 2001 From: Owen Date: Thu, 19 Oct 2023 16:56:16 +0100 Subject: [PATCH 13/25] Linting --- darwin/future/core/items/uploads.py | 14 +++----------- .../future/tests/core/items/test_upload_items.py | 5 +++-- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index ab4a32158..7d6e8f128 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -143,9 +143,7 @@ async def async_register_upload( if isinstance(items_and_paths, tuple): items_and_paths = [items_and_paths] - assert all( - (isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths - ), "items must be a list of Items" + assert all((isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths), "items must be a list of Items" payload_items = await _build_payload_items(items_and_paths) @@ -296,11 +294,7 @@ def register_upload( Whether to ignore the dicom layout """ - response = asyncio.run( - async_register_upload( - api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout - ) - ) + response = asyncio.run(async_register_upload(api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout)) return response @@ -358,9 +352,7 @@ def register_and_create_signed_upload_url( """ return asyncio.run( - async_register_and_create_signed_upload_url( - api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout - ) + async_register_and_create_signed_upload_url(api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout) ) diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 5400924bc..7fab0513e 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -3,11 +3,12 @@ from typing import Dict, Generator, List, Tuple from unittest.mock import MagicMock, Mock, patch +import orjson as json import pytest import responses import darwin.future.core.items.uploads as uploads -from darwin.future.core.client import ClientCore +from darwin.future.core.client import ClientCore, DarwinConfig from darwin.future.data_objects.item import Item, ItemLayoutV1, ItemLayoutV2, ItemSlot from darwin.future.exceptions import DarwinException from darwin.future.tests.core.fixtures import * # noqa: F401,F403 @@ -605,7 +606,7 @@ def test_async_create_signed_upload_url(self, default_url: str, base_config: Dar assert actual_response == expected_response def test_async_create_signed_upload_url_raises(self, base_client: ClientCore) -> None: - base_client.post = MagicMock() + base_client.post = MagicMock() # type: ignore base_client.post.side_effect = DarwinException("Error") with pytest.raises(DarwinException): From a5bf77028e6a34a8c5fd4ed952dce267fcf8b9f6 Mon Sep 17 00:00:00 2001 From: Owen Date: Thu, 19 Oct 2023 18:04:30 +0100 Subject: [PATCH 14/25] Tests for relevant parts of data items --- darwin/future/data_objects/item.py | 19 +++-- .../tests/core/items/test_item_data_object.py | 84 +++++++++++++++++++ 2 files changed, 94 insertions(+), 9 deletions(-) create mode 100644 darwin/future/tests/core/items/test_item_data_object.py diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index 5559d1984..a353acf82 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -3,6 +3,7 @@ from typing import Dict, List, Literal, Optional, Union from pydantic import root_validator, validator +from torch import isin from darwin.datatypes import NumberLike from darwin.future.data_objects.pydantic_base import DefaultDarwin @@ -14,7 +15,7 @@ def validate_no_slashes(v: UnknownType) -> str: assert isinstance(v, str), "Must be a string" assert len(v) > 0, "cannot be empty" - assert r"^[^/].*$".find(v) == -1, "cannot start with a slash" + assert not v.startswith("/"), "cannot start with a slash" return v @@ -71,6 +72,8 @@ def validate_fps(cls, v: UnknownType) -> ItemFrameRate: assert isinstance(v, (int, float, str)), "fps must be a number or 'native'" if isinstance(v, str): assert v == "native", "fps must be 'native' or a number greater than 0" + elif isinstance(v, (int, float)): + assert v >= 0, "fps must be 'native' or a number greater than or equal to 0" return v class Config: @@ -81,20 +84,18 @@ def infer_type(cls, values: Dict[str, UnknownType]) -> Dict[str, UnknownType]: file_name = values.get("file_name") if file_name is not None: - suffix = Path(file_name).suffix.lower() - # TODO - Review types - if suffix in (".jpg", ".jpeg", ".png", ".bmp", ".gif"): + if file_name.endswith((".jpg", ".jpeg", ".png", ".bmp", ".gif")): values["type"] = "image" - elif suffix == ".pdf": + elif file_name.endswith(".pdf"): values["type"] = "pdf" - elif suffix in [".dcm", ".nii", ".nii.gz"]: + elif file_name.endswith((".dcm", ".nii", ".nii.gz")): values["type"] = "dicom" - elif suffix in (".mp4", ".avi", ".mov", ".wmv", ".mkv"): + elif file_name.endswith((".mp4", ".avi", ".mov", ".wmv", ".mkv")): values["type"] = "video" - if values["type"] is None: - values["type"] = "image" + if "type" not in values or values["type"] is None: + values["type"] = "image" return values diff --git a/darwin/future/tests/core/items/test_item_data_object.py b/darwin/future/tests/core/items/test_item_data_object.py new file mode 100644 index 000000000..69106469c --- /dev/null +++ b/darwin/future/tests/core/items/test_item_data_object.py @@ -0,0 +1,84 @@ +from typing import List, Literal, Tuple + +import pytest + +from darwin.future.data_objects.item import ItemSlot, validate_no_slashes +from darwin.future.data_objects.typing import UnknownType + + +def generate_extension_expectations( + extension: str, expectation: Literal["image", "video", "pdf", "dicom"] +) -> List[Tuple[str, Literal["image", "video", "pdf", "dicom"]]]: + """ + Generate a list of tuples of the form (file_name, expectation) where + """ + return [ + (f"file.{extension}", expectation), + (f"file.with.dots.{extension}", expectation), + (f"/file/with/slashes.{extension}", expectation), + ] + + +expectations_list = [ + # Supported images + *generate_extension_expectations("jpg", "image"), + *generate_extension_expectations("jpeg", "image"), + *generate_extension_expectations("png", "image"), + *generate_extension_expectations("gif", "image"), + *generate_extension_expectations("bmp", "image"), + # Supported documents + *generate_extension_expectations("pdf", "pdf"), + # Supported medical imaging + *generate_extension_expectations("dcm", "dicom"), + *generate_extension_expectations("nii", "dicom"), + *generate_extension_expectations("nii.gz", "dicom"), + # Supported videos + *generate_extension_expectations("mp4", "video"), + *generate_extension_expectations("avi", "video"), + *generate_extension_expectations("mov", "video"), + *generate_extension_expectations("wmv", "video"), + *generate_extension_expectations("mkv", "video"), + # Unsupported + *generate_extension_expectations("unsupported", "image"), +] + + +class TestValidateNoSlashes: + @pytest.mark.parametrize("string", [("validname"), ("valid/name"), ("valid/name/still")]) + def test_happy_paths(self, string: str) -> None: + assert validate_no_slashes(string) == string + + @pytest.mark.parametrize("string", [(""), (123), ("/invalid_string")]) + def test_sad_paths(self, string: UnknownType) -> None: + with pytest.raises(AssertionError): + validate_no_slashes(string) + + +class TestSlotNameValidator: + @pytest.mark.parametrize("string", [("validname"), ("valid/name"), ("valid/name/still")]) + def test_happy_paths(self, string: str) -> None: + assert ItemSlot.validate_slot_name(string) == string + + @pytest.mark.parametrize("string", [(""), (123)]) + def test_sad_paths(self, string: UnknownType) -> None: + with pytest.raises(AssertionError): + ItemSlot.validate_slot_name(string) + + +class TestFpsValidator: + @pytest.mark.parametrize("fps", [(0), (1), (1.0), ("native")]) + def test_happy_paths(self, fps: UnknownType) -> None: + assert ItemSlot.validate_fps(fps) == fps + + @pytest.mark.parametrize("fps", [(-1), ("invalid")]) + def test_sad_paths(self, fps: UnknownType) -> None: + with pytest.raises(AssertionError): + ItemSlot.validate_fps(fps) + + +class TestRootValidator: + @pytest.mark.parametrize("file_name, expectation", expectations_list) + def test_happy_paths(self, file_name: str, expectation: str) -> None: + assert ( + ItemSlot.infer_type({"file_name": file_name})["type"] == expectation + ), f"Failed for {file_name}, got {expectation}" From 28b197643466d37be23ef8ddb485c5d96bed1c49 Mon Sep 17 00:00:00 2001 From: Owen Date: Thu, 19 Oct 2023 18:14:04 +0100 Subject: [PATCH 15/25] Formatting --- darwin/future/core/items/uploads.py | 43 ++++++++++++--- .../tests/core/items/test_item_data_object.py | 8 ++- .../tests/core/items/test_upload_items.py | 54 ++++++++++++++----- deploy/_filter_files.py | 6 ++- 4 files changed, 88 insertions(+), 23 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index 7d6e8f128..2816da04c 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -143,7 +143,10 @@ async def async_register_upload( if isinstance(items_and_paths, tuple): items_and_paths = [items_and_paths] - assert all((isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths), "items must be a list of Items" + assert all( + (isinstance(item, Item) and isinstance(path, Path)) + for item, path in items_and_paths + ), "items must be a list of Items" payload_items = await _build_payload_items(items_and_paths) @@ -184,7 +187,9 @@ async def async_create_signed_upload_url( JSONType The response from the API """ - return api_client.post(f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={}) + return api_client.post( + f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={} + ) async def async_register_and_create_signed_upload_url( @@ -238,12 +243,16 @@ async def async_register_and_create_signed_upload_url( if "errors" in register or not download_id: raise DarwinException(f"Failed to register upload in {__name__}") - signed_info = await async_create_signed_upload_url(api_client, team_slug, download_id) + signed_info = await async_create_signed_upload_url( + api_client, team_slug, download_id + ) return signed_info -async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JSONType: +async def async_confirm_upload( + api_client: ClientCore, team_slug: str, upload_id: str +) -> JSONType: """ Asynchronously confirm an upload/uploads was successful by ID @@ -261,7 +270,9 @@ async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id JSONType The response from the API """ - return api_client.post(f"/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={}) + return api_client.post( + f"/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={} + ) def register_upload( @@ -294,7 +305,17 @@ def register_upload( Whether to ignore the dicom layout """ - response = asyncio.run(async_register_upload(api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout)) + response = asyncio.run( + async_register_upload( + api_client, + team_slug, + dataset_slug, + items_and_paths, + force_tiling, + handle_as_slices, + ignore_dicom_layout, + ) + ) return response @@ -352,7 +373,15 @@ def register_and_create_signed_upload_url( """ return asyncio.run( - async_register_and_create_signed_upload_url(api_client, team_slug, dataset_slug, items_and_paths, force_tiling, handle_as_slices, ignore_dicom_layout) + async_register_and_create_signed_upload_url( + api_client, + team_slug, + dataset_slug, + items_and_paths, + force_tiling, + handle_as_slices, + ignore_dicom_layout, + ) ) diff --git a/darwin/future/tests/core/items/test_item_data_object.py b/darwin/future/tests/core/items/test_item_data_object.py index 69106469c..9e0307c10 100644 --- a/darwin/future/tests/core/items/test_item_data_object.py +++ b/darwin/future/tests/core/items/test_item_data_object.py @@ -44,7 +44,9 @@ def generate_extension_expectations( class TestValidateNoSlashes: - @pytest.mark.parametrize("string", [("validname"), ("valid/name"), ("valid/name/still")]) + @pytest.mark.parametrize( + "string", [("validname"), ("valid/name"), ("valid/name/still")] + ) def test_happy_paths(self, string: str) -> None: assert validate_no_slashes(string) == string @@ -55,7 +57,9 @@ def test_sad_paths(self, string: UnknownType) -> None: class TestSlotNameValidator: - @pytest.mark.parametrize("string", [("validname"), ("valid/name"), ("valid/name/still")]) + @pytest.mark.parametrize( + "string", [("validname"), ("valid/name"), ("valid/name/still")] + ) def test_happy_paths(self, string: str) -> None: assert ItemSlot.validate_slot_name(string) == string diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 7fab0513e..85f8eeaee 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -349,7 +349,9 @@ class TestBuildSlots: ) ) - @pytest.mark.parametrize("item,expected", [(item, expected) for item, expected in items_and_expectations]) + @pytest.mark.parametrize( + "item,expected", [(item, expected) for item, expected in items_and_expectations] + ) def test_build_slots(self, item: Item, expected: List[Dict]) -> None: result = asyncio.run(uploads._build_slots(item)) assert result == expected @@ -362,7 +364,9 @@ class TestBuildLayout: ( Item( name="test_item", - layout=ItemLayoutV1(version=1, type="grid", slots=["slot1", "slot2"]), + layout=ItemLayoutV1( + version=1, type="grid", slots=["slot1", "slot2"] + ), ), { "slots": ["slot1", "slot2"], @@ -448,7 +452,9 @@ class TestBuildPayloadItems: ) ], ) - def test_build_payload_items(self, items_and_paths: List[Tuple[Item, Path]], expected: List[Dict]) -> None: + def test_build_payload_items( + self, items_and_paths: List[Tuple[Item, Path]], expected: List[Dict] + ) -> None: result = asyncio.run(uploads._build_payload_items(items_and_paths)) assert result == expected @@ -524,7 +530,9 @@ def test_async_register_upload_happy_path( ] mock_build_payload_items.return_value = items - responses.add(responses.POST, f"{default_url}/register_upload", json={"status": "success"}) + responses.add( + responses.POST, f"{default_url}/register_upload", json={"status": "success"} + ) asyncio.run( uploads.async_register_upload( api_client=base_client, @@ -543,11 +551,17 @@ def test_async_register_upload_happy_path( assert received_call == { "dataset_slug": "my-dataset", "items": items, - "options": {"force_tiling": True, "handle_as_slices": True, "ignore_dicom_layout": True}, + "options": { + "force_tiling": True, + "handle_as_slices": True, + "ignore_dicom_layout": True, + }, }, "The request body should be empty" @patch.object(uploads, "_build_payload_items") - def test_async_register_upload_raises(self, mock_build_payload_items: MagicMock, base_client: ClientCore) -> None: + def test_async_register_upload_raises( + self, mock_build_payload_items: MagicMock, base_client: ClientCore + ) -> None: with pytest.raises(DarwinException) as exc: mock_build_payload_items.side_effect = DarwinException("Error1") @@ -585,7 +599,9 @@ def test_async_register_upload_raises(self, mock_build_payload_items: MagicMock, class TestCreateSignedUploadUrl(SetupTests): - def test_async_create_signed_upload_url(self, default_url: str, base_config: DarwinConfig) -> None: + def test_async_create_signed_upload_url( + self, default_url: str, base_config: DarwinConfig + ) -> None: with responses.RequestsMock() as rsps: # Mock the API response expected_response = {"upload_url": "https://signed.url"} @@ -597,7 +613,9 @@ def test_async_create_signed_upload_url(self, default_url: str, base_config: Dar # Call the function with mocked arguments api_client = ClientCore(base_config) - actual_response = asyncio.run(uploads.async_create_signed_upload_url(api_client, "1", "my-team")) + actual_response = asyncio.run( + uploads.async_create_signed_upload_url(api_client, "1", "my-team") + ) # Check that the response matches the expected response if not actual_response: @@ -605,12 +623,16 @@ def test_async_create_signed_upload_url(self, default_url: str, base_config: Dar assert actual_response == expected_response - def test_async_create_signed_upload_url_raises(self, base_client: ClientCore) -> None: + def test_async_create_signed_upload_url_raises( + self, base_client: ClientCore + ) -> None: base_client.post = MagicMock() # type: ignore base_client.post.side_effect = DarwinException("Error") with pytest.raises(DarwinException): - asyncio.run(uploads.async_create_signed_upload_url(base_client, "1", "my-team")) + asyncio.run( + uploads.async_create_signed_upload_url(base_client, "1", "my-team") + ) class TestRegisterAndCreateSignedUploadUrl: @@ -691,7 +713,9 @@ def test_async_register_and_create_signed_upload_url_raises( class TestConfirmUpload(SetupTests): @responses.activate - def test_async_confirm_upload(self, base_client: ClientCore, default_url: str) -> None: + def test_async_confirm_upload( + self, base_client: ClientCore, default_url: str + ) -> None: # Call the function with mocked arguments responses.add( "POST", @@ -732,7 +756,9 @@ def mock_async_create_signed_upload_url(self) -> Generator: @pytest.fixture def mock_async_register_and_create_signed_upload_url(self) -> Generator: - with patch.object(uploads, "async_register_and_create_signed_upload_url") as mock: + with patch.object( + uploads, "async_register_and_create_signed_upload_url" + ) as mock: yield mock @pytest.fixture @@ -763,7 +789,9 @@ def test_register_and_create_signed_upload_url( mock_async_register_and_create_signed_upload_url: MagicMock, base_client: ClientCore, ) -> None: - uploads.register_and_create_signed_upload_url(base_client, "team", "dataset", [(Mock(), Mock())]) + uploads.register_and_create_signed_upload_url( + base_client, "team", "dataset", [(Mock(), Mock())] + ) mock_async_register_and_create_signed_upload_url.assert_called_once() diff --git a/deploy/_filter_files.py b/deploy/_filter_files.py index c1777b0df..ae183fa44 100755 --- a/deploy/_filter_files.py +++ b/deploy/_filter_files.py @@ -11,7 +11,11 @@ def main(argv: List[str]) -> None: if file_extension.startswith("."): file_extension = file_extension[1:] - files_out = [file for file in files_in if file.endswith(f".{file_extension}") and 'future' in file] + files_out = [ + file + for file in files_in + if file.endswith(f".{file_extension}") and "future" in file + ] print(" ".join(files_out)) From 39a27fc60c11eb7965392971f4b4b35ca11f1e70 Mon Sep 17 00:00:00 2001 From: Owen Date: Thu, 19 Oct 2023 18:18:04 +0100 Subject: [PATCH 16/25] Linting --- darwin/future/data_objects/item.py | 2 - .../tests/core/items/test_upload_items.py | 52 +++++-------------- 2 files changed, 14 insertions(+), 40 deletions(-) diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index a353acf82..184a3b9a9 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -1,9 +1,7 @@ # @see: GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.ExistingItem -from pathlib import Path from typing import Dict, List, Literal, Optional, Union from pydantic import root_validator, validator -from torch import isin from darwin.datatypes import NumberLike from darwin.future.data_objects.pydantic_base import DefaultDarwin diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 85f8eeaee..eba59a008 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -349,9 +349,7 @@ class TestBuildSlots: ) ) - @pytest.mark.parametrize( - "item,expected", [(item, expected) for item, expected in items_and_expectations] - ) + @pytest.mark.parametrize("item,expected", [(item, expected) for item, expected in items_and_expectations]) def test_build_slots(self, item: Item, expected: List[Dict]) -> None: result = asyncio.run(uploads._build_slots(item)) assert result == expected @@ -364,9 +362,7 @@ class TestBuildLayout: ( Item( name="test_item", - layout=ItemLayoutV1( - version=1, type="grid", slots=["slot1", "slot2"] - ), + layout=ItemLayoutV1(version=1, type="grid", slots=["slot1", "slot2"]), ), { "slots": ["slot1", "slot2"], @@ -452,9 +448,7 @@ class TestBuildPayloadItems: ) ], ) - def test_build_payload_items( - self, items_and_paths: List[Tuple[Item, Path]], expected: List[Dict] - ) -> None: + def test_build_payload_items(self, items_and_paths: List[Tuple[Item, Path]], expected: List[Dict]) -> None: result = asyncio.run(uploads._build_payload_items(items_and_paths)) assert result == expected @@ -530,9 +524,7 @@ def test_async_register_upload_happy_path( ] mock_build_payload_items.return_value = items - responses.add( - responses.POST, f"{default_url}/register_upload", json={"status": "success"} - ) + responses.add(responses.POST, f"{default_url}/register_upload", json={"status": "success"}) asyncio.run( uploads.async_register_upload( api_client=base_client, @@ -544,10 +536,10 @@ def test_async_register_upload_happy_path( ignore_dicom_layout=True, ) ) - + # noqa: E501 assert responses.calls[0].request.url == f"{default_url}/register_upload" # type: ignore + # noqa: E501 received_call = json.loads(responses.calls[0].request.body.decode("utf-8")) # type: ignore - assert received_call == { "dataset_slug": "my-dataset", "items": items, @@ -559,9 +551,7 @@ def test_async_register_upload_happy_path( }, "The request body should be empty" @patch.object(uploads, "_build_payload_items") - def test_async_register_upload_raises( - self, mock_build_payload_items: MagicMock, base_client: ClientCore - ) -> None: + def test_async_register_upload_raises(self, mock_build_payload_items: MagicMock, base_client: ClientCore) -> None: with pytest.raises(DarwinException) as exc: mock_build_payload_items.side_effect = DarwinException("Error1") @@ -599,9 +589,7 @@ def test_async_register_upload_raises( class TestCreateSignedUploadUrl(SetupTests): - def test_async_create_signed_upload_url( - self, default_url: str, base_config: DarwinConfig - ) -> None: + def test_async_create_signed_upload_url(self, default_url: str, base_config: DarwinConfig) -> None: with responses.RequestsMock() as rsps: # Mock the API response expected_response = {"upload_url": "https://signed.url"} @@ -613,9 +601,7 @@ def test_async_create_signed_upload_url( # Call the function with mocked arguments api_client = ClientCore(base_config) - actual_response = asyncio.run( - uploads.async_create_signed_upload_url(api_client, "1", "my-team") - ) + actual_response = asyncio.run(uploads.async_create_signed_upload_url(api_client, "1", "my-team")) # Check that the response matches the expected response if not actual_response: @@ -623,16 +609,12 @@ def test_async_create_signed_upload_url( assert actual_response == expected_response - def test_async_create_signed_upload_url_raises( - self, base_client: ClientCore - ) -> None: + def test_async_create_signed_upload_url_raises(self, base_client: ClientCore) -> None: base_client.post = MagicMock() # type: ignore base_client.post.side_effect = DarwinException("Error") with pytest.raises(DarwinException): - asyncio.run( - uploads.async_create_signed_upload_url(base_client, "1", "my-team") - ) + asyncio.run(uploads.async_create_signed_upload_url(base_client, "1", "my-team")) class TestRegisterAndCreateSignedUploadUrl: @@ -713,9 +695,7 @@ def test_async_register_and_create_signed_upload_url_raises( class TestConfirmUpload(SetupTests): @responses.activate - def test_async_confirm_upload( - self, base_client: ClientCore, default_url: str - ) -> None: + def test_async_confirm_upload(self, base_client: ClientCore, default_url: str) -> None: # Call the function with mocked arguments responses.add( "POST", @@ -756,9 +736,7 @@ def mock_async_create_signed_upload_url(self) -> Generator: @pytest.fixture def mock_async_register_and_create_signed_upload_url(self) -> Generator: - with patch.object( - uploads, "async_register_and_create_signed_upload_url" - ) as mock: + with patch.object(uploads, "async_register_and_create_signed_upload_url") as mock: yield mock @pytest.fixture @@ -789,9 +767,7 @@ def test_register_and_create_signed_upload_url( mock_async_register_and_create_signed_upload_url: MagicMock, base_client: ClientCore, ) -> None: - uploads.register_and_create_signed_upload_url( - base_client, "team", "dataset", [(Mock(), Mock())] - ) + uploads.register_and_create_signed_upload_url(base_client, "team", "dataset", [(Mock(), Mock())]) mock_async_register_and_create_signed_upload_url.assert_called_once() From 59ac52e15f4dfba654512970f5a0bfe69eff2934 Mon Sep 17 00:00:00 2001 From: Owen Date: Fri, 20 Oct 2023 10:58:43 +0100 Subject: [PATCH 17/25] Disable E501 in pyproject --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 75362bb10..e2f88c43d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -56,7 +56,7 @@ warn_untyped_fields = true [tool.ruff] select = ["E", "F", "C"] -ignore = ["E203", "E402"] +ignore = ["E203", "E402", "E501"] line-length = 88 [tool.ruff.per-file-ignores] From c00a24d4ee92f518ceaa98b6c75dd99adc0aa178 Mon Sep 17 00:00:00 2001 From: Owen Date: Fri, 20 Oct 2023 11:00:47 +0100 Subject: [PATCH 18/25] Fix actual ruff thing --- darwin/future/tests/core/items/test_upload_items.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index eba59a008..8cf2b8a26 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -349,7 +349,7 @@ class TestBuildSlots: ) ) - @pytest.mark.parametrize("item,expected", [(item, expected) for item, expected in items_and_expectations]) + @pytest.mark.parametrize("item,expected", items_and_expectations) def test_build_slots(self, item: Item, expected: List[Dict]) -> None: result = asyncio.run(uploads._build_slots(item)) assert result == expected From 9e8a890a5a50886bde2d8d50689f6e70a966ebf4 Mon Sep 17 00:00:00 2001 From: Owen Date: Fri, 20 Oct 2023 15:00:47 +0100 Subject: [PATCH 19/25] Refactor based on sync --- darwin/future/core/items/uploads.py | 112 ++++++++++++++---- darwin/future/data_objects/item.py | 82 +++++++------ .../tests/core/items/test_item_data_object.py | 20 ++-- .../tests/core/items/test_upload_items.py | 40 ++++++- 4 files changed, 185 insertions(+), 69 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index 2816da04c..42505b7aa 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -1,13 +1,17 @@ import asyncio +from logging import getLogger from pathlib import Path from typing import Dict, List, Tuple, Union +from pydantic import parse_obj_as + from darwin.future.core.client import ClientCore -from darwin.future.core.types.common import JSONType from darwin.future.data_objects.item import Item from darwin.future.data_objects.typing import UnknownType from darwin.future.exceptions import DarwinException +logger = getLogger(__name__) + async def _build_slots(item: Item) -> List[Dict]: """ @@ -23,7 +27,6 @@ async def _build_slots(item: Item) -> List[Dict]: List[Dict] The built slots """ - # ! testme if not item.slots: return [] @@ -119,7 +122,7 @@ async def async_register_upload( force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, -) -> JSONType: +) -> Item: """ Registers an upload for a dataset that can then be used to upload files to Darwin @@ -162,14 +165,24 @@ async def async_register_upload( "options": options, } - return api_client.post(f"/v2/teams/{team_slug}/items/register_upload", payload) + try: + response = api_client.post( + f"/v2/teams/{team_slug}/items/register_upload", payload + ) + except Exception as exc: + logger.error(f"Failed to register upload in {__name__}", exc_info=exc) + raise DarwinException(f"Failed to register upload in {__name__}") from exc + + assert isinstance(response, dict), "Unexpected return type from register upload" + + return parse_obj_as(Item, response) async def async_create_signed_upload_url( api_client: ClientCore, upload_id: str, team_slug: str, -) -> JSONType: +) -> str: """ Asynchronously create a signed upload URL for an upload or uploads @@ -187,9 +200,43 @@ async def async_create_signed_upload_url( JSONType The response from the API """ - return api_client.post( - f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={} - ) + try: + response = api_client.post( + f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={} + ) + except Exception as exc: + logger.error(f"Failed to create signed upload url in {__name__}", exc_info=exc) + raise DarwinException( + f"Failed to create signed upload url in {__name__}" + ) from exc + + assert isinstance( + response, dict + ), "Unexpected return type from create signed upload url" + + if not response: + logger.error( + f"Failed to create signed upload url in {__name__}, got no response" + ) + raise DarwinException( + f"Failed to create signed upload url in {__name__}, got no response" + ) + + if "errors" in response: + logger.error( + f"Failed to create signed upload url in {__name__}, got errors: {response['errors']}" + ) + raise DarwinException(f"Failed to create signed upload url in {__name__}") + + if "upload_url" not in response: + logger.error( + f"Failed to create signed upload url in {__name__}, got no upload_url" + ) + raise DarwinException( + f"Failed to create signed upload url in {__name__}, got no upload_url" + ) + + return response["upload_url"] async def async_register_and_create_signed_upload_url( @@ -200,7 +247,7 @@ async def async_register_and_create_signed_upload_url( force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, -) -> JSONType: +) -> str: """ Asynchronously register and create a signed upload URL for an upload or uploads @@ -223,8 +270,8 @@ async def async_register_and_create_signed_upload_url( Returns ------- - JSONType - The response from the API + str + The signed upload URL """ register = await async_register_upload( @@ -252,7 +299,7 @@ async def async_register_and_create_signed_upload_url( async def async_confirm_upload( api_client: ClientCore, team_slug: str, upload_id: str -) -> JSONType: +) -> None: """ Asynchronously confirm an upload/uploads was successful by ID @@ -270,9 +317,28 @@ async def async_confirm_upload( JSONType The response from the API """ - return api_client.post( - f"/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={} - ) + + try: + response = api_client.post( + f"/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={} + ) + except Exception as exc: + logger.error(f"Failed to confirm upload in {__name__}", exc_info=exc) + raise DarwinException(f"Failed to confirm upload in {__name__}") from exc + + assert isinstance(response, dict), "Unexpected return type from confirm upload" + + if not response: + logger.error(f"Failed to confirm upload in {__name__}, got no response") + raise DarwinException( + f"Failed to confirm upload in {__name__}, got no response" + ) + + if "errors" in response: + logger.error( + f"Failed to confirm upload in {__name__}, got errors: {response['errors']}" + ) + raise DarwinException(f"Failed to confirm upload in {__name__}") def register_upload( @@ -283,7 +349,7 @@ def register_upload( force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, -) -> JSONType: +) -> Item: """ Asynchronously register an upload/uploads for a dataset that can then be used to upload files to Darwin @@ -323,7 +389,7 @@ def create_signed_upload_url( api_client: ClientCore, upload_id: str, team_slug: str, -) -> JSONType: +) -> str: """ Create a signed upload URL for an upload or uploads @@ -353,7 +419,7 @@ def register_and_create_signed_upload_url( force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, -) -> JSONType: +) -> str: """ Register and create a signed upload URL for an upload or uploads @@ -385,7 +451,7 @@ def register_and_create_signed_upload_url( ) -def confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JSONType: +def confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> None: """ Confirm an upload/uploads was successful by ID @@ -400,8 +466,12 @@ def confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> JS Returns ------- - JSONType - The response from the API + None + + Raises + ------ + DarwinException + If the upload could not be confirmed """ response = asyncio.run(async_confirm_upload(api_client, team_slug, upload_id)) diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index 184a3b9a9..75a1cf15b 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -13,30 +13,29 @@ def validate_no_slashes(v: UnknownType) -> str: assert isinstance(v, str), "Must be a string" assert len(v) > 0, "cannot be empty" - assert not v.startswith("/"), "cannot start with a slash" + assert "/" not in v, "cannot contain slashes" + assert " " not in v, "cannot contain spaces" return v -class ItemLayoutV1(DefaultDarwin): +class ItemLayout(DefaultDarwin): # GraphotateWeb.Schemas.DatasetsV2.Common.ItemLayoutV1 # Required fields slots: List[str] type: Literal["grid", "horizontal", "vertical", "simple"] - version: Literal[1] + version: Literal[1, 2] + # Required only in version 2 + layout_shape: Optional[List[int]] = None -class ItemLayoutV2(DefaultDarwin): - # GraphotateWeb.Schemas.DatasetsV2.Common.ItemLayoutV2 + @validator("layout_shape", always=True) + def layout_validator(cls, value: UnknownType, values: Dict): + if not value and values.get("version") == 2: + raise ValueError("layout_shape must be specified for version 2 layouts") - # Required fields - slots: List[str] - type: Literal["grid", "horizontal", "vertical", "simple"] - version: Literal[2] - - # Optional fields - layout_shape: Optional[List[int]] = None + return value class ItemSlot(DefaultDarwin): @@ -45,12 +44,12 @@ class ItemSlot(DefaultDarwin): # Required fields slot_name: str file_name: str - storage_key: str # Optional fields + storage_key: Optional[str] = None as_frames: Optional[bool] = None extract_views: Optional[bool] = None - fps: Optional[Union[int, float, Literal["native"]]] = 0 + fps: Optional[Union[int, float, Literal["native"]]] = None metadata: Optional[Dict[str, UnknownType]] = None tags: Optional[Union[List[str], Dict[str, str]]] = None type: Optional[Literal["image", "video", "pdf", "dicom"]] = None @@ -61,23 +60,26 @@ def validate_slot_name(cls, v: UnknownType) -> str: assert len(v) > 0, "slot_name cannot be empty" return v - @validator("file_name") - def validate_storage_key(cls, v: UnknownType) -> str: - return validate_no_slashes(v) - - @validator("fps") - def validate_fps(cls, v: UnknownType) -> ItemFrameRate: - assert isinstance(v, (int, float, str)), "fps must be a number or 'native'" - if isinstance(v, str): - assert v == "native", "fps must be 'native' or a number greater than 0" - elif isinstance(v, (int, float)): - assert v >= 0, "fps must be 'native' or a number greater than or equal to 0" - return v - - class Config: - smart_union = True - - @root_validator + @classmethod + def validate_fps(cls, values): + value = values.get("fps") + + if value is None: + values["fps"] = 0 + return values + + assert isinstance(value, (int, float, str)), "fps must be a number or 'native'" + if isinstance(value, str): + assert value == "native", "fps must be 'native' or a number greater than 0" + elif isinstance(value, (int, float)): + type = values.get("type") + if type == "image": + assert value == 0, "fps must be 0 for images" + else: + assert value >= 0, "fps must be greater than or equal to 0 for videos" + return value + + @classmethod def infer_type(cls, values: Dict[str, UnknownType]) -> Dict[str, UnknownType]: file_name = values.get("file_name") @@ -92,11 +94,16 @@ def infer_type(cls, values: Dict[str, UnknownType]) -> Dict[str, UnknownType]: elif file_name.endswith((".mp4", ".avi", ".mov", ".wmv", ".mkv")): values["type"] = "video" - if "type" not in values or values["type"] is None: - values["type"] = "image" - return values + class Config: + smart_union = True + + @root_validator + def root(cls, v, values): + values = cls.infer_type(values) + values = cls.validate_fps(values) + class Item(DefaultDarwin): # GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.NewItem @@ -104,11 +111,14 @@ class Item(DefaultDarwin): # Required fields name: str slots: List[ItemSlot] = [] + path: str = "/" + dataset_id: int + processing_status: str # Optional fields - path: Optional[str] = None + priority: Optional[int] = None tags: Optional[Union[List[str], Dict[str, str]]] = [] - layout: Optional[Union[ItemLayoutV1, ItemLayoutV2]] = None + layout: Optional[ItemLayout] = None @validator("name") def validate_name(cls, v: UnknownType) -> str: diff --git a/darwin/future/tests/core/items/test_item_data_object.py b/darwin/future/tests/core/items/test_item_data_object.py index 9e0307c10..5bbb47772 100644 --- a/darwin/future/tests/core/items/test_item_data_object.py +++ b/darwin/future/tests/core/items/test_item_data_object.py @@ -1,4 +1,4 @@ -from typing import List, Literal, Tuple +from typing import List, Literal, Optional, Tuple import pytest @@ -7,8 +7,8 @@ def generate_extension_expectations( - extension: str, expectation: Literal["image", "video", "pdf", "dicom"] -) -> List[Tuple[str, Literal["image", "video", "pdf", "dicom"]]]: + extension: str, expectation: Optional[Literal["image", "video", "pdf", "dicom"]] +) -> List[Tuple[str, Optional[Literal["image", "video", "pdf", "dicom"]]]]: """ Generate a list of tuples of the form (file_name, expectation) where """ @@ -16,6 +16,7 @@ def generate_extension_expectations( (f"file.{extension}", expectation), (f"file.with.dots.{extension}", expectation), (f"/file/with/slashes.{extension}", expectation), + (f"file/with/slashes.{extension}", expectation), ] @@ -39,13 +40,13 @@ def generate_extension_expectations( *generate_extension_expectations("wmv", "video"), *generate_extension_expectations("mkv", "video"), # Unsupported - *generate_extension_expectations("unsupported", "image"), + *generate_extension_expectations("unsupported", None), ] class TestValidateNoSlashes: @pytest.mark.parametrize( - "string", [("validname"), ("valid/name"), ("valid/name/still")] + "string", [("validname"), ("valid-name"), ("valid_name_still")] ) def test_happy_paths(self, string: str) -> None: assert validate_no_slashes(string) == string @@ -70,19 +71,22 @@ def test_sad_paths(self, string: UnknownType) -> None: class TestFpsValidator: + def test_sets_value_if_absent(self) -> None: + assert ItemSlot.validate_fps({}) == {"fps": 0} + @pytest.mark.parametrize("fps", [(0), (1), (1.0), ("native")]) def test_happy_paths(self, fps: UnknownType) -> None: - assert ItemSlot.validate_fps(fps) == fps + assert ItemSlot.validate_fps({"fps": fps}) == {"fps": fps} @pytest.mark.parametrize("fps", [(-1), ("invalid")]) def test_sad_paths(self, fps: UnknownType) -> None: with pytest.raises(AssertionError): - ItemSlot.validate_fps(fps) + ItemSlot.validate_fps({"fps": fps}) class TestRootValidator: @pytest.mark.parametrize("file_name, expectation", expectations_list) def test_happy_paths(self, file_name: str, expectation: str) -> None: assert ( - ItemSlot.infer_type({"file_name": file_name})["type"] == expectation + ItemSlot.infer_type({"file_name": file_name}).get("type") == expectation ), f"Failed for {file_name}, got {expectation}" diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 8cf2b8a26..64d0bca8c 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -9,7 +9,7 @@ import darwin.future.core.items.uploads as uploads from darwin.future.core.client import ClientCore, DarwinConfig -from darwin.future.data_objects.item import Item, ItemLayoutV1, ItemLayoutV2, ItemSlot +from darwin.future.data_objects.item import Item, ItemLayout, ItemSlot from darwin.future.exceptions import DarwinException from darwin.future.tests.core.fixtures import * # noqa: F401,F403 @@ -22,7 +22,17 @@ class TestBuildSlots: items_and_expectations: List[Tuple[Item, BUILD_SLOT_RETURN_TYPE]] = [] # Test empty slots - items_and_expectations.append((Item(name="name_with_no_slots", slots=[]), [])) + items_and_expectations.append( + ( + Item( + name="name_with_no_slots", + slots=[], + dataset_id=1, + processing_status="processing", + ), + [], + ) + ) # Test Simple slot with no non-required fields items_and_expectations.append( @@ -36,6 +46,8 @@ class TestBuildSlots: storage_key="storage_key", ) ], + dataset_id=1, + processing_status="processing", ), [ { @@ -54,6 +66,8 @@ class TestBuildSlots: ( Item( name="name_with_multiple_slots", + dataset_id=1, + processing_status="processing", slots=[ ItemSlot( slot_name="slot_name1", @@ -91,6 +105,8 @@ class TestBuildSlots: ( Item( name="name_testing_as_frames", + dataset_id=1, + processing_status="processing", slots=[ ItemSlot( slot_name="slot_name1", @@ -144,6 +160,8 @@ class TestBuildSlots: ( Item( name="name_testing_extract_views", + dataset_id=1, + processing_status="processing", slots=[ ItemSlot( slot_name="slot_name1", @@ -197,6 +215,8 @@ class TestBuildSlots: ( Item( name="name_with_simple_slot", + dataset_id=1, + processing_status="processing", slots=[ ItemSlot( slot_name="slot_name25", @@ -261,6 +281,8 @@ class TestBuildSlots: ( Item( name="name_with_simple_slot", + dataset_id=1, + processing_status="processing", slots=[ ItemSlot( slot_name="slot_name", @@ -313,6 +335,8 @@ class TestBuildSlots: ( Item( name="name_testing_tags", + dataset_id=1, + processing_status="processing", slots=[ ItemSlot( slot_name="slot_name_with_string_list", @@ -362,7 +386,9 @@ class TestBuildLayout: ( Item( name="test_item", - layout=ItemLayoutV1(version=1, type="grid", slots=["slot1", "slot2"]), + layout=ItemLayout(version=1, type="grid", slots=["slot1", "slot2"]), + dataset_id=1, + processing_status="processing", ), { "slots": ["slot1", "slot2"], @@ -373,7 +399,9 @@ class TestBuildLayout: ( Item( name="test_item", - layout=ItemLayoutV2( + dataset_id=1, + processing_status="processing", + layout=ItemLayout( version=2, type="grid", slots=["slot1", "slot2"], @@ -402,6 +430,8 @@ class TestBuildPayloadItems: ( Item( name="test_item", + dataset_id=1, + processing_status="processing", slots=[ ItemSlot( slot_name="slot_name_with_string_list", @@ -469,6 +499,8 @@ def items_and_paths(self) -> List[Tuple[Item, Path]]: ( Item( name="test_item", + dataset_id=1, + processing_status="processing", slots=[ ItemSlot( slot_name="slot_name_with_string_list", From db8ec1fb07827c0ff3acf46eca3f0e335cd1ecc1 Mon Sep 17 00:00:00 2001 From: Owen Date: Fri, 20 Oct 2023 16:16:33 +0100 Subject: [PATCH 20/25] Discarded idea of returning Items --- darwin/future/core/items/uploads.py | 70 ++++++++--------------------- darwin/future/data_objects/item.py | 11 +++-- 2 files changed, 26 insertions(+), 55 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index 42505b7aa..1eee98c27 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -3,8 +3,7 @@ from pathlib import Path from typing import Dict, List, Tuple, Union -from pydantic import parse_obj_as - +from darwin.datatypes import JSONType from darwin.future.core.client import ClientCore from darwin.future.data_objects.item import Item from darwin.future.data_objects.typing import UnknownType @@ -122,7 +121,7 @@ async def async_register_upload( force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, -) -> Item: +) -> Dict: """ Registers an upload for a dataset that can then be used to upload files to Darwin @@ -147,8 +146,7 @@ async def async_register_upload( if isinstance(items_and_paths, tuple): items_and_paths = [items_and_paths] assert all( - (isinstance(item, Item) and isinstance(path, Path)) - for item, path in items_and_paths + (isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths ), "items must be a list of Items" payload_items = await _build_payload_items(items_and_paths) @@ -166,16 +164,14 @@ async def async_register_upload( } try: - response = api_client.post( - f"/v2/teams/{team_slug}/items/register_upload", payload - ) + response = api_client.post(f"/v2/teams/{team_slug}/items/register_upload", payload) except Exception as exc: logger.error(f"Failed to register upload in {__name__}", exc_info=exc) raise DarwinException(f"Failed to register upload in {__name__}") from exc assert isinstance(response, dict), "Unexpected return type from register upload" - return parse_obj_as(Item, response) + return response async def async_create_signed_upload_url( @@ -201,40 +197,24 @@ async def async_create_signed_upload_url( The response from the API """ try: - response = api_client.post( - f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={} - ) + response = api_client.post(f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={}) except Exception as exc: logger.error(f"Failed to create signed upload url in {__name__}", exc_info=exc) - raise DarwinException( - f"Failed to create signed upload url in {__name__}" - ) from exc + raise DarwinException(f"Failed to create signed upload url in {__name__}") from exc - assert isinstance( - response, dict - ), "Unexpected return type from create signed upload url" + assert isinstance(response, dict), "Unexpected return type from create signed upload url" if not response: - logger.error( - f"Failed to create signed upload url in {__name__}, got no response" - ) - raise DarwinException( - f"Failed to create signed upload url in {__name__}, got no response" - ) + logger.error(f"Failed to create signed upload url in {__name__}, got no response") + raise DarwinException(f"Failed to create signed upload url in {__name__}, got no response") if "errors" in response: - logger.error( - f"Failed to create signed upload url in {__name__}, got errors: {response['errors']}" - ) + logger.error(f"Failed to create signed upload url in {__name__}, got errors: {response['errors']}") raise DarwinException(f"Failed to create signed upload url in {__name__}") if "upload_url" not in response: - logger.error( - f"Failed to create signed upload url in {__name__}, got no upload_url" - ) - raise DarwinException( - f"Failed to create signed upload url in {__name__}, got no upload_url" - ) + logger.error(f"Failed to create signed upload url in {__name__}, got no upload_url") + raise DarwinException(f"Failed to create signed upload url in {__name__}, got no upload_url") return response["upload_url"] @@ -284,22 +264,16 @@ async def async_register_and_create_signed_upload_url( ignore_dicom_layout, ) - assert isinstance(register, dict), "Unexpected return type from register upload" - download_id = register["id"] if "errors" in register or not download_id: raise DarwinException(f"Failed to register upload in {__name__}") - signed_info = await async_create_signed_upload_url( - api_client, team_slug, download_id - ) + signed_info = await async_create_signed_upload_url(api_client, team_slug, download_id) return signed_info -async def async_confirm_upload( - api_client: ClientCore, team_slug: str, upload_id: str -) -> None: +async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> None: """ Asynchronously confirm an upload/uploads was successful by ID @@ -319,9 +293,7 @@ async def async_confirm_upload( """ try: - response = api_client.post( - f"/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={} - ) + response = api_client.post(f"/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={}) except Exception as exc: logger.error(f"Failed to confirm upload in {__name__}", exc_info=exc) raise DarwinException(f"Failed to confirm upload in {__name__}") from exc @@ -330,14 +302,10 @@ async def async_confirm_upload( if not response: logger.error(f"Failed to confirm upload in {__name__}, got no response") - raise DarwinException( - f"Failed to confirm upload in {__name__}, got no response" - ) + raise DarwinException(f"Failed to confirm upload in {__name__}, got no response") if "errors" in response: - logger.error( - f"Failed to confirm upload in {__name__}, got errors: {response['errors']}" - ) + logger.error(f"Failed to confirm upload in {__name__}, got errors: {response['errors']}") raise DarwinException(f"Failed to confirm upload in {__name__}") @@ -349,7 +317,7 @@ def register_upload( force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, -) -> Item: +) -> Dict: """ Asynchronously register an upload/uploads for a dataset that can then be used to upload files to Darwin diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index 70729ffab..b82f26ee6 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -31,7 +31,7 @@ class ItemLayout(DefaultDarwin): layout_shape: Optional[List[int]] = None @validator("layout_shape", always=True) - def layout_validator(cls, value: UnknownType, values: Dict): + def layout_validator(cls, value: UnknownType, values: Dict) -> Dict: if not value and values.get("version") == 2: raise ValueError("layout_shape must be specified for version 2 layouts") @@ -61,7 +61,7 @@ def validate_slot_name(cls, v: UnknownType) -> str: return v @classmethod - def validate_fps(cls, values): + def validate_fps(cls, values: dict): value = values.get("fps") if value is None: @@ -77,7 +77,8 @@ def validate_fps(cls, values): assert value == 0, "fps must be 0 for images" else: assert value >= 0, "fps must be greater than or equal to 0 for videos" - return value + + return values @classmethod def infer_type(cls, values: Dict[str, UnknownType]) -> Dict[str, UnknownType]: @@ -100,10 +101,12 @@ class Config: smart_union = True @root_validator - def root(cls, v, values): + def root(cls, v: UnknownType, values: Dict) -> Dict: values = cls.infer_type(values) values = cls.validate_fps(values) + return values + class Item(DefaultDarwin): # GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.NewItem From 2604ec9e007a388edacfbaa67bc8428cbcdf7369 Mon Sep 17 00:00:00 2001 From: Owen Date: Fri, 20 Oct 2023 17:09:53 +0100 Subject: [PATCH 21/25] Fixing some tests --- darwin/future/core/items/uploads.py | 23 +++++++++--- darwin/future/data_objects/item.py | 7 ++-- darwin/future/tests/core/items/fixtures.py | 4 +-- .../tests/core/items/test_upload_items.py | 36 +++++++++---------- 4 files changed, 40 insertions(+), 30 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index 1eee98c27..80565d389 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -36,11 +36,12 @@ async def _build_slots(item: Item) -> List[Dict]: slot_dict: Dict[str, UnknownType] = { "slot_name": slot.slot_name, "file_name": slot.file_name, - "storage_key": slot.storage_key, "fps": slot.fps, - "type": slot.type, } + if slot.storage_key is not None: + slot_dict["storage_key"] = slot.storage_key + if slot.as_frames is not None: slot_dict["as_frames"] = slot.as_frames @@ -53,6 +54,9 @@ async def _build_slots(item: Item) -> List[Dict]: if slot.tags is not None: slot_dict["tags"] = slot.tags + if slot.type is not None: + slot_dict["type"] = slot.type + slots_to_return.append(slot_dict) return slots_to_return @@ -98,12 +102,21 @@ async def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List for item, path in items_and_paths: base_item = { "name": getattr(item, "name"), + "id": getattr(item, "id"), + "slots": await _build_slots(item), + "dataset_id": getattr(item, "dataset_id"), "path:": str(path), - "tags": getattr(item, "tags", []), + "processing_status": getattr(item, "processing_status"), } - if getattr(item, "slots", None): - base_item["slots"] = await _build_slots(item) + if getattr(item, "archived", None): + base_item["archived"] = item.archived + + if getattr(item, "priority", None): + base_item["priority"] = item.priority + + if getattr(item, "tags", None): + base_item["tags"] = item.tags if getattr(item, "layout", None): base_item["layout"] = await _build_layout(item) diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index b82f26ee6..4d623eb63 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -1,5 +1,6 @@ # @see: GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.ExistingItem from typing import Dict, List, Literal, Optional, Union +from uuid import UUID from pydantic import root_validator, validator @@ -44,12 +45,12 @@ class ItemSlot(DefaultDarwin): # Required fields slot_name: str file_name: str + fps: Optional[ItemFrameRate] = None # Optional fields storage_key: Optional[str] = None as_frames: Optional[bool] = None extract_views: Optional[bool] = None - fps: Optional[ItemFrameRate] = None metadata: Optional[Dict[str, UnknownType]] = None tags: Optional[Union[List[str], Dict[str, str]]] = None type: Optional[Literal["image", "video", "pdf", "dicom"]] = None @@ -101,7 +102,7 @@ class Config: smart_union = True @root_validator - def root(cls, v: UnknownType, values: Dict) -> Dict: + def root(cls, values: Dict) -> Dict: values = cls.infer_type(values) values = cls.validate_fps(values) @@ -113,12 +114,14 @@ class Item(DefaultDarwin): # Required fields name: str + id: UUID slots: List[ItemSlot] = [] path: str = "/" dataset_id: int processing_status: str # Optional fields + archived: Optional[bool] = False priority: Optional[int] = None tags: Optional[Union[List[str], Dict[str, str]]] = [] layout: Optional[ItemLayout] = None diff --git a/darwin/future/tests/core/items/fixtures.py b/darwin/future/tests/core/items/fixtures.py index 806997042..b0fcb9402 100644 --- a/darwin/future/tests/core/items/fixtures.py +++ b/darwin/future/tests/core/items/fixtures.py @@ -13,9 +13,7 @@ def base_items() -> List[Item]: name=f"test_{i}", path="test_path", dataset_id=1, - id=uuid4(), - archived=False, - layout={}, + id=UUID("00000000-0000-0000-0000-000000000000"), slots=[], processing_status="complete", priority=0, diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 64d0bca8c..1ee24c121 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -2,6 +2,7 @@ from pathlib import Path from typing import Dict, Generator, List, Tuple from unittest.mock import MagicMock, Mock, patch +from uuid import UUID import orjson as json import pytest @@ -26,6 +27,7 @@ class TestBuildSlots: ( Item( name="name_with_no_slots", + id=UUID("00000000-0000-0000-0000-000000000000"), slots=[], dataset_id=1, processing_status="processing", @@ -39,6 +41,7 @@ class TestBuildSlots: ( Item( name="name_with_simple_slot", + id=UUID("00000000-0000-0000-0000-000000000000"), slots=[ ItemSlot( slot_name="slot_name_simple", @@ -54,7 +57,6 @@ class TestBuildSlots: "slot_name": "slot_name_simple", "file_name": "file_name", "storage_key": "storage_key", - "type": "image", "fps": 0, } ], @@ -66,6 +68,7 @@ class TestBuildSlots: ( Item( name="name_with_multiple_slots", + id=UUID("00000000-0000-0000-0000-000000000000"), dataset_id=1, processing_status="processing", slots=[ @@ -86,14 +89,12 @@ class TestBuildSlots: "slot_name": "slot_name1", "file_name": "file_name1", "storage_key": "storage_key1", - "type": "image", "fps": 0, }, { "slot_name": "slot_name2", "file_name": "file_name2", "storage_key": "storage_key2", - "type": "image", "fps": 0, }, ], @@ -105,6 +106,7 @@ class TestBuildSlots: ( Item( name="name_testing_as_frames", + id=UUID("00000000-0000-0000-0000-000000000000"), dataset_id=1, processing_status="processing", slots=[ @@ -133,7 +135,6 @@ class TestBuildSlots: "file_name": "file_name", "storage_key": "storage_key", "fps": 0, - "type": "image", "as_frames": True, }, { @@ -141,7 +142,6 @@ class TestBuildSlots: "file_name": "file_name", "storage_key": "storage_key", "fps": 0, - "type": "image", "as_frames": False, }, { @@ -149,7 +149,6 @@ class TestBuildSlots: "file_name": "file_name", "storage_key": "storage_key", "fps": 0, - "type": "image", }, ], ) @@ -160,6 +159,7 @@ class TestBuildSlots: ( Item( name="name_testing_extract_views", + id=UUID("00000000-0000-0000-0000-000000000000"), dataset_id=1, processing_status="processing", slots=[ @@ -188,7 +188,6 @@ class TestBuildSlots: "file_name": "file_name", "storage_key": "storage_key", "fps": 0, - "type": "image", "extract_views": True, }, { @@ -196,7 +195,6 @@ class TestBuildSlots: "file_name": "file_name", "storage_key": "storage_key", "fps": 0, - "type": "image", "extract_views": False, }, { @@ -204,7 +202,6 @@ class TestBuildSlots: "file_name": "file_name", "storage_key": "storage_key", "fps": 0, - "type": "image", }, ], ) @@ -215,6 +212,7 @@ class TestBuildSlots: ( Item( name="name_with_simple_slot", + id=UUID("00000000-0000-0000-0000-000000000000"), dataset_id=1, processing_status="processing", slots=[ @@ -248,28 +246,24 @@ class TestBuildSlots: "slot_name": "slot_name25", "file_name": "file_name", "storage_key": "storage_key", - "type": "image", "fps": 25, }, { "slot_name": "slot_name29.997", "file_name": "file_name", "storage_key": "storage_key", - "type": "image", "fps": 29.997, }, { "slot_name": "slot_namenative", "file_name": "file_name", "storage_key": "storage_key", - "type": "image", "fps": "native", }, { "slot_name": "slot_name", "file_name": "file_name", "storage_key": "storage_key", - "type": "image", "fps": 0, }, ], @@ -281,6 +275,7 @@ class TestBuildSlots: ( Item( name="name_with_simple_slot", + id=UUID("00000000-0000-0000-0000-000000000000"), dataset_id=1, processing_status="processing", slots=[ @@ -309,7 +304,6 @@ class TestBuildSlots: "file_name": "file_name", "storage_key": "storage_key", "fps": 0, - "type": "image", "metadata": {"key": "value"}, }, { @@ -317,14 +311,12 @@ class TestBuildSlots: "file_name": "file_name", "storage_key": "storage_key", "fps": 0, - "type": "image", }, { "slot_name": "slot_name", "file_name": "file_name", "storage_key": "storage_key", "fps": 0, - "type": "image", }, ], ) @@ -335,6 +327,7 @@ class TestBuildSlots: ( Item( name="name_testing_tags", + id=UUID("00000000-0000-0000-0000-000000000000"), dataset_id=1, processing_status="processing", slots=[ @@ -359,7 +352,6 @@ class TestBuildSlots: "storage_key": "storage_key", "tags": ["tag1", "tag2"], "fps": 0, - "type": "image", }, { "slot_name": "slot_name_with_kv_pairs", @@ -367,7 +359,6 @@ class TestBuildSlots: "storage_key": "storage_key", "tags": {"key": "value"}, "fps": 0, - "type": "image", }, ], ) @@ -386,6 +377,7 @@ class TestBuildLayout: ( Item( name="test_item", + id=UUID("00000000-0000-0000-0000-000000000000"), layout=ItemLayout(version=1, type="grid", slots=["slot1", "slot2"]), dataset_id=1, processing_status="processing", @@ -399,6 +391,7 @@ class TestBuildLayout: ( Item( name="test_item", + id=UUID("00000000-0000-0000-0000-000000000000"), dataset_id=1, processing_status="processing", layout=ItemLayout( @@ -430,6 +423,7 @@ class TestBuildPayloadItems: ( Item( name="test_item", + id=UUID("00000000-0000-0000-0000-000000000000"), dataset_id=1, processing_status="processing", slots=[ @@ -453,6 +447,9 @@ class TestBuildPayloadItems: [ { "name": "test_item", + "id": "00000000-0000-0000-0000-000000000000", + "dataset_id": 1, + "processing_status": "processing", "path:": "test_path", "tags": [], "slots": [ @@ -462,7 +459,6 @@ class TestBuildPayloadItems: "storage_key": "storage_key", "tags": ["tag1", "tag2"], "fps": 0, - "type": "image", }, { "slot_name": "slot_name_with_kv_pairs", @@ -470,7 +466,6 @@ class TestBuildPayloadItems: "storage_key": "storage_key", "tags": {"key": "value"}, "fps": 0, - "type": "image", }, ], } @@ -499,6 +494,7 @@ def items_and_paths(self) -> List[Tuple[Item, Path]]: ( Item( name="test_item", + id=UUID("00000000-0000-0000-0000-000000000000"), dataset_id=1, processing_status="processing", slots=[ From 4fe4381284879ff5703c00a9f47e7c58de9d8bed Mon Sep 17 00:00:00 2001 From: Owen Date: Mon, 23 Oct 2023 11:44:56 +0100 Subject: [PATCH 22/25] Test fixes --- darwin/future/core/items/uploads.py | 8 ++--- .../tests/core/items/test_upload_items.py | 35 +++++++++++++++---- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index 80565d389..e2bc584a0 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -105,7 +105,7 @@ async def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List "id": getattr(item, "id"), "slots": await _build_slots(item), "dataset_id": getattr(item, "dataset_id"), - "path:": str(path), + "path": str(path), "processing_status": getattr(item, "processing_status"), } @@ -313,13 +313,9 @@ async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id assert isinstance(response, dict), "Unexpected return type from confirm upload" - if not response: - logger.error(f"Failed to confirm upload in {__name__}, got no response") - raise DarwinException(f"Failed to confirm upload in {__name__}, got no response") - if "errors" in response: logger.error(f"Failed to confirm upload in {__name__}, got errors: {response['errors']}") - raise DarwinException(f"Failed to confirm upload in {__name__}") + raise DarwinException(f"Failed to confirm upload in {__name__}: {str(response['errors'])}") def register_upload( diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 1ee24c121..887bd1f39 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -450,8 +450,7 @@ class TestBuildPayloadItems: "id": "00000000-0000-0000-0000-000000000000", "dataset_id": 1, "processing_status": "processing", - "path:": "test_path", - "tags": [], + "path": "test_path", "slots": [ { "slot_name": "slot_name_with_string_list", @@ -475,7 +474,9 @@ class TestBuildPayloadItems: ) def test_build_payload_items(self, items_and_paths: List[Tuple[Item, Path]], expected: List[Dict]) -> None: result = asyncio.run(uploads._build_payload_items(items_and_paths)) - assert result == expected + result_with_uuid_as_str = [{**item, "id": str(item["id"])} for item in result] + + assert result_with_uuid_as_str == expected class SetupTests: @@ -528,7 +529,7 @@ def test_async_register_upload_happy_path( items = [ { "name": "test_item", - "path:": "test_path", + "path": "test_path", "tags": [], "slots": [ { @@ -635,7 +636,7 @@ def test_async_create_signed_upload_url(self, default_url: str, base_config: Dar if not actual_response: pytest.fail("Response was None") - assert actual_response == expected_response + assert actual_response == expected_response["upload_url"] def test_async_create_signed_upload_url_raises(self, base_client: ClientCore) -> None: base_client.post = MagicMock() # type: ignore @@ -740,8 +741,28 @@ def test_async_confirm_upload(self, base_client: ClientCore, default_url: str) - ) ) - # Check that the response matches the expected response - assert actual_response == {} + assert actual_response is None # Function doesn't return anything on success + + @responses.activate + def test_async_confirm_upload_raises_on_returned_errors(self, base_client: ClientCore, default_url: str) -> None: + # Call the function with mocked arguments + responses.add( + "POST", + f"{default_url}/uploads/123/confirm", + status=200, # API will not normally return this on success, but we're not test API + json={"errors": ["error1", "error2"]}, + ) + + with pytest.raises(DarwinException) as exc: + asyncio.run( + uploads.async_confirm_upload( + base_client, + "my-team", + "123", + ) + ) + + assert "Failed to confirm upload in darwin.future.core.items.uploads: ['error1', 'error2']" in str(exc) def test_async_confirm_upload_raises(self, base_client: ClientCore) -> None: base_client.post = MagicMock() # type: ignore From 303503740bb9a3e690f5be00906c7ab16eee51a5 Mon Sep 17 00:00:00 2001 From: Owen Date: Mon, 23 Oct 2023 11:47:01 +0100 Subject: [PATCH 23/25] Lint fixes --- darwin/future/core/items/uploads.py | 60 ++++++++++++++----- .../tests/core/items/test_upload_items.py | 49 +++++++++++---- 2 files changed, 81 insertions(+), 28 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index e2bc584a0..f602d7204 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -3,7 +3,6 @@ from pathlib import Path from typing import Dict, List, Tuple, Union -from darwin.datatypes import JSONType from darwin.future.core.client import ClientCore from darwin.future.data_objects.item import Item from darwin.future.data_objects.typing import UnknownType @@ -159,7 +158,8 @@ async def async_register_upload( if isinstance(items_and_paths, tuple): items_and_paths = [items_and_paths] assert all( - (isinstance(item, Item) and isinstance(path, Path)) for item, path in items_and_paths + (isinstance(item, Item) and isinstance(path, Path)) + for item, path in items_and_paths ), "items must be a list of Items" payload_items = await _build_payload_items(items_and_paths) @@ -177,7 +177,9 @@ async def async_register_upload( } try: - response = api_client.post(f"/v2/teams/{team_slug}/items/register_upload", payload) + response = api_client.post( + f"/v2/teams/{team_slug}/items/register_upload", payload + ) except Exception as exc: logger.error(f"Failed to register upload in {__name__}", exc_info=exc) raise DarwinException(f"Failed to register upload in {__name__}") from exc @@ -210,24 +212,40 @@ async def async_create_signed_upload_url( The response from the API """ try: - response = api_client.post(f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={}) + response = api_client.post( + f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={} + ) except Exception as exc: logger.error(f"Failed to create signed upload url in {__name__}", exc_info=exc) - raise DarwinException(f"Failed to create signed upload url in {__name__}") from exc + raise DarwinException( + f"Failed to create signed upload url in {__name__}" + ) from exc - assert isinstance(response, dict), "Unexpected return type from create signed upload url" + assert isinstance( + response, dict + ), "Unexpected return type from create signed upload url" if not response: - logger.error(f"Failed to create signed upload url in {__name__}, got no response") - raise DarwinException(f"Failed to create signed upload url in {__name__}, got no response") + logger.error( + f"Failed to create signed upload url in {__name__}, got no response" + ) + raise DarwinException( + f"Failed to create signed upload url in {__name__}, got no response" + ) if "errors" in response: - logger.error(f"Failed to create signed upload url in {__name__}, got errors: {response['errors']}") + logger.error( + f"Failed to create signed upload url in {__name__}, got errors: {response['errors']}" + ) raise DarwinException(f"Failed to create signed upload url in {__name__}") if "upload_url" not in response: - logger.error(f"Failed to create signed upload url in {__name__}, got no upload_url") - raise DarwinException(f"Failed to create signed upload url in {__name__}, got no upload_url") + logger.error( + f"Failed to create signed upload url in {__name__}, got no upload_url" + ) + raise DarwinException( + f"Failed to create signed upload url in {__name__}, got no upload_url" + ) return response["upload_url"] @@ -281,12 +299,16 @@ async def async_register_and_create_signed_upload_url( if "errors" in register or not download_id: raise DarwinException(f"Failed to register upload in {__name__}") - signed_info = await async_create_signed_upload_url(api_client, team_slug, download_id) + signed_info = await async_create_signed_upload_url( + api_client, team_slug, download_id + ) return signed_info -async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id: str) -> None: +async def async_confirm_upload( + api_client: ClientCore, team_slug: str, upload_id: str +) -> None: """ Asynchronously confirm an upload/uploads was successful by ID @@ -306,7 +328,9 @@ async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id """ try: - response = api_client.post(f"/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={}) + response = api_client.post( + f"/v2/teams/{team_slug}/items/uploads/{upload_id}/confirm", data={} + ) except Exception as exc: logger.error(f"Failed to confirm upload in {__name__}", exc_info=exc) raise DarwinException(f"Failed to confirm upload in {__name__}") from exc @@ -314,8 +338,12 @@ async def async_confirm_upload(api_client: ClientCore, team_slug: str, upload_id assert isinstance(response, dict), "Unexpected return type from confirm upload" if "errors" in response: - logger.error(f"Failed to confirm upload in {__name__}, got errors: {response['errors']}") - raise DarwinException(f"Failed to confirm upload in {__name__}: {str(response['errors'])}") + logger.error( + f"Failed to confirm upload in {__name__}, got errors: {response['errors']}" + ) + raise DarwinException( + f"Failed to confirm upload in {__name__}: {str(response['errors'])}" + ) def register_upload( diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 887bd1f39..998387dab 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -472,7 +472,9 @@ class TestBuildPayloadItems: ) ], ) - def test_build_payload_items(self, items_and_paths: List[Tuple[Item, Path]], expected: List[Dict]) -> None: + def test_build_payload_items( + self, items_and_paths: List[Tuple[Item, Path]], expected: List[Dict] + ) -> None: result = asyncio.run(uploads._build_payload_items(items_and_paths)) result_with_uuid_as_str = [{**item, "id": str(item["id"])} for item in result] @@ -553,7 +555,9 @@ def test_async_register_upload_happy_path( ] mock_build_payload_items.return_value = items - responses.add(responses.POST, f"{default_url}/register_upload", json={"status": "success"}) + responses.add( + responses.POST, f"{default_url}/register_upload", json={"status": "success"} + ) asyncio.run( uploads.async_register_upload( api_client=base_client, @@ -580,7 +584,9 @@ def test_async_register_upload_happy_path( }, "The request body should be empty" @patch.object(uploads, "_build_payload_items") - def test_async_register_upload_raises(self, mock_build_payload_items: MagicMock, base_client: ClientCore) -> None: + def test_async_register_upload_raises( + self, mock_build_payload_items: MagicMock, base_client: ClientCore + ) -> None: with pytest.raises(DarwinException) as exc: mock_build_payload_items.side_effect = DarwinException("Error1") @@ -618,7 +624,9 @@ def test_async_register_upload_raises(self, mock_build_payload_items: MagicMock, class TestCreateSignedUploadUrl(SetupTests): - def test_async_create_signed_upload_url(self, default_url: str, base_config: DarwinConfig) -> None: + def test_async_create_signed_upload_url( + self, default_url: str, base_config: DarwinConfig + ) -> None: with responses.RequestsMock() as rsps: # Mock the API response expected_response = {"upload_url": "https://signed.url"} @@ -630,7 +638,9 @@ def test_async_create_signed_upload_url(self, default_url: str, base_config: Dar # Call the function with mocked arguments api_client = ClientCore(base_config) - actual_response = asyncio.run(uploads.async_create_signed_upload_url(api_client, "1", "my-team")) + actual_response = asyncio.run( + uploads.async_create_signed_upload_url(api_client, "1", "my-team") + ) # Check that the response matches the expected response if not actual_response: @@ -638,12 +648,16 @@ def test_async_create_signed_upload_url(self, default_url: str, base_config: Dar assert actual_response == expected_response["upload_url"] - def test_async_create_signed_upload_url_raises(self, base_client: ClientCore) -> None: + def test_async_create_signed_upload_url_raises( + self, base_client: ClientCore + ) -> None: base_client.post = MagicMock() # type: ignore base_client.post.side_effect = DarwinException("Error") with pytest.raises(DarwinException): - asyncio.run(uploads.async_create_signed_upload_url(base_client, "1", "my-team")) + asyncio.run( + uploads.async_create_signed_upload_url(base_client, "1", "my-team") + ) class TestRegisterAndCreateSignedUploadUrl: @@ -724,7 +738,9 @@ def test_async_register_and_create_signed_upload_url_raises( class TestConfirmUpload(SetupTests): @responses.activate - def test_async_confirm_upload(self, base_client: ClientCore, default_url: str) -> None: + def test_async_confirm_upload( + self, base_client: ClientCore, default_url: str + ) -> None: # Call the function with mocked arguments responses.add( "POST", @@ -744,7 +760,9 @@ def test_async_confirm_upload(self, base_client: ClientCore, default_url: str) - assert actual_response is None # Function doesn't return anything on success @responses.activate - def test_async_confirm_upload_raises_on_returned_errors(self, base_client: ClientCore, default_url: str) -> None: + def test_async_confirm_upload_raises_on_returned_errors( + self, base_client: ClientCore, default_url: str + ) -> None: # Call the function with mocked arguments responses.add( "POST", @@ -762,7 +780,10 @@ def test_async_confirm_upload_raises_on_returned_errors(self, base_client: Clien ) ) - assert "Failed to confirm upload in darwin.future.core.items.uploads: ['error1', 'error2']" in str(exc) + assert ( + "Failed to confirm upload in darwin.future.core.items.uploads: ['error1', 'error2']" + in str(exc) + ) def test_async_confirm_upload_raises(self, base_client: ClientCore) -> None: base_client.post = MagicMock() # type: ignore @@ -785,7 +806,9 @@ def mock_async_create_signed_upload_url(self) -> Generator: @pytest.fixture def mock_async_register_and_create_signed_upload_url(self) -> Generator: - with patch.object(uploads, "async_register_and_create_signed_upload_url") as mock: + with patch.object( + uploads, "async_register_and_create_signed_upload_url" + ) as mock: yield mock @pytest.fixture @@ -816,7 +839,9 @@ def test_register_and_create_signed_upload_url( mock_async_register_and_create_signed_upload_url: MagicMock, base_client: ClientCore, ) -> None: - uploads.register_and_create_signed_upload_url(base_client, "team", "dataset", [(Mock(), Mock())]) + uploads.register_and_create_signed_upload_url( + base_client, "team", "dataset", [(Mock(), Mock())] + ) mock_async_register_and_create_signed_upload_url.assert_called_once() From cb07c9736e8e4941563936434291ae95661de1d1 Mon Sep 17 00:00:00 2001 From: Owen Date: Mon, 23 Oct 2023 13:57:40 +0100 Subject: [PATCH 24/25] Refactor to introduce UploadItem to make ID and other non present members unnecessary at upload --- darwin/future/core/items/uploads.py | 35 ++++---- darwin/future/data_objects/item.py | 18 ++++ .../tests/core/items/test_upload_items.py | 84 +++++-------------- 3 files changed, 52 insertions(+), 85 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index f602d7204..a4413b2e5 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -4,20 +4,20 @@ from typing import Dict, List, Tuple, Union from darwin.future.core.client import ClientCore -from darwin.future.data_objects.item import Item +from darwin.future.data_objects.item import UploadItem from darwin.future.data_objects.typing import UnknownType from darwin.future.exceptions import DarwinException logger = getLogger(__name__) -async def _build_slots(item: Item) -> List[Dict]: +async def _build_slots(item: UploadItem) -> List[Dict]: """ (internal) Builds the slots for an item Parameters ---------- - item: Item + item: UploadItem The item to build slots for Returns @@ -61,7 +61,7 @@ async def _build_slots(item: Item) -> List[Dict]: return slots_to_return -async def _build_layout(item: Item) -> Dict: +async def _build_layout(item: UploadItem) -> Dict: if not item.layout: return {} @@ -83,13 +83,15 @@ async def _build_layout(item: Item) -> Dict: raise DarwinException(f"Invalid layout version {item.layout.version}") -async def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List[Dict]: +async def _build_payload_items( + items_and_paths: List[Tuple[UploadItem, Path]] +) -> List[Dict]: """ Builds the payload for the items to be registered for upload Parameters ---------- - items_and_paths: List[Tuple[Item, Path]] + items_and_paths: List[Tuple[UploadItem, Path]] Returns ------- @@ -101,19 +103,10 @@ async def _build_payload_items(items_and_paths: List[Tuple[Item, Path]]) -> List for item, path in items_and_paths: base_item = { "name": getattr(item, "name"), - "id": getattr(item, "id"), "slots": await _build_slots(item), - "dataset_id": getattr(item, "dataset_id"), "path": str(path), - "processing_status": getattr(item, "processing_status"), } - if getattr(item, "archived", None): - base_item["archived"] = item.archived - - if getattr(item, "priority", None): - base_item["priority"] = item.priority - if getattr(item, "tags", None): base_item["tags"] = item.tags @@ -129,7 +122,7 @@ async def async_register_upload( api_client: ClientCore, team_slug: str, dataset_slug: str, - items_and_paths: Union[Tuple[Item, Path], List[Tuple[Item, Path]]], + items_and_paths: Union[Tuple[UploadItem, Path], List[Tuple[UploadItem, Path]]], force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, @@ -145,7 +138,7 @@ async def async_register_upload( The slug of the team to register the upload for dataset_slug: str The slug of the dataset to register the upload for - items_and_paths: Union[Tuple[Item, Path], List[Tuple[Item, Path]]] + items_and_paths: Union[Tuple[UploadItem, Path], List[Tuple[UploadItem, Path]]] A list of tuples of Items and Paths to register for upload force_tiling: bool Whether to force tiling for the upload @@ -158,7 +151,7 @@ async def async_register_upload( if isinstance(items_and_paths, tuple): items_and_paths = [items_and_paths] assert all( - (isinstance(item, Item) and isinstance(path, Path)) + (isinstance(item, UploadItem) and isinstance(path, Path)) for item, path in items_and_paths ), "items must be a list of Items" @@ -254,7 +247,7 @@ async def async_register_and_create_signed_upload_url( api_client: ClientCore, team_slug: str, dataset_slug: str, - items_and_paths: Union[Tuple[Item, Path], List[Tuple[Item, Path]]], + items_and_paths: Union[Tuple[UploadItem, Path], List[Tuple[UploadItem, Path]]], force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, @@ -350,7 +343,7 @@ def register_upload( api_client: ClientCore, team_slug: str, dataset_slug: str, - items_and_paths: Union[Tuple[Item, Path], List[Tuple[Item, Path]]], + items_and_paths: Union[Tuple[UploadItem, Path], List[Tuple[UploadItem, Path]]], force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, @@ -420,7 +413,7 @@ def register_and_create_signed_upload_url( api_client: ClientCore, team_slug: str, dataset_slug: str, - items_and_paths: Union[List[Tuple[Item, Path]], Tuple[Item, Path]], + items_and_paths: Union[List[Tuple[UploadItem, Path]], Tuple[UploadItem, Path]], force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, diff --git a/darwin/future/data_objects/item.py b/darwin/future/data_objects/item.py index 4d623eb63..e3639be7a 100644 --- a/darwin/future/data_objects/item.py +++ b/darwin/future/data_objects/item.py @@ -109,6 +109,24 @@ def root(cls, values: Dict) -> Dict: return values +class UploadItem(DefaultDarwin): + # GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.NewItem + + # Required fields + name: str + slots: List[ItemSlot] = [] + + # Optional fields + description: Optional[str] = None + path: str = "/" + tags: Optional[Union[List[str], Dict[str, str]]] = [] + layout: Optional[ItemLayout] = None + + @validator("name") + def validate_name(cls, v: UnknownType) -> str: + return validate_no_slashes(v) + + class Item(DefaultDarwin): # GraphotateWeb.Schemas.DatasetsV2.ItemRegistration.NewItem diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 998387dab..4d5a367aa 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -2,7 +2,6 @@ from pathlib import Path from typing import Dict, Generator, List, Tuple from unittest.mock import MagicMock, Mock, patch -from uuid import UUID import orjson as json import pytest @@ -10,7 +9,7 @@ import darwin.future.core.items.uploads as uploads from darwin.future.core.client import ClientCore, DarwinConfig -from darwin.future.data_objects.item import Item, ItemLayout, ItemSlot +from darwin.future.data_objects.item import ItemLayout, ItemSlot, UploadItem from darwin.future.exceptions import DarwinException from darwin.future.tests.core.fixtures import * # noqa: F401,F403 @@ -20,18 +19,12 @@ class TestBuildSlots: BUILD_SLOT_RETURN_TYPE = List[Dict] - items_and_expectations: List[Tuple[Item, BUILD_SLOT_RETURN_TYPE]] = [] + items_and_expectations: List[Tuple[UploadItem, BUILD_SLOT_RETURN_TYPE]] = [] # Test empty slots items_and_expectations.append( ( - Item( - name="name_with_no_slots", - id=UUID("00000000-0000-0000-0000-000000000000"), - slots=[], - dataset_id=1, - processing_status="processing", - ), + UploadItem(name="name_with_no_slots", slots=[]), [], ) ) @@ -39,9 +32,8 @@ class TestBuildSlots: # Test Simple slot with no non-required fields items_and_expectations.append( ( - Item( + UploadItem( name="name_with_simple_slot", - id=UUID("00000000-0000-0000-0000-000000000000"), slots=[ ItemSlot( slot_name="slot_name_simple", @@ -49,8 +41,6 @@ class TestBuildSlots: storage_key="storage_key", ) ], - dataset_id=1, - processing_status="processing", ), [ { @@ -66,11 +56,8 @@ class TestBuildSlots: # Test with multiple slots items_and_expectations.append( ( - Item( + UploadItem( name="name_with_multiple_slots", - id=UUID("00000000-0000-0000-0000-000000000000"), - dataset_id=1, - processing_status="processing", slots=[ ItemSlot( slot_name="slot_name1", @@ -104,11 +91,8 @@ class TestBuildSlots: # Test with `as_frames` optional field items_and_expectations.append( ( - Item( + UploadItem( name="name_testing_as_frames", - id=UUID("00000000-0000-0000-0000-000000000000"), - dataset_id=1, - processing_status="processing", slots=[ ItemSlot( slot_name="slot_name1", @@ -157,11 +141,8 @@ class TestBuildSlots: # Test with `extract_views` optional field items_and_expectations.append( ( - Item( + UploadItem( name="name_testing_extract_views", - id=UUID("00000000-0000-0000-0000-000000000000"), - dataset_id=1, - processing_status="processing", slots=[ ItemSlot( slot_name="slot_name1", @@ -210,11 +191,8 @@ class TestBuildSlots: # Test with `fps` semi-optional field - field defaults to 0 if not provided items_and_expectations.append( ( - Item( + UploadItem( name="name_with_simple_slot", - id=UUID("00000000-0000-0000-0000-000000000000"), - dataset_id=1, - processing_status="processing", slots=[ ItemSlot( slot_name="slot_name25", @@ -273,11 +251,8 @@ class TestBuildSlots: # Test with `metadata` optional field items_and_expectations.append( ( - Item( + UploadItem( name="name_with_simple_slot", - id=UUID("00000000-0000-0000-0000-000000000000"), - dataset_id=1, - processing_status="processing", slots=[ ItemSlot( slot_name="slot_name", @@ -325,11 +300,8 @@ class TestBuildSlots: # Test with `tags` optional field items_and_expectations.append( ( - Item( + UploadItem( name="name_testing_tags", - id=UUID("00000000-0000-0000-0000-000000000000"), - dataset_id=1, - processing_status="processing", slots=[ ItemSlot( slot_name="slot_name_with_string_list", @@ -365,7 +337,7 @@ class TestBuildSlots: ) @pytest.mark.parametrize("item,expected", items_and_expectations) - def test_build_slots(self, item: Item, expected: List[Dict]) -> None: + def test_build_slots(self, item: UploadItem, expected: List[Dict]) -> None: result = asyncio.run(uploads._build_slots(item)) assert result == expected @@ -375,12 +347,9 @@ class TestBuildLayout: "item, expected", [ ( - Item( + UploadItem( name="test_item", - id=UUID("00000000-0000-0000-0000-000000000000"), layout=ItemLayout(version=1, type="grid", slots=["slot1", "slot2"]), - dataset_id=1, - processing_status="processing", ), { "slots": ["slot1", "slot2"], @@ -389,11 +358,8 @@ class TestBuildLayout: }, ), ( - Item( + UploadItem( name="test_item", - id=UUID("00000000-0000-0000-0000-000000000000"), - dataset_id=1, - processing_status="processing", layout=ItemLayout( version=2, type="grid", @@ -410,7 +376,7 @@ class TestBuildLayout: ), ], ) - def test_build_layout(self, item: Item, expected: Dict) -> None: + def test_build_layout(self, item: UploadItem, expected: Dict) -> None: assert asyncio.run(uploads._build_layout(item)) == expected @@ -421,11 +387,8 @@ class TestBuildPayloadItems: ( [ ( - Item( + UploadItem( name="test_item", - id=UUID("00000000-0000-0000-0000-000000000000"), - dataset_id=1, - processing_status="processing", slots=[ ItemSlot( slot_name="slot_name_with_string_list", @@ -447,9 +410,6 @@ class TestBuildPayloadItems: [ { "name": "test_item", - "id": "00000000-0000-0000-0000-000000000000", - "dataset_id": 1, - "processing_status": "processing", "path": "test_path", "slots": [ { @@ -473,12 +433,11 @@ class TestBuildPayloadItems: ], ) def test_build_payload_items( - self, items_and_paths: List[Tuple[Item, Path]], expected: List[Dict] + self, items_and_paths: List[Tuple[UploadItem, Path]], expected: List[Dict] ) -> None: result = asyncio.run(uploads._build_payload_items(items_and_paths)) - result_with_uuid_as_str = [{**item, "id": str(item["id"])} for item in result] - assert result_with_uuid_as_str == expected + assert result == expected class SetupTests: @@ -492,14 +451,11 @@ class TestRegisterUpload(SetupTests): dataset_slug = "my-dataset" @pytest.fixture - def items_and_paths(self) -> List[Tuple[Item, Path]]: + def items_and_paths(self) -> List[Tuple[UploadItem, Path]]: return [ ( - Item( + UploadItem( name="test_item", - id=UUID("00000000-0000-0000-0000-000000000000"), - dataset_id=1, - processing_status="processing", slots=[ ItemSlot( slot_name="slot_name_with_string_list", @@ -526,7 +482,7 @@ def test_async_register_upload_happy_path( mock_build_payload_items: MagicMock, base_client: ClientCore, default_url: str, - items_and_paths: List[Tuple[Item, Path]], + items_and_paths: List[Tuple[UploadItem, Path]], ) -> None: items = [ { From 2adeb124af0a3ec2dc18257c28241e77e9fddcba Mon Sep 17 00:00:00 2001 From: Owen Date: Mon, 23 Oct 2023 16:15:41 +0100 Subject: [PATCH 25/25] Test fix, dogfooding done. --- darwin/future/core/items/uploads.py | 45 +++++++++++++------ .../tests/core/items/test_upload_items.py | 15 ++++--- 2 files changed, 41 insertions(+), 19 deletions(-) diff --git a/darwin/future/core/items/uploads.py b/darwin/future/core/items/uploads.py index a4413b2e5..459759811 100644 --- a/darwin/future/core/items/uploads.py +++ b/darwin/future/core/items/uploads.py @@ -184,8 +184,8 @@ async def async_register_upload( async def async_create_signed_upload_url( api_client: ClientCore, - upload_id: str, team_slug: str, + upload_id: str, ) -> str: """ Asynchronously create a signed upload URL for an upload or uploads @@ -205,8 +205,8 @@ async def async_create_signed_upload_url( The response from the API """ try: - response = api_client.post( - f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign", data={} + response = api_client.get( + f"/v2/teams/{team_slug}/items/uploads/{upload_id}/sign" ) except Exception as exc: logger.error(f"Failed to create signed upload url in {__name__}", exc_info=exc) @@ -251,7 +251,7 @@ async def async_register_and_create_signed_upload_url( force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, -) -> str: +) -> List[Tuple[str, str]]: """ Asynchronously register and create a signed upload URL for an upload or uploads @@ -274,8 +274,8 @@ async def async_register_and_create_signed_upload_url( Returns ------- - str - The signed upload URL + List[Tuple[str, str]] + List of tuples of signed upload urls and upload ids """ register = await async_register_upload( @@ -288,15 +288,34 @@ async def async_register_and_create_signed_upload_url( ignore_dicom_layout, ) - download_id = register["id"] - if "errors" in register or not download_id: + if "errors" in register: raise DarwinException(f"Failed to register upload in {__name__}") - signed_info = await async_create_signed_upload_url( - api_client, team_slug, download_id - ) + if ( + "blocked_items" in register + and isinstance(register["blocked_items"], list) + and len(register["blocked_items"]) > 0 + ): + raise DarwinException( + f"Failed to register upload in {__name__}, got blocked items: {register['blocked_items']}" + ) + + assert "items" in register, "Unexpected return type from register upload" + assert "blocked_items" in register, "Unexpected return type from register upload" + + uploaded_items = register["items"] - return signed_info + upload_ids = [] + for item in uploaded_items: + if "slots" in item: + for slot in item["slots"]: + if "upload_id" in slot: + upload_ids.append(slot["upload_id"]) + + return [ + (await async_create_signed_upload_url(api_client, team_slug, id), id) + for id in upload_ids + ] async def async_confirm_upload( @@ -417,7 +436,7 @@ def register_and_create_signed_upload_url( force_tiling: bool = False, handle_as_slices: bool = False, ignore_dicom_layout: bool = False, -) -> str: +) -> List[Tuple[str, str]]: """ Register and create a signed upload URL for an upload or uploads diff --git a/darwin/future/tests/core/items/test_upload_items.py b/darwin/future/tests/core/items/test_upload_items.py index 4d5a367aa..03794154d 100644 --- a/darwin/future/tests/core/items/test_upload_items.py +++ b/darwin/future/tests/core/items/test_upload_items.py @@ -587,7 +587,7 @@ def test_async_create_signed_upload_url( # Mock the API response expected_response = {"upload_url": "https://signed.url"} rsps.add( - rsps.POST, + rsps.GET, f"{default_url}/uploads/1/sign", json=expected_response, ) @@ -595,7 +595,7 @@ def test_async_create_signed_upload_url( # Call the function with mocked arguments api_client = ClientCore(base_config) actual_response = asyncio.run( - uploads.async_create_signed_upload_url(api_client, "1", "my-team") + uploads.async_create_signed_upload_url(api_client, "my-team", "1") ) # Check that the response matches the expected response @@ -633,8 +633,11 @@ def test_async_register_and_create_signed_upload_url( mock_async_create_signed_upload_url: MagicMock, ) -> None: # Set up mock responses - mock_async_register_upload.return_value = {"id": "123"} - mock_signed_url_response = {"upload_url": "https://signed.url"} + mock_async_register_upload.return_value = { + "blocked_items": [], + "items": [{"id": "123", "slots": [{"upload_id": "321"}]}], + } + mock_signed_url_response = "https://signed.url" mock_async_create_signed_upload_url.return_value = mock_signed_url_response # Set up mock API client @@ -658,11 +661,11 @@ def test_async_register_and_create_signed_upload_url( mock_async_create_signed_upload_url.assert_called_once_with( mock_api_client, "my-team", - "123", + "321", ) # Check that the response matches the expected response - assert actual_response == mock_signed_url_response + assert actual_response == [("https://signed.url", "321")] def test_async_register_and_create_signed_upload_url_raises( self,