From 482aef9889ce4d9506904ba2dbd29b67172382d3 Mon Sep 17 00:00:00 2001 From: David Abramov Date: Thu, 30 Jul 2026 11:12:09 -0700 Subject: [PATCH 1/2] Updating DINO export format --- backend/annotation_server.py | 16 ++++-- backend/coco_export.py | 37 +++++++++---- backend/tests/test_lightly_export.py | 77 +++++++++++++++++++++++----- 3 files changed, 102 insertions(+), 28 deletions(-) diff --git a/backend/annotation_server.py b/backend/annotation_server.py index 44970c1..b177737 100644 --- a/backend/annotation_server.py +++ b/backend/annotation_server.py @@ -55,6 +55,7 @@ from cache import TTLCache from coco_export import ( build_export_plan, + fold_lightly_splits, lightly_classes_map, shape_to_mask, write_coco_split, @@ -982,6 +983,7 @@ def _cb(message: str, _jid: str = jid) -> None: sample_global_stats_fn=images_mod._sample_global_stats, progress_cb=_cb, include_polygons=payload.include_polygons, + lightly=lightly, ) skipped_total += plan["skipped_zero_area"] if not merged_categories: @@ -1011,6 +1013,10 @@ def _cb(message: str, _jid: str = jid) -> None: (c.model_dump() if hasattr(c, "model_dump") else dict(c)) for c in payload.classes ], } + if lightly: + # Document the DINOv3/Lightly convention: masks are 0-indexed class ids + # with unannotated pixels set to this ignore index. + manifest["ignore_index"] = 255 # Write files AND build the download .zip in one pass. ZIP_STORED: the # PNGs are already compressed, so re-deflating them is wasted CPU. @@ -1028,14 +1034,14 @@ def _cb(message: str, _jid: str = jid) -> None: zf.writestr("manifest.json", manifest_json) if lightly: - # DINOv3 / Lightly: classes.json (index→name, 0=bg) at the dataset - # root; each split as images/ + masks/ with matching stems. + # DINOv3 / Lightly: classes.json (index→name, 0-indexed, no background) + # at the dataset root; each split as images/ + masks/ with matching + # stems. Unannotated pixels are 255 (the ignore index). classes_json = json.dumps(lightly_classes_map(merged_categories), indent=2) (out_root / "classes.json").write_text(classes_json) zf.writestr("classes.json", classes_json.encode("utf-8")) - for split_name, split_data in merged_splits.items(): - # Lightly convention: 'valid' → 'val'; 'train'/'test' unchanged. - dir_name = "val" if split_name == "valid" else split_name + # Lightly uses train/val only: 'valid' AND 'test' fold into 'val'. + for dir_name, split_data in fold_lightly_splits(merged_splits).items(): export_jobs.log(jid, f"Writing split '{dir_name}' ({len(split_data['images'])} images + masks)…") written[dir_name] = write_lightly_split( out_root / dir_name, diff --git a/backend/coco_export.py b/backend/coco_export.py index 75aeb3c..c30d061 100644 --- a/backend/coco_export.py +++ b/backend/coco_export.py @@ -433,13 +433,27 @@ def _emit(rel: str, data: bytes) -> None: return {"n_images": n, "n_annotations": 0, "path": str(img_dir)} +# Lightly/DINOv3 uses train/val only: the app's 'valid' AND 'test' both fold into 'val'. +LIGHTLY_SPLIT_DIRS = {"train": "train", "valid": "val", "test": "val"} + + +def fold_lightly_splits(merged_splits: dict[str, Any]) -> dict[str, dict[str, Any]]: + """Map the app's train/valid/test splits onto Lightly's train/val directories, + merging the image lists of any splits that map to the same directory (valid+test + → val). Returns ``{dir_name: {"images": [...]}}``.""" + folded: dict[str, dict[str, Any]] = {} + for split_name, split_data in merged_splits.items(): + dir_name = LIGHTLY_SPLIT_DIRS.get(split_name, split_name) + folded.setdefault(dir_name, {"images": []})["images"].extend(split_data.get("images", [])) + return folded + + def lightly_classes_map(categories: list[dict[str, Any]]) -> dict[str, str]: - """Build the Lightly ``classes`` mapping (index → name), 0 = background. - Category ids start at 1 and are contiguous (see build_export_plan).""" - out: dict[str, str] = {"0": "background"} - for c in categories: - out[str(int(c["id"]))] = str(c["name"]) - return out + """Build the Lightly/DINOv3 ``classes`` mapping (index → name), 0-indexed and + contiguous with NO background class. Internal category ids are 1-based (see + build_export_plan), so shift by -1; unannotated pixels use 255 (ignore) instead + of a background class.""" + return {str(int(c["id"]) - 1): str(c["name"]) for c in categories} def build_export_plan( @@ -451,6 +465,7 @@ def build_export_plan( sample_global_stats_fn: Any, progress_cb: Any = None, include_polygons: bool = False, + lightly: bool = False, ) -> dict[str, Any]: """Build the full export plan (rasterize all shapes, render PNGs). @@ -516,9 +531,10 @@ def _process_slice(slice_key: str) -> dict[str, Any]: file_name = f"{source_stem}_{slice_idx:04d}.png" anns: list[dict[str, Any]] = [] skipped = 0 - # Semantic label map (class index per pixel, 0 = bg) + per-class binary - # masks, built from the same rasterization used for the COCO annotations. - label = np.zeros((h, w), dtype=np.uint8) + # Semantic label map + per-class binary masks, from the same rasterization + # used for the COCO annotations. COCO: 0 = background, classes 1..N. Lightly/ + # DINOv3: unannotated = 255 (ignore), classes 0-indexed (cat_id - 1). + label = np.full((h, w), 255, dtype=np.uint8) if lightly else np.zeros((h, w), dtype=np.uint8) class_acc: dict[str, np.ndarray] = {} for shape in payload.slices.get(slice_key, []): shape_dict = shape if isinstance(shape, dict) else shape.model_dump() @@ -541,7 +557,8 @@ def _process_slice(slice_key: str) -> dict[str, Any]: ann["_image_file_name"] = file_name anns.append(ann) # Paint label map (last shape wins on overlap) + accumulate per class. - label[mask] = cat_id + # Lightly is 0-indexed (cat_id - 1); COCO keeps the 1-based id. + label[mask] = (cat_id - 1) if lightly else cat_id cname = cat_id_to_name.get(cat_id, str(cat_id)) if cname not in class_acc: class_acc[cname] = np.zeros((h, w), dtype=bool) diff --git a/backend/tests/test_lightly_export.py b/backend/tests/test_lightly_export.py index c317cb4..e7f4602 100644 --- a/backend/tests/test_lightly_export.py +++ b/backend/tests/test_lightly_export.py @@ -44,30 +44,81 @@ def test_write_lightly_split_matching_stems(tmp_path) -> None: assert imgs == masks == ["sample_0000.png", "sample_0001.png"] -def test_write_lightly_split_mask_is_index_labelmap(tmp_path) -> None: - """Mask PNGs are single-channel with pixel value == class index.""" +def test_write_lightly_split_mask_is_single_channel_passthrough(tmp_path) -> None: + """write_lightly_split writes the given label PNG verbatim as a single-channel mask + (the DINOv3 255/0-indexed convention is applied upstream in build_export_plan).""" from coco_export import write_lightly_split - label = np.zeros((16, 16), dtype=np.uint8) - label[4:8, 4:8] = 1 - label[10:14, 10:14] = 2 + label = np.full((16, 16), 255, dtype=np.uint8) + label[4:8, 4:8] = 0 + label[10:14, 10:14] = 1 write_lightly_split(tmp_path / "val", [_mk_image("s_0000.png", label)]) m = PILImage.open(tmp_path / "val" / "masks" / "s_0000.png") assert m.mode == "L" # single-channel integer arr = np.array(m) - assert arr[0, 0] == 0 # background - assert arr[5, 5] == 1 # class 1 - assert arr[12, 12] == 2 # class 2 - assert set(np.unique(arr)).issubset({0, 1, 2}) + assert arr[0, 0] == 255 # unannotated → ignore index + assert arr[5, 5] == 0 # class 0 (0-indexed) + assert arr[12, 12] == 1 # class 1 + assert set(np.unique(arr)).issubset({0, 1, 255}) -def test_lightly_classes_map(tmp_path) -> None: - """classes.json maps index→name with 0=background and contiguous ids.""" +def test_lightly_classes_map() -> None: + """classes.json maps index→name, 0-indexed and contiguous, with NO background.""" from coco_export import lightly_classes_map cats = [{"id": 1, "name": "cell"}, {"id": 2, "name": "wall"}] m = lightly_classes_map(cats) - assert m == {"0": "background", "1": "cell", "2": "wall"} + assert m == {"0": "cell", "1": "wall"} + assert "background" not in m.values() # round-trips as JSON (Lightly accepts a path to this) - assert json.loads(json.dumps(m))["1"] == "cell" + assert json.loads(json.dumps(m))["0"] == "cell" + + +def test_build_export_plan_lightly_labelmap() -> None: + """In lightly mode, build_export_plan's label map is 0-indexed with unannotated=255.""" + import numpy as np + + from coco_export import build_export_plan + from schemas import AnnotationClass, ExportRequest, RenderOpts + + h, w = 40, 40 + # Two classes; a polygon of the SECOND class (classId=7) → internal cat id 2 → pixel 1. + poly = {"id": "s1", "kind": "polygon", "classId": 7, "points": [5, 5, 25, 5, 25, 25, 5, 25]} + payload = ExportRequest( + kind="local", source="vol", server_uri=None, + slices={"0": [poly]}, split_by_slice={"0": "train"}, negative_slices=[], + classes=[AnnotationClass(classId=3, label="cell", color="#00ff00"), + AnnotationClass(classId=7, label="wall", color="#ff0000")], + render=RenderOpts(norm="slice"), + auto_split={"ratios": [1, 0, 0], "seed": 1}, + ) + meta = {"height": h, "width": w, "n_slices": 1} + plan = build_export_plan( + object(), payload, + render_slice_fn=lambda arr, opts, gr: np.zeros((h, w, 3), dtype=np.uint8), + array_shape_meta_fn=lambda n: meta, + read_slice_fn=lambda n, m, i: np.zeros((h, w), dtype=np.uint8), + sample_global_stats_fn=lambda n, m: (0.0, 1.0), + lightly=True, + ) + img = plan["splits"]["train"]["images"][0] + arr = np.array(PILImage.open(io.BytesIO(img["label_png_bytes"]))) + assert arr[0, 0] == 255 # unannotated → ignore + assert arr[15, 15] == 1 # 'wall' is the 2nd class → pixel 1 (0-indexed) + assert set(np.unique(arr)).issubset({0, 1, 255}) + + +def test_fold_lightly_splits_merges_valid_and_test() -> None: + """valid + test fold into a single 'val' bucket; train stays; images concatenated.""" + from coco_export import fold_lightly_splits + + merged = { + "train": {"images": [{"file_name": "a"}]}, + "valid": {"images": [{"file_name": "b"}]}, + "test": {"images": [{"file_name": "c"}]}, + } + folded = fold_lightly_splits(merged) + assert set(folded) == {"train", "val"} + assert [i["file_name"] for i in folded["train"]["images"]] == ["a"] + assert sorted(i["file_name"] for i in folded["val"]["images"]) == ["b", "c"] From 0af7d47538065fb7bf5a14f7bd1a7fc76f03b8fd Mon Sep 17 00:00:00 2001 From: David Abramov Date: Thu, 30 Jul 2026 14:23:06 -0700 Subject: [PATCH 2/2] Fixing bug where if the same dataset was attempted to be ingested a second time, it would throw a cryptic 409 error. Now a dialogue appears with a few options --- backend/annotation_server.py | 30 +- backend/ingest.py | 165 ++++++++++- backend/schemas.py | 19 ++ backend/tests/test_ingest_conflict.py | 274 +++++++++++++++++ backend/tests/test_schemas.py | 25 +- frontend/src/app/pages/BrowsePage.tsx | 14 +- frontend/src/app/pages/ConnectPage.tsx | 26 +- .../src/components/Browse/ColumnBrowser.tsx | 19 ++ .../src/components/Browse/ItemsColumn.tsx | 34 ++- .../src/components/Ingest/ConflictDialog.tsx | 170 +++++++++++ .../src/components/Ingest/IngestDropzone.tsx | 280 +++++++++++++++--- .../components/Ingest/ingestErrors.test.ts | 67 +++++ .../src/components/Ingest/ingestErrors.ts | 102 +++++++ frontend/src/lib/sourceKey.test.ts | 37 +++ frontend/src/lib/sourceKey.ts | 33 +++ frontend/src/stores/connectionStore.ts | 22 +- 16 files changed, 1251 insertions(+), 66 deletions(-) create mode 100644 backend/tests/test_ingest_conflict.py create mode 100644 frontend/src/components/Ingest/ConflictDialog.tsx create mode 100644 frontend/src/components/Ingest/ingestErrors.test.ts create mode 100644 frontend/src/components/Ingest/ingestErrors.ts create mode 100644 frontend/src/lib/sourceKey.test.ts diff --git a/backend/annotation_server.py b/backend/annotation_server.py index b177737..66591a7 100644 --- a/backend/annotation_server.py +++ b/backend/annotation_server.py @@ -67,6 +67,7 @@ ExportSourceItem, GuidePayload, ImageMeta, + IngestPreflightRequest, MeasureRequest, SaveVersionRequest, ) @@ -1161,11 +1162,33 @@ def _run() -> dict: raise HTTPException(500, f"Import failed: {exc}") from exc +@app.post("/api/ingest/preflight") +async def ingest_preflight(req: IngestPreflightRequest) -> dict: + """Report which of ``req.names`` already exist in ``req.container_path``. + + Called before the upload so the user can resolve collisions (replace / skip / + new dataset / browse the existing one) instead of uploading files that would + each fail with a 409. POST (not GET) because a dropped folder easily carries + hundreds of filenames — see :class:`schemas.IngestPreflightRequest`. + + Raises: + HTTPException: 502 if the Tiled server could not be read. + """ + try: + return await asyncio.to_thread( + ingest_mod.preflight, req.server_uri, req.container_path, req.names + ) + except Exception as exc: + logger.warning("ingest preflight failed for %s: %s", req.container_path, exc) + raise HTTPException(502, "Could not check the destination on the Tiled server") from exc + + @app.post("/api/ingest/upload") async def ingest_upload( server_uri: Optional[str] = Query(None, description="Target Tiled server URI"), container_path: str = Form(..., description="Target container, e.g. 'browse/myset'"), description: str = Form("", description="Optional keyword(s) stored on every ingested node"), + on_conflict: str = Form("fail", description="'fail', 'replace' or 'skip' for existing keys"), files: list[UploadFile] = File(..., description="Image files to copy into Tiled"), ) -> dict: """Stream uploaded files to temp storage and start a background ingest job. @@ -1173,6 +1196,11 @@ async def ingest_upload( Each supported image becomes its own browsable node in *container_path* on the connected Tiled server. Returns a ``job_id`` to poll for progress. """ + if on_conflict not in ingest_mod.ON_CONFLICT_MODES: + raise HTTPException( + 400, f"on_conflict must be one of {sorted(ingest_mod.ON_CONFLICT_MODES)}" + ) + tmp_dir = Path(tempfile.mkdtemp(prefix="ingest_")) saved: list[tuple[str, Path]] = [] for index, upload in enumerate(files): @@ -1195,7 +1223,7 @@ async def ingest_upload( jid = ingest_mod.new_job(len(saved), server_uri, container_path) threading.Thread( target=ingest_mod.run_ingest_job, - args=(jid, server_uri, container_path, saved, description), + args=(jid, server_uri, container_path, saved, description, on_conflict), daemon=True, ).start() return {"job_id": jid, "total": len(saved), "container_path": container_path} diff --git a/backend/ingest.py b/backend/ingest.py index 3609e57..efc4777 100644 --- a/backend/ingest.py +++ b/backend/ingest.py @@ -46,8 +46,16 @@ # the frame index. _FRAME_RE = re.compile(r"(\d+)$") _MIN_PAD = 5 +# Leading HTTP status in a Tiled ClientError message ("409: "). +_STATUS_RE = re.compile(r"^\s*(\d{3}):") +_URL_RE = re.compile(r"https?://\S+") +# Trailing "_N" on a container name, so suggestions bump instead of stacking. +_SUFFIX_RE = re.compile(r"^(.*?)_(\d+)$") -# job_id -> {state, total, done, failed, errors[], container_path, server_uri} +# What to do when a target node key already exists (see ``run_ingest_job``). +ON_CONFLICT_MODES: frozenset[str] = frozenset({"fail", "replace", "skip"}) + +# job_id -> {state, total, done, failed, skipped, errors[], container_path, server_uri} _jobs: dict[str, dict[str, Any]] = {} _jobs_lock = threading.Lock() @@ -61,6 +69,7 @@ def new_job(total: int, server_uri: str | None, container_path: str) -> str: "total": total, "done": 0, "failed": 0, + "skipped": 0, "errors": [], "container_path": container_path, "server_uri": server_uri, @@ -81,13 +90,21 @@ def _update(jid: str, **kw: Any) -> None: _jobs[jid].update(kw) -def _bump(jid: str, *, done: int = 0, failed: int = 0, error: str | None = None) -> None: +def _bump( + jid: str, + *, + done: int = 0, + failed: int = 0, + skipped: int = 0, + error: dict[str, str] | None = None, +) -> None: with _jobs_lock: job = _jobs.get(jid) if not job: return job["done"] += done job["failed"] += failed + job["skipped"] += skipped if error: job["errors"].append(error) @@ -108,16 +125,124 @@ def _read_array(path: Path) -> np.ndarray: def _ensure_container(client: Any, parts: list[str]) -> Any: - """Navigate to ``client[parts...]``, creating containers as needed.""" + """Navigate to ``client[parts...]``, creating containers as needed. + + Only a genuine ``KeyError`` means "missing" — catching every exception here + would send a transient transport blip down the ``create_container`` path, + which then collides with the container that does in fact exist. + """ node = client for key in parts: try: node = node[key] - except Exception: # noqa: BLE001 — KeyError or transport error → create + except KeyError: node = node.create_container(key=key, metadata={}) return node +def _walk(client: Any, parts: list[str]) -> Any | None: + """Return the node at ``parts``, or ``None`` if any segment is missing.""" + node = client + for key in parts: + try: + node = node[key] + except KeyError: + return None + return node + + +def _child_keys(node: Any) -> set[str]: + """Return a node's child keys; empty for leaves (arrays have no children).""" + if node is None: + return set() + try: + return set(node.keys()) + except Exception: # noqa: BLE001 — leaf/array node + return set() + + +def _classify_error(exc: BaseException) -> dict[str, str]: + """Map an ingest exception to a ``{kind, message}`` pair for the UI. + + ``kind`` is a stable token the frontend turns into human copy. The message + is a short fallback with any server URL stripped — Tiled's ``ClientError`` + text is ``": "``, which must not + reach the browser. The full raw text is logged by the caller instead. + + Args: + exc: The exception raised while ingesting one file. + + Returns: + ``{"kind": ..., "message": ...}`` with kind in ``conflict``, + ``unreadable``, ``unreachable``, ``auth`` or ``unknown``. + """ + raw = str(exc) or type(exc).__name__ + name = type(exc).__name__ + match = _STATUS_RE.match(raw) + status = match.group(1) if match else None + + if status == "409" or "Collision" in raw or "Conflict" in name: + return {"kind": "conflict", "message": "a sample with this name already exists"} + if status in ("401", "403"): + return {"kind": "auth", "message": "not authorized to write to this server"} + if "Connect" in name or "Timeout" in name or "Connection" in raw: + return {"kind": "unreachable", "message": "could not reach the Tiled server"} + if isinstance(exc, (ValueError, OSError)): + return {"kind": "unreadable", "message": "file could not be read as an image"} + return {"kind": "unknown", "message": _URL_RE.sub("", raw).strip()[:200]} + + +def _suggest_container_path(client: Any, parts: list[str]) -> str: + """Return ``parts`` with a ``_N`` suffix that is unused among its siblings. + + An existing numeric suffix is bumped rather than appended to, so repeated + suggestions give ``myset_2``, ``myset_3`` — never ``myset_2_2``. + """ + if not parts: + return "" + siblings = _child_keys(_walk(client, parts[:-1])) + match = _SUFFIX_RE.match(parts[-1]) + stem, index = (match.group(1), int(match.group(2)) + 1) if match else (parts[-1], 2) + while f"{stem}_{index}" in siblings: + index += 1 + return "/".join([*parts[:-1], f"{stem}_{index}"]) + + +def preflight( + server_uri: str | None, container_path: str, filenames: list[str] +) -> dict[str, Any]: + """Report which uploads would collide with nodes already in the container. + + Called before any bytes are uploaded so the user can choose how to resolve + the collision (replace / skip / new dataset / just browse the existing one) + instead of watching a large upload fail with a 409 per file. + + Args: + server_uri: Connected Tiled server URI. + container_path: Slash-separated target container (e.g. ``browse/testset``). + filenames: Original filenames the user is about to upload. + + Returns: + ``{container_exists, existing_count, conflicts, suggested_container_path}`` + where each conflict is ``{"filename", "key"}``. + """ + api_key = api_key_for_uri(server_uri) + client = get_tiled_client(server_uri, api_key) + parts = [p for p in container_path.strip("/").split("/") if p] + node = _walk(client, parts) + existing = _child_keys(node) + return { + "container_exists": node is not None, + "existing_count": len(existing), + "conflicts": [ + {"filename": name, "key": Path(name).stem} + for name in filenames + if Path(name).stem in existing + ], + "suggested_container_path": _suggest_container_path(client, parts), + } + + def _size_str(arr: np.ndarray) -> str: """Return pixel dimensions as ``"H x W"`` (first two dims).""" return " x ".join(str(d) for d in arr.shape[:2]) @@ -173,6 +298,7 @@ def run_ingest_job( container_path: str, temp_files: list[tuple[str, Path]], description: str = "", + on_conflict: str = "fail", ) -> None: """Copy each temp file into the target Tiled container. @@ -183,9 +309,15 @@ def run_ingest_job( temp_files: list of ``(original_filename, temp_path)``. description: Optional user-supplied keyword(s) stored on every node so the batch is identifiable/filterable in Browse (empty string → omitted). + on_conflict: What to do when the node key already exists — ``"fail"`` + (record a ``conflict`` error), ``"replace"`` (delete then rewrite) or + ``"skip"`` (leave the existing node, count it as skipped). The user + picks this in the conflict dialog after :func:`preflight`. """ description = (description or "").strip() keywords = parse_keywords(description) + if on_conflict not in ON_CONFLICT_MODES: + on_conflict = "fail" _update(jid, state="running") try: api_key = api_key_for_uri(server_uri) @@ -213,10 +345,22 @@ def run_ingest_job( except Exception as exc: # noqa: BLE001 — best-effort; per-array meta still set logger.warning("could not set container metadata on %s: %s", container_path, exc) + # Only needed to resolve collisions; a container we just created is empty. + existing_keys = _child_keys(target) if on_conflict != "fail" else set() + for idx, (orig_name, tmp) in enumerate(temp_files): try: - arr = _read_array(tmp) stem = Path(orig_name).stem + if stem and stem in existing_keys: + if on_conflict == "skip": + _bump(jid, skipped=1) + continue + # "replace": drop the existing child so write_array can't + # collide. Must be delete_contents(key) — Container.delete() + # deletes the container ITSELF. external_only=False because + # we wrote these arrays into Tiled's own storage. + target.delete_contents(stem, recursive=True, external_only=False) + arr = _read_array(tmp) meta = { "image_number": _image_number(stem, width, idx), "size": _size_str(arr), @@ -236,17 +380,22 @@ def run_ingest_job( target.write_array(arr, key=stem, metadata=meta, dims=dims) _bump(jid, done=1) except Exception as exc: # noqa: BLE001 — isolate per-file failures + # Log the raw text (URLs and all); send only classified copy out. logger.warning("ingest %s failed: %s", orig_name, exc) - _bump(jid, failed=1, error=f"{orig_name}: {exc}") + _bump(jid, failed=1, error={"filename": orig_name, **_classify_error(exc)}) finally: try: tmp.unlink(missing_ok=True) except Exception: # noqa: BLE001 pass - _update(jid, state="done") + # A batch where nothing landed is a failure, not a success — reporting + # "done" there is what made the UI show a green "Ingested 0 of 3". + job = get_job(jid) or {} + wholly_failed = job.get("done", 0) == 0 and job.get("failed", 0) > 0 + _update(jid, state="error" if wholly_failed else "done") except Exception as exc: # noqa: BLE001 — fatal (e.g. cannot reach server) logger.error("ingest job %s fatal: %s", jid, exc) - _bump(jid, error=str(exc)) + _bump(jid, error={"filename": "", **_classify_error(exc)}) _update(jid, state="error") finally: for _, tmp in temp_files: diff --git a/backend/schemas.py b/backend/schemas.py index a41d403..eaf50ea 100644 --- a/backend/schemas.py +++ b/backend/schemas.py @@ -262,6 +262,25 @@ class MeasureRequest(BaseModel): shapes: list[dict[str, Any]] = Field(default_factory=list) +class IngestPreflightRequest(BaseModel): + """Request body for the pre-upload duplicate check. + + Sent as a POST body rather than query params because a dropped folder can + hold hundreds of filenames — as a query string that exceeds the HTTP + header size limit and the request is rejected with 431 before routing. + + Attributes: + container_path: Target container, e.g. ``browse/myset``. + names: Original filenames about to be uploaded (may be empty to only + ask for a suggested free container name). + server_uri: Target Tiled server URI; ``None`` uses the default server. + """ + + container_path: str + names: list[str] = Field(default_factory=list) + server_uri: str | None = None + + class GuideClass(BaseModel): """One class entry in an annotation guide. diff --git a/backend/tests/test_ingest_conflict.py b/backend/tests/test_ingest_conflict.py new file mode 100644 index 0000000..60e0ed0 --- /dev/null +++ b/backend/tests/test_ingest_conflict.py @@ -0,0 +1,274 @@ +"""Tests for duplicate-upload handling in the ingest flow (issue #8). + +Covers ``preflight`` (what already exists at the destination), the three +``on_conflict`` modes of ``run_ingest_job``, and the error classifier that keeps +raw Tiled URLs out of user-facing messages. +""" + +from __future__ import annotations + +from pathlib import Path + +import numpy as np +import pytest + +import ingest + + +class FakeNode: + """Minimal stand-in for a Tiled container client.""" + + def __init__(self, children: dict | None = None) -> None: + self._children: dict = dict(children or {}) + self.written: dict = {} + self.deleted: list[str] = [] + self.metadata_updates: list[dict] = [] + + def __getitem__(self, key: str): + if key not in self._children: + raise KeyError(key) + return self._children[key] + + def keys(self) -> list[str]: + return list(self._children) + + def create_container(self, key: str, metadata: dict | None = None) -> "FakeNode": + node = FakeNode() + self._children[key] = node + return node + + def update_metadata(self, metadata: dict | None = None) -> None: + self.metadata_updates.append(metadata or {}) + + def write_array(self, arr, key=None, metadata=None, dims=None) -> None: + if key in self._children: + # Mirrors tiled's ClientError text: ": ". + raise RuntimeError( + f"409: /browse/ds/{key} http://127.0.0.1:8010/api/v1/metadata/browse/ds" + ) + self._children[key] = "array" + self.written[key] = (arr, metadata) + + def delete_contents( + self, keys=None, recursive: bool = False, external_only: bool = True + ) -> None: + # Guard against the real API's footgun: keys=None wipes the container. + assert keys, "delete_contents(None) would delete every sample" + assert external_only is False, "internally-managed arrays need external_only=False" + for key in [keys] if isinstance(keys, str) else keys: + self.deleted.append(key) + self._children.pop(key, None) + + def delete(self, recursive: bool = False, external_only: bool = True) -> None: + raise AssertionError("Container.delete() deletes the container itself — never call it") + + +class FakeArray: + """Leaf node: arrays have no children, so keys() raises.""" + + def keys(self): + raise AttributeError("arrays have no children") + + +@pytest.fixture +def fake_client(monkeypatch): + """Root client with ``browse/ds`` holding two samples.""" + ds = FakeNode({"img_00001": "array", "img_00002": "array"}) + root = FakeNode({"browse": FakeNode({"ds": ds})}) + monkeypatch.setattr(ingest, "api_key_for_uri", lambda uri: None) + monkeypatch.setattr(ingest, "get_tiled_client", lambda uri, key=None: root) + return root, ds + + +def _npy(tmp_path: Path, name: str) -> tuple[str, Path]: + """Write a tiny .npy temp file and return the (original_name, path) pair.""" + path = tmp_path / f"{name}.npy" + np.save(path, np.zeros((4, 4), dtype=np.uint8)) + return (f"{name}.npy", path) + + +def _run(jid: str, temp_files, on_conflict: str) -> dict: + ingest.run_ingest_job(jid, None, "browse/ds", temp_files, "", on_conflict) + return ingest.get_job(jid) + + +# --- preflight --------------------------------------------------------------- + + +def test_preflight_reports_overlapping_keys(fake_client) -> None: + result = ingest.preflight(None, "browse/ds", ["img_00002.tif", "img_00009.tif"]) + + assert result["container_exists"] is True + assert result["existing_count"] == 2 + assert result["conflicts"] == [{"filename": "img_00002.tif", "key": "img_00002"}] + assert result["suggested_container_path"] == "browse/ds_2" + + +def test_preflight_missing_container_has_no_conflicts(fake_client) -> None: + result = ingest.preflight(None, "browse/brand_new", ["img_00001.tif"]) + + assert result["container_exists"] is False + assert result["existing_count"] == 0 + assert result["conflicts"] == [] + + +def test_preflight_tolerates_leaf_destination(fake_client, monkeypatch) -> None: + """Pointing at an array (not a container) must not blow up.""" + root, _ = fake_client + root["browse"]._children["leaf"] = FakeArray() + + result = ingest.preflight(None, "browse/leaf", ["img_00001.tif"]) + + assert result["existing_count"] == 0 + assert result["conflicts"] == [] + + +def test_suggested_path_skips_taken_names_and_bumps_suffix(fake_client) -> None: + root, _ = fake_client + browse = root["browse"] + browse._children["ds_2"] = FakeNode() + browse._children["ds_3"] = FakeNode() + + assert ingest.preflight(None, "browse/ds", [])["suggested_container_path"] == "browse/ds_4" + # An existing numeric suffix is bumped, not stacked (never "ds_2_2"). + assert ingest.preflight(None, "browse/ds_2", [])["suggested_container_path"] == "browse/ds_4" + + +# --- on_conflict modes ------------------------------------------------------- + + +def test_skip_leaves_existing_nodes_untouched(fake_client, tmp_path) -> None: + _, ds = fake_client + files = [_npy(tmp_path, "img_00002"), _npy(tmp_path, "img_00009")] + jid = ingest.new_job(len(files), None, "browse/ds") + + job = _run(jid, files, "skip") + + assert (job["done"], job["skipped"], job["failed"]) == (1, 1, 0) + assert job["state"] == "done" + assert job["errors"] == [] + assert ds.deleted == [] + assert list(ds.written) == ["img_00009"] + + +def test_replace_deletes_then_rewrites(fake_client, tmp_path) -> None: + _, ds = fake_client + files = [_npy(tmp_path, "img_00002"), _npy(tmp_path, "img_00009")] + jid = ingest.new_job(len(files), None, "browse/ds") + + job = _run(jid, files, "replace") + + assert (job["done"], job["skipped"], job["failed"]) == (2, 0, 0) + assert ds.deleted == ["img_00002"] + assert sorted(ds.written) == ["img_00002", "img_00009"] + + +def test_fail_mode_classifies_the_conflict_and_hides_the_url(fake_client, tmp_path) -> None: + files = [_npy(tmp_path, "img_00001"), _npy(tmp_path, "img_00002")] + jid = ingest.new_job(len(files), None, "browse/ds") + + job = _run(jid, files, "fail") + + assert (job["done"], job["failed"]) == (0, 2) + # Nothing landed → 'error', not a green 'done' (issue #8's misleading status). + assert job["state"] == "error" + assert [e["kind"] for e in job["errors"]] == ["conflict", "conflict"] + assert job["errors"][0]["filename"] == "img_00001.npy" + assert all("http" not in e["message"] for e in job["errors"]) + + +def test_partial_success_still_reports_done(fake_client, tmp_path) -> None: + files = [_npy(tmp_path, "img_00001"), _npy(tmp_path, "img_00009")] + jid = ingest.new_job(len(files), None, "browse/ds") + + job = _run(jid, files, "fail") + + assert (job["done"], job["failed"]) == (1, 1) + assert job["state"] == "done" + + +def test_unknown_mode_falls_back_to_fail(fake_client, tmp_path) -> None: + _, ds = fake_client + files = [_npy(tmp_path, "img_00002")] + jid = ingest.new_job(len(files), None, "browse/ds") + + job = _run(jid, files, "obliterate") + + assert job["failed"] == 1 + assert ds.deleted == [] + + +def test_temp_files_are_cleaned_up_even_when_skipped(fake_client, tmp_path) -> None: + files = [_npy(tmp_path, "img_00002"), _npy(tmp_path, "img_00009")] + jid = ingest.new_job(len(files), None, "browse/ds") + + _run(jid, files, "skip") + + assert not any(path.exists() for _, path in files) + + +# --- error classification ---------------------------------------------------- + + +@pytest.mark.parametrize( + "exc, kind", + [ + (RuntimeError("409: /browse/ds/img http://127.0.0.1:8010/api/v1/metadata/browse/ds"), "conflict"), + (RuntimeError("401: unauthorized http://127.0.0.1:8010/api/v1/metadata"), "auth"), + (RuntimeError("403: forbidden http://127.0.0.1:8010/api/v1/metadata"), "auth"), + (ValueError("unsupported extension '.bin'"), "unreadable"), + (OSError("truncated file"), "unreadable"), + (RuntimeError("something odd happened"), "unknown"), + ], +) +def test_classify_error_kinds(exc: Exception, kind: str) -> None: + assert ingest._classify_error(exc)["kind"] == kind + + +def test_classify_error_recognizes_connection_failures() -> None: + class ConnectError(Exception): + pass + + assert ingest._classify_error(ConnectError("nope"))["kind"] == "unreachable" + + +def test_classify_error_strips_urls_from_unknown_messages() -> None: + exc = RuntimeError("weird failure at http://127.0.0.1:8010/api/v1/metadata/browse/ds") + message = ingest._classify_error(exc)["message"] + + assert "http" not in message + assert message.startswith("weird failure at") + + +def test_filename_digits_are_not_mistaken_for_a_status_code() -> None: + """'409' inside a message body must not be read as a 409 status.""" + result = ingest._classify_error(RuntimeError("could not read IMG_409.tif")) + + assert result["kind"] == "unknown" + + +# --- container navigation ---------------------------------------------------- + + +def test_ensure_container_reuses_existing_nodes(fake_client) -> None: + root, ds = fake_client + + assert ingest._ensure_container(root, ["browse", "ds"]) is ds + + +def test_ensure_container_creates_missing_nodes(fake_client) -> None: + root, _ = fake_client + node = ingest._ensure_container(root, ["browse", "fresh"]) + + assert node is root["browse"]["fresh"] + + +def test_ensure_container_propagates_transport_errors(fake_client) -> None: + """A transport blip must NOT be misread as 'missing' → create → 409.""" + + class Flaky(FakeNode): + def __getitem__(self, key): + raise RuntimeError("connection reset") + + with pytest.raises(RuntimeError): + ingest._ensure_container(Flaky(), ["browse"]) diff --git a/backend/tests/test_schemas.py b/backend/tests/test_schemas.py index 8dbf913..ea9e396 100644 --- a/backend/tests/test_schemas.py +++ b/backend/tests/test_schemas.py @@ -2,7 +2,15 @@ from __future__ import annotations -from schemas import BrushShape, BrushStroke, EllipseShape, PolygonShape, RectShape, RenderOpts +from schemas import ( + BrushShape, + BrushStroke, + EllipseShape, + IngestPreflightRequest, + PolygonShape, + RectShape, + RenderOpts, +) def test_polygon_shape_roundtrip() -> None: @@ -52,3 +60,18 @@ def test_brush_stroke_erase_mode() -> None: """BrushStroke should accept 'erase' mode.""" stroke = BrushStroke(points=[5.0, 5.0], radius=3.0, mode="erase") assert stroke.mode == "erase" + + +def test_ingest_preflight_request_carries_a_whole_folder() -> None: + """A folder's worth of filenames must fit — as query params it 431'd.""" + names = [f"20260221_135217_petiole22_{i:05d}.tiff" for i in range(690)] + req = IngestPreflightRequest(container_path="browse/ds", names=names, server_uri=None) + assert len(req.names) == 690 + assert req.names[0].endswith("_00000.tiff") + + +def test_ingest_preflight_request_names_default_to_empty() -> None: + """Omitting names is allowed — used to only ask for a free container name.""" + req = IngestPreflightRequest(container_path="browse/ds") + assert req.names == [] + assert req.server_uri is None diff --git a/frontend/src/app/pages/BrowsePage.tsx b/frontend/src/app/pages/BrowsePage.tsx index eb44549..81b2d9d 100644 --- a/frontend/src/app/pages/BrowsePage.tsx +++ b/frontend/src/app/pages/BrowsePage.tsx @@ -16,8 +16,17 @@ import type { AnnotationFilter } from '@/types/annotationFilter'; export default function BrowsePage() { const navigate = useNavigate(); - const { kind, serverUri, browseContainerPath, localRoot, localRel, label, sampleCount, setConnection } = - useConnectionStore(); + const { + kind, + serverUri, + browseContainerPath, + browseFocusPath, + localRoot, + localRel, + label, + sampleCount, + setConnection, + } = useConnectionStore(); const { openLocalFile } = useOpenInAnnotate(); const [annotationFilter, setAnnotationFilter] = useState('all'); @@ -87,6 +96,7 @@ export default function BrowsePage() { key={`${serverUri}:${browseContainerPath ?? ''}`} serverUri={serverUri} containerPath={browseContainerPath} + focusPath={browseFocusPath} servers={servers} selectedServerUri={serverUri} onServerChange={handleServerChange} diff --git a/frontend/src/app/pages/ConnectPage.tsx b/frontend/src/app/pages/ConnectPage.tsx index 92407fb..07e0abb 100644 --- a/frontend/src/app/pages/ConnectPage.tsx +++ b/frontend/src/app/pages/ConnectPage.tsx @@ -115,18 +115,38 @@ export default function ConnectPage() { const canConnect = mode === 'tiled' ? !!selectedServerUri : !!grantedRoot && !!selectedFolder; - /** Set the Tiled connection (optional browse container) and, by default, navigate to Browse. */ - const connectTiled = (containerPath: string | null, gotoBrowse = true) => { + /** + * Set the Tiled connection (optional browse container) and, by default, navigate to Browse. + * + * @param focusPath Sample to auto-select on arrival, when browsing from the root. + */ + const connectTiled = ( + containerPath: string | null, + gotoBrowse = true, + focusPath: string | null = null, + ) => { setConnection({ kind: 'tiled', serverUri: selectedServerUri, browseContainerPath: containerPath, + browseFocusPath: focusPath, label: servers.find((s) => s.uri === selectedServerUri)?.name ?? selectedServerUri, sampleCount: 0, }); if (gotoBrowse) navigate('/browse'); }; + /** + * Open an ingested dataset in Browse. + * + * Deliberately browses from the ROOT, not the dataset container: scoped into + * the container each row would be a single array slice, whose source key does + * not match the volume-level key annotations are stored under — so a dataset + * with existing annotations would look untouched. From the root the dataset is + * one drillable sample; `focusPath` selects it so the user still lands on it. + */ + const browseIngested = (containerPath: string) => connectTiled(null, true, containerPath); + // Jump straight from ingest to the Annotate tab for the first uploaded sample. const annotateIngested = (containerPath: string, firstKey: string) => { connectTiled(containerPath, false); // set connection context, don't navigate to Browse @@ -349,7 +369,7 @@ export default function ConnectPage() { {selectedServerUri ? ( connectTiled(containerPath)} + onBrowse={browseIngested} onAnnotate={annotateIngested} /> ) : ( diff --git a/frontend/src/components/Browse/ColumnBrowser.tsx b/frontend/src/components/Browse/ColumnBrowser.tsx index 9b3e2b1..9e6f315 100644 --- a/frontend/src/components/Browse/ColumnBrowser.tsx +++ b/frontend/src/components/Browse/ColumnBrowser.tsx @@ -13,6 +13,8 @@ import { ANNOTATION_FILTER_OPTIONS, type AnnotationFilter } from '@/types/annota interface ColumnBrowserProps { serverUri: string; containerPath?: string | null; + /** Path of a sample to auto-select on arrival (e.g. jumping in from ingest). */ + focusPath?: string | null; servers: ServerInfo[]; selectedServerUri: string; onServerChange: (uri: string) => void; @@ -61,6 +63,7 @@ function formatColumnValue(field: string, value: string): string { export default function ColumnBrowser({ serverUri, containerPath, + focusPath, servers, selectedServerUri, onServerChange, @@ -180,6 +183,22 @@ export default function ColumnBrowser({ [actions, state.expandedSample], ); + /** + * Select the sample the caller pointed us at, once the full sample list has + * loaded. Lets "Browse this dataset" land on a specific dataset while Browse + * still lists from the root — where a sample is the whole volume and its + * annotations resolve. Runs at most once per focusPath. + */ + const focusedPath = useRef(null); + useEffect(() => { + if (!focusPath || focusedPath.current === focusPath) return; + if (!state.showingAll || state.itemsLoading) return; + focusedPath.current = focusPath; + const match = state.items.find((it) => it.path === focusPath); + // Not in this listing (different container, or filtered out) — leave as-is. + if (match) handleSelectItem(match); + }, [focusPath, state.showingAll, state.itemsLoading, state.items, handleSelectItem]); + const activeFilters = useMemo(() => { const out: Record = {}; state.columns.forEach((col) => { diff --git a/frontend/src/components/Browse/ItemsColumn.tsx b/frontend/src/components/Browse/ItemsColumn.tsx index be84380..dc4010d 100644 --- a/frontend/src/components/Browse/ItemsColumn.tsx +++ b/frontend/src/components/Browse/ItemsColumn.tsx @@ -1,9 +1,9 @@ -import { useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; import { File, PencilSimple, PencilLine, CaretRight, Stack } from '@phosphor-icons/react'; import StarRating from './StarRating'; import { useRatingStore, type StarRating as StarRatingValue } from '@/stores/ratingStore'; import { useAnnotatedSourceKeys } from '@/hooks/useAnnotatedSourceKeys'; -import { buildSourceKey } from '@/lib/sourceKey'; +import { buildSourceKey, isAnnotatedPath } from '@/lib/sourceKey'; import type { AnnotationFilter } from '@/types/annotationFilter'; import type { BrowseItem } from './hooks/useBrowseData'; @@ -40,7 +40,7 @@ export default function ItemsColumn({ const filtered = items.filter((item) => { const sk = buildSourceKey('tiled', item.path, serverUri); - const isAnnotated = itemIsAnnotated(item, sk, annotatedKeys); + const isAnnotated = itemIsAnnotated(item, serverUri, annotatedKeys); if (annotationFilter === 'annotated' && !isAnnotated) return false; if (annotationFilter === 'unannotated' && isAnnotated) return false; @@ -124,13 +124,20 @@ interface ItemRowProps { serverUri: string; } -/** True if annotated in-session, in drafts, or synced to Tiled metadata. */ +/** + * True if annotated in-session, in drafts, or synced to Tiled metadata. + * + * The draft lookup is path-tolerant (see `isAnnotatedPath`): a volume counts as + * annotated when one of its arrays was annotated under its own key, which is + * what "Annotate first image" produces — otherwise the dataset row looks + * untouched. + */ function itemIsAnnotated( item: BrowseItem, - sourceKey: string, + serverUri: string, annotatedKeys: Set, ): boolean { - if (annotatedKeys.has(sourceKey)) return true; + if (isAnnotatedPath(annotatedKeys, item.path, serverUri)) return true; const flag = item.metadata?.studio_annotated; return flag === 'yes' || flag === true; } @@ -143,15 +150,26 @@ function ItemRow({ item, isSelected, isExpanded, onSelect, onOpenInAnnotate, ser const setRating = useRatingStore((s) => s.setRating); const annotatedKeys = useAnnotatedSourceKeys(); - const isAnnotated = itemIsAnnotated(item, sourceKey, annotatedKeys); + const isAnnotated = itemIsAnnotated(item, serverUri, annotatedKeys); const sliceCount = item.n_slices ?? 1; const isVolume = sliceCount > 1; const background = isSelected || isExpanded ? 'bg-blue-700' : 'bg-transparent hover:bg-slate-700'; const lit = isSelected || isExpanded; + // Keep the active row on screen. Matters when the selection is made for the + // user (arriving from ingest with a focus path) rather than by clicking; + // 'nearest' makes it a no-op for a row that is already visible. + const rowRef = useRef(null); + useEffect(() => { + if (isSelected || isExpanded) rowRef.current?.scrollIntoView({ block: 'nearest' }); + }, [isSelected, isExpanded]); + return ( -
+
{/* Selectable area is a role="button" div, not a +
+ +
+

+ {conflicts.length} of {totalFiles} image{totalFiles === 1 ? '' : 's'}{' '} + {conflicts.length === 1 ? 'is' : 'are'} already in{' '} + {containerPath} + {existingCount > 0 && ( + + {' '} + ({existingCount} sample{existingCount === 1 ? '' : 's'} there now) + + )} + . +

+ +
    + {conflicts.slice(0, MAX_LISTED).map((c) => ( +
  • {c.filename}
    • + ))} + {conflicts.length > MAX_LISTED && ( +
  • + …and {conflicts.length - MAX_LISTED} more +
    • + )} +
+ +
+ + + + + + + +
+ + +
+
+ + ); +} diff --git a/frontend/src/components/Ingest/IngestDropzone.tsx b/frontend/src/components/Ingest/IngestDropzone.tsx index ec2a525..718d217 100644 --- a/frontend/src/components/Ingest/IngestDropzone.tsx +++ b/frontend/src/components/Ingest/IngestDropzone.tsx @@ -8,6 +8,8 @@ import { useCallback, useEffect, useRef, useState } from 'react'; import { UploadSimple, Warning, CheckCircle, CaretRight, CaretDown } from '@phosphor-icons/react'; import { API_BASE } from '@/config'; +import ConflictDialog, { type IngestConflict } from './ConflictDialog'; +import { summarizeIngestErrors, type IngestError } from './ingestErrors'; const SUPPORTED_EXTS = ['tif', 'tiff', 'npy', 'png', 'jpg', 'jpeg']; @@ -16,10 +18,30 @@ interface JobStatus { total: number; done: number; failed: number; - errors: string[]; + /** Duplicates left untouched because the user chose "skip". */ + skipped: number; + errors: IngestError[]; container_path: string; } +/** Response of GET /api/ingest/preflight — what already exists at the destination. */ +interface Preflight { + container_exists: boolean; + existing_count: number; + conflicts: IngestConflict[]; + suggested_container_path: string; +} + +/** An upload held back while the user resolves a collision in ConflictDialog. */ +interface PendingUpload { + files: File[]; + target: string; + preflight: Preflight; +} + +/** How the backend should treat node keys that already exist. */ +type OnConflict = 'fail' | 'replace' | 'skip'; + interface IngestDropzoneProps { serverUri: string; /** Called when the user wants to browse the freshly-ingested container. */ @@ -95,8 +117,17 @@ export default function IngestDropzone({ serverUri, onBrowse, onAnnotate }: Inge const [status, setStatus] = useState(null); const [error, setError] = useState(null); const [uploading, setUploading] = useState(false); + // True while the pre-flight duplicate check is in flight (before any upload). + const [checking, setChecking] = useState(false); // Node key of the first ingested sample (for "Open in Annotate"). const [firstKey, setFirstKey] = useState(null); + // Set when duplicates are found; renders the ConflictDialog. + const [pending, setPending] = useState(null); + // Files of the in-flight/just-finished batch, kept so the post-upload conflict + // backstop can re-upload them without the user re-dropping the folder. + const [lastBatch, setLastBatch] = useState<{ files: File[]; target: string } | null>(null); + // Indices of error groups whose filenames are expanded. + const [expandedErrors, setExpandedErrors] = useState>(new Set()); const pollRef = useRef(null); /** Clear the active status-polling interval, if any. */ @@ -130,10 +161,78 @@ export default function IngestDropzone({ serverUri, onBrowse, onAnnotate }: Inge ); /** - * Filter to supported files, derive a destination container name, POST the upload, - * then start polling the resulting job. Side-effects: sets status/error/firstKey state. + * Ask the server which of *names* already exist in *target*. + * + * POST, not GET: a dropped folder can hold hundreds of filenames, and as query + * params those blow past the HTTP header size limit (431) before the request is + * ever routed. Pass an empty *names* to only ask for a free container name. + */ + const requestPreflight = useCallback( + async (target: string, names: string[]): Promise => { + const res = await fetch(`${API_BASE}/api/ingest/preflight`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ container_path: target, names, server_uri: serverUri }), + }); + if (!res.ok) throw new Error(await res.text()); + return res.json(); + }, + [serverUri], + ); + + /** + * POST the files to the ingest endpoint and start polling the resulting job. + * + * @param onConflict How the backend should treat keys that already exist — + * only ever anything but 'fail' after the user chose in ConflictDialog. + */ + const doUpload = useCallback( + async (files: File[], target: string, onConflict: OnConflict) => { + setPending(null); + setError(null); + setStatus(null); + setExpandedErrors(new Set()); + setLastBatch({ files, target }); + setUploading(true); + try { + const fd = new FormData(); + fd.append('container_path', target); + fd.append('on_conflict', onConflict); + if (description.trim()) fd.append('description', description.trim()); + for (const f of files) fd.append('files', f, f.name); + + const res = await fetch( + `${API_BASE}/api/ingest/upload?server_uri=${encodeURIComponent(serverUri)}`, + { method: 'POST', body: fd }, + ); + if (!res.ok) throw new Error(await res.text()); + const { job_id } = await res.json(); + setJobId(job_id); + setStatus({ + state: 'running', + total: files.length, + done: 0, + failed: 0, + skipped: 0, + errors: [], + container_path: target, + }); + poll(job_id); + } catch (e) { + setError(`Could not start the upload: ${e instanceof Error ? e.message : String(e)}`); + } finally { + setUploading(false); + } + }, + [description, serverUri, poll], + ); + + /** + * Filter to supported files, derive a destination container name, then ask the + * server whether any of these samples already exist there. Clean → upload + * straight away; duplicates → hand off to ConflictDialog so the user decides. */ - const startUpload = useCallback( + const prepareUpload = useCallback( async (files: File[], suggestedName: string) => { const supported = files.filter((f) => isSupported(f.name)); if (supported.length === 0) { @@ -154,34 +253,63 @@ export default function IngestDropzone({ serverUri, onBrowse, onAnnotate }: Inge // Remember the first sample (by name) so we can jump straight to Annotate. setFirstKey(fileStem(sortedNames[0])); - setError(null); - setStatus(null); - setUploading(true); - try { - const fd = new FormData(); - fd.append('container_path', target); - if (description.trim()) fd.append('description', description.trim()); - for (const f of supported) fd.append('files', f, f.name); - const res = await fetch( - `${API_BASE}/api/ingest/upload?server_uri=${encodeURIComponent(serverUri)}`, - { method: 'POST', body: fd }, - ); - if (!res.ok) throw new Error(await res.text()); - const { job_id } = await res.json(); - setJobId(job_id); - setStatus({ state: 'running', total: supported.length, done: 0, failed: 0, errors: [], container_path: target }); - poll(job_id); + setChecking(true); + try { + const pre = await requestPreflight(target, supported.map((f) => f.name)); + if (pre.conflicts.length > 0) { + setPending({ files: supported, target, preflight: pre }); + return; + } } catch (e) { - setError(String(e)); + // Pre-flight is an optimization; never block the upload on it. If it did + // fail, the post-upload backstop below still offers the same choices. + console.warn('ingest pre-flight failed, uploading anyway', e); } finally { - setUploading(false); + setChecking(false); } + await doUpload(supported, target, 'fail'); }, - [containerPath, description, serverUri, poll], + [containerPath, requestPreflight, doUpload], ); + /** + * Backstop for when pre-flight didn't run (or couldn't): if a finished job + * reports conflicts, offer the same four choices after the fact, re-using the + * files we still hold so the user never has to re-drop the folder. + */ + useEffect(() => { + if (!status || (status.state !== 'done' && status.state !== 'error')) return; + const conflicts = status.errors.filter((e) => e?.kind === 'conflict'); + if (conflicts.length === 0) { + setLastBatch(null); + return; + } + if (!lastBatch || lastBatch.target !== status.container_path) return; + + let cancelled = false; + (async () => { + // Only for existing_count / a free name suggestion; conflicts are known. + const pre = await requestPreflight(lastBatch.target, []).catch(() => null); + if (cancelled) return; + setPending({ + files: lastBatch.files, + target: lastBatch.target, + preflight: { + container_exists: true, + existing_count: pre?.existing_count ?? conflicts.length, + conflicts: conflicts.map((e) => ({ filename: e.filename, key: fileStem(e.filename) })), + suggested_container_path: pre?.suggested_container_path ?? `${lastBatch.target}_2`, + }, + }); + })(); + return () => { + cancelled = true; + }; + // `pending` is deliberately not a dependency — setting it here must not retrigger. + }, [status, lastBatch, requestPreflight]); + const fileInputRef = useRef(null); const dirInputRef = useRef(null); @@ -192,9 +320,9 @@ export default function IngestDropzone({ serverUri, onBrowse, onAnnotate }: Inge e.stopPropagation(); setDragging(false); const { files, folderName } = await collectFromDrop(e.dataTransfer); - await startUpload(files, folderName); + await prepareUpload(files, folderName); }, - [startUpload], + [prepareUpload], ); /** File/folder input change handler: start the upload from the chosen files. */ @@ -202,14 +330,26 @@ export default function IngestDropzone({ serverUri, onBrowse, onAnnotate }: Inge (e: React.ChangeEvent) => { const files = Array.from(e.target.files ?? []); const folderName = files[0]?.webkitRelativePath?.split('/')[0] ?? ''; - startUpload(files, folderName); + prepareUpload(files, folderName); }, - [startUpload], + [prepareUpload], ); + /** Toggle the filename list of one error group. */ + const toggleErrorGroup = useCallback((index: number) => { + setExpandedErrors((prev) => { + const next = new Set(prev); + if (!next.delete(index)) next.add(index); + return next; + }); + }, []); + + /** Pre-flight and upload both lock the dropzone. */ + const busy = uploading || checking; + const errorGroups = status ? summarizeIngestErrors(status.errors) : []; const done = status?.state === 'done'; const failedFatally = status?.state === 'error'; - const processed = status ? status.done + status.failed : 0; + const processed = status ? status.done + status.failed + status.skipped : 0; const pct = status && status.total ? Math.round((processed / status.total) * 100) : 0; return ( @@ -222,7 +362,7 @@ export default function IngestDropzone({ serverUri, onBrowse, onAnnotate }: Inge type="text" value={description} onChange={(e) => setDescription(e.target.value)} - disabled={uploading} + disabled={busy} placeholder="e.g. air, sample, void, pore" className="mt-1 w-full border border-white/20 rounded-md px-2 py-1.5 text-sm bg-white/10 text-white focus:outline-none focus:ring-2 focus:ring-sky-500 disabled:opacity-50" /> @@ -235,7 +375,7 @@ export default function IngestDropzone({ serverUri, onBrowse, onAnnotate }: Inge
!uploading && fileInputRef.current?.click()} + onClick={() => !busy && fileInputRef.current?.click()} onDragEnter={(e) => { e.preventDefault(); e.stopPropagation(); @@ -263,7 +403,7 @@ export default function IngestDropzone({ serverUri, onBrowse, onAnnotate }: Inge multiple accept=".tif,.tiff,.png,.jpg,.jpeg,.npy" onChange={onPick} - disabled={uploading} + disabled={busy} className="hidden" />

- {uploading ? 'Uploading…' : 'Drag an image file or folder of images here'} + {checking + ? 'Checking the destination…' + : uploading + ? 'Uploading…' + : 'Drag an image file or folder of images here'}

- {!uploading && ( + {!busy && (

or + )} +

+ {expandedErrors.has(i) && ( +
    + {group.filenames.map((name) => ( +
  • {name}
    • + ))} +
+ )} + ))} )} - {done && status.done > 0 && (onBrowse || onAnnotate) && ( + {/* Skipped duplicates are still in the dataset, so browsing makes sense. */} + {done && status.done + status.skipped > 0 && (onBrowse || onAnnotate) && (
{onBrowse && (
); } diff --git a/frontend/src/components/Ingest/ingestErrors.test.ts b/frontend/src/components/Ingest/ingestErrors.test.ts new file mode 100644 index 0000000..6a8bda7 --- /dev/null +++ b/frontend/src/components/Ingest/ingestErrors.test.ts @@ -0,0 +1,67 @@ +import { describe, it, expect } from 'vitest'; +import { describeIngestError, summarizeIngestErrors, type IngestError } from './ingestErrors'; + +function err(filename: string, kind: IngestError['kind'], message = ''): IngestError { + return { filename, kind, message }; +} + +describe('describeIngestError', () => { + it('names the file and the reason', () => { + expect(describeIngestError(err('a.tif', 'conflict'))).toBe( + 'a.tif — already exists in this dataset', + ); + }); + + it('shows the backend message for an unknown kind instead of generic copy', () => { + const line = describeIngestError(err('a.tif', 'unknown', 'array is 4-D')); + expect(line).toBe('a.tif — array is 4-D'); + }); + + it('renders a legacy plain-string error verbatim', () => { + // Older backends sent `errors: string[]`; must not collapse to a useless line. + expect(describeIngestError('a.tif: 409: /browse/ds/a')).toBe('a.tif: 409: /browse/ds/a'); + }); + + it('omits the dash for a job-level failure with no filename', () => { + expect(describeIngestError(err('', 'unreachable'))).toBe('could not reach the Tiled server'); + }); +}); + +describe('summarizeIngestErrors', () => { + it('collapses a whole folder of identical failures into one row', () => { + const errors = Array.from({ length: 690 }, (_, i) => err(`img_${i}.tif`, 'conflict')); + const groups = summarizeIngestErrors(errors); + + expect(groups).toHaveLength(1); + expect(groups[0].summary).toBe('690 images already exist in this dataset'); + expect(groups[0].filenames).toHaveLength(690); + }); + + it('keeps distinct reasons in separate rows, in first-seen order', () => { + const groups = summarizeIngestErrors([ + err('a.tif', 'conflict'), + err('b.tif', 'unreadable'), + err('c.tif', 'conflict'), + ]); + + expect(groups.map((g) => g.kind)).toEqual(['conflict', 'unreadable']); + expect(groups[0].filenames).toEqual(['a.tif', 'c.tif']); + }); + + it('does not merge unknown failures that have different messages', () => { + const groups = summarizeIngestErrors([ + err('a.tif', 'unknown', 'array is 4-D'), + err('b.tif', 'unknown', 'array is 4-D'), + err('c.tif', 'unknown', 'disk full'), + ]); + + expect(groups).toHaveLength(2); + expect(groups[0].summary).toBe('2 images could not be ingested — array is 4-D'); + expect(groups[1].summary).toBe('c.tif — disk full'); + }); + + it('renders a single failure as itself, not as "1 image …"', () => { + const [group] = summarizeIngestErrors([err('a.tif', 'conflict')]); + expect(group.summary).toBe('a.tif — already exists in this dataset'); + }); +}); diff --git a/frontend/src/components/Ingest/ingestErrors.ts b/frontend/src/components/Ingest/ingestErrors.ts new file mode 100644 index 0000000..909ff31 --- /dev/null +++ b/frontend/src/components/Ingest/ingestErrors.ts @@ -0,0 +1,102 @@ +/** + * Human-readable copy for the classified per-file errors an ingest job reports. + * + * The backend (see ``_classify_error`` in backend/ingest.py) sends a stable + * `kind` token plus a short fallback message, deliberately stripped of internal + * API URLs. This module owns the user-facing wording. + */ + +export type IngestErrorKind = + | 'conflict' + | 'unreadable' + | 'unreachable' + | 'auth' + | 'unknown'; + +export interface IngestError { + /** Original filename, or '' for a job-level failure. */ + filename: string; + kind: IngestErrorKind; + /** Backend fallback text; used when the kind carries no specific copy. */ + message: string; +} + +/** One line of the error list: a reason plus every file that hit it. */ +export interface IngestErrorGroup { + kind: IngestErrorKind; + /** Sentence describing the reason, already pluralized for `filenames`. */ + summary: string; + filenames: string[]; +} + +/** Per-file copy. `unknown` has none — the backend's own message is better. */ +const COPY: Partial> = { + conflict: 'already exists in this dataset', + unreadable: 'could not be read as an image', + unreachable: 'could not reach the Tiled server', + auth: 'not authorized to write to this server', +}; + +/** Copy for a whole group, as ` image(s) `. */ +const GROUP_COPY: Partial> = { + conflict: 'already exist in this dataset', + unreadable: 'could not be read as images', + unreachable: 'could not be sent — the Tiled server was unreachable', + auth: 'were rejected — not authorized to write to this server', +}; + +const GENERIC = 'could not be ingested'; + +/** + * Normalize an error entry. Older backends sent plain strings, and an entry from + * a newer one may carry a kind we don't know — render those verbatim rather than + * flattening every row to a useless generic sentence. + */ +function normalize(err: IngestError | string): IngestError { + if (typeof err === 'string') return { filename: '', kind: 'unknown', message: err }; + return { + filename: err?.filename ?? '', + kind: err?.kind ?? 'unknown', + message: err?.message ?? '', + }; +} + +/** Return a one-line sentence for an ingest error, prefixed by its filename. */ +export function describeIngestError(err: IngestError | string): string { + const { filename, kind, message } = normalize(err); + const detail = COPY[kind] ?? message ?? ''; + return filename ? `${filename} — ${detail || GENERIC}` : detail || message || GENERIC; +} + +/** + * Collapse an error list into one row per reason, so a 690-file batch that all + * failed the same way reads as a single sentence instead of five identical rows. + * + * Errors of an unrecognized kind keep their own distinct message, so genuinely + * different failures never get merged. + * + * @returns Groups in first-seen order, each with the filenames that hit it. + */ +export function summarizeIngestErrors(errors: (IngestError | string)[]): IngestErrorGroup[] { + const groups = new Map(); + for (const raw of errors) { + const err = normalize(raw); + // Known kinds merge by kind; unknown ones merge only by identical message. + const key = GROUP_COPY[err.kind] ? err.kind : `${err.kind}:${err.message}`; + const bucket = groups.get(key); + if (bucket) bucket.push(err); + else groups.set(key, [err]); + } + + return [...groups.values()].map((entries) => { + const { kind, message } = entries[0]; + const reason = GROUP_COPY[kind] ?? (message ? `${GENERIC} — ${message}` : GENERIC); + return { + kind, + // A lone failure reads better as itself than as "1 image …". + summary: + entries.length === 1 ? describeIngestError(entries[0]) : `${entries.length} images ${reason}`, + filenames: entries.map((e) => e.filename).filter(Boolean), + }; + }); +} diff --git a/frontend/src/lib/sourceKey.test.ts b/frontend/src/lib/sourceKey.test.ts new file mode 100644 index 0000000..3ddf469 --- /dev/null +++ b/frontend/src/lib/sourceKey.test.ts @@ -0,0 +1,37 @@ +import { describe, it, expect } from 'vitest'; +import { buildSourceKey, isAnnotatedPath } from './sourceKey'; + +const URI = 'http://127.0.0.1:8010'; +const key = (path: string) => buildSourceKey('tiled', path, URI); + +describe('isAnnotatedPath', () => { + it('matches the sample own key', () => { + const keys = new Set([key('browse/ds')]); + expect(isAnnotatedPath(keys, 'browse/ds', URI)).toBe(true); + }); + + it('matches a volume whose individual array was annotated', () => { + // "Annotate first image" keys the array, not the volume. + const keys = new Set([key('browse/ds/img_00003')]); + expect(isAnnotatedPath(keys, 'browse/ds', URI)).toBe(true); + }); + + it('does not badge a slice because a sibling slice was annotated', () => { + const keys = new Set([key('browse/ds')]); + expect(isAnnotatedPath(keys, 'browse/ds/img_00003', URI)).toBe(false); + }); + + it('does not match a different dataset with a shared name prefix', () => { + const keys = new Set([key('browse/ds_2/img_1')]); + expect(isAnnotatedPath(keys, 'browse/ds', URI)).toBe(false); + }); + + it('does not match the same path on another server', () => { + const keys = new Set([buildSourceKey('tiled', 'browse/ds', 'http://other:8010')]); + expect(isAnnotatedPath(keys, 'browse/ds', URI)).toBe(false); + }); + + it('is false for an empty key set', () => { + expect(isAnnotatedPath(new Set(), 'browse/ds', URI)).toBe(false); + }); +}); diff --git a/frontend/src/lib/sourceKey.ts b/frontend/src/lib/sourceKey.ts index c039151..5b0072e 100644 --- a/frontend/src/lib/sourceKey.ts +++ b/frontend/src/lib/sourceKey.ts @@ -17,3 +17,36 @@ export function buildSourceKey( ): string { return kind === 'tiled' ? `tiled:${serverUri ?? ''}:${path}` : `local:${path}`; } + +/** + * True if *path* itself, or anything below it, has annotations. + * + * One image can legitimately be keyed two ways: as a whole volume + * (`browse/ds`, annotated slice-by-slice — what Browse opens from the browse + * root) or as a standalone array (`browse/ds/img_0001` — what "Annotate first + * image" and slice-level entry points use). An exact-match lookup therefore + * leaves a dataset looking untouched when it was annotated through the other + * entry point, so a container also counts as annotated when a descendant is. + * + * Deliberately NOT the reverse: a single slice must not be badged just because + * some other slice of its volume was annotated — that would light up every + * frame of a 690-image stack on the strength of one. + * + * @param annotatedKeys Source keys known to have annotations. + * @param path Tiled path of the row being rendered. + * @param serverUri Server the row belongs to (part of the key). + */ +export function isAnnotatedPath( + annotatedKeys: Set, + path: string, + serverUri?: string | null, +): boolean { + const self = buildSourceKey('tiled', path, serverUri); + if (annotatedKeys.has(self)) return true; + + const childPrefix = `${self}/`; + for (const key of annotatedKeys) { + if (key.startsWith(childPrefix)) return true; + } + return false; +} diff --git a/frontend/src/stores/connectionStore.ts b/frontend/src/stores/connectionStore.ts index 9db4830..56df832 100644 --- a/frontend/src/stores/connectionStore.ts +++ b/frontend/src/stores/connectionStore.ts @@ -12,6 +12,13 @@ export interface ConnectionState { serverUri: string | null; /** Tiled container to browse (e.g. "browse/myset"); null = auto-discover */ browseContainerPath: string | null; + /** + * Full path of a sample to auto-select on arriving in Browse (e.g. + * "browse/myset"). Set when jumping in from ingest so the user lands on the + * dataset they just touched, while Browse still lists from the root — where a + * sample is the whole volume and its annotations resolve. + */ + browseFocusPath: string | null; /** Granted absolute browse root (local mode) */ localRoot: string | null; /** Chosen subfolder relative to localRoot (local mode) */ @@ -24,6 +31,7 @@ export interface ConnectionState { kind: 'tiled' | 'local'; serverUri?: string | null; browseContainerPath?: string | null; + browseFocusPath?: string | null; localRoot?: string | null; localRel?: string | null; label: string; @@ -36,6 +44,7 @@ export const useConnectionStore = create((set) => ({ kind: null, serverUri: null, browseContainerPath: null, + browseFocusPath: null, localRoot: null, localRel: null, label: null, @@ -46,12 +55,22 @@ export const useConnectionStore = create((set) => ({ kind, serverUri = null, browseContainerPath = null, + browseFocusPath = null, localRoot = null, localRel = null, label, sampleCount, }) => - set({ kind, serverUri, browseContainerPath, localRoot, localRel, label, sampleCount }), + set({ + kind, + serverUri, + browseContainerPath, + browseFocusPath, + localRoot, + localRel, + label, + sampleCount, + }), /** Resets all connection fields to null (disconnect). */ clearConnection: () => @@ -59,6 +78,7 @@ export const useConnectionStore = create((set) => ({ kind: null, serverUri: null, browseContainerPath: null, + browseFocusPath: null, localRoot: null, localRel: null, label: null,