From cf7c4516c9e8231b4ff3149d213d9675240999d4 Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Wed, 6 Dec 2023 16:55:38 -0600 Subject: [PATCH 01/10] update resolveURL, add tests (#3890) --- .../useEvents/useSetFieldVisibilityStage.ts | 14 +++- .../app/src/useEvents/useStateUpdate.ts | 15 ++-- .../app/src/useSetters/onSetDataset.ts | 13 ++- .../useSetters/onSetFieldVisibilityStage.ts | 14 +++- app/packages/app/src/useSetters/onSetView.ts | 13 ++- .../app/src/useSetters/onSetViewName.ts | 14 +++- app/packages/app/src/utils.test.ts | 82 +++++++++++++++++++ app/packages/app/src/utils.ts | 56 +++++++------ 8 files changed, 175 insertions(+), 46 deletions(-) create mode 100644 app/packages/app/src/utils.test.ts diff --git a/app/packages/app/src/useEvents/useSetFieldVisibilityStage.ts b/app/packages/app/src/useEvents/useSetFieldVisibilityStage.ts index dea5177a73..0334385a7d 100644 --- a/app/packages/app/src/useEvents/useSetFieldVisibilityStage.ts +++ b/app/packages/app/src/useEvents/useSetFieldVisibilityStage.ts @@ -24,10 +24,16 @@ const useSetFieldVisibilityStage: EventHandlerHook = () => { unsubscribe(); }); - router.history.replace(resolveURL(router), { - ...router.get().state, - fieldVisibility: stage, - }); + router.history.replace( + resolveURL({ + currentPathname: router.history.location.pathname, + currentSearch: router.history.location.search, + }), + { + ...router.get().state, + fieldVisibility: stage, + } + ); }, [session, setter] ); diff --git a/app/packages/app/src/useEvents/useStateUpdate.ts b/app/packages/app/src/useEvents/useStateUpdate.ts index 557051522f..294d97e3bb 100644 --- a/app/packages/app/src/useEvents/useStateUpdate.ts +++ b/app/packages/app/src/useEvents/useStateUpdate.ts @@ -16,11 +16,16 @@ const useStateUpdate: EventHandlerHook = ({ (payload: any) => { const state = processState(session.current, payload.state); const stateless = env().VITE_NO_STATE; - const path = resolveURL( - router, - stateless ? getDatasetName() : payload.state.dataset, - stateless ? getSavedViewName() : payload.state.saved_view_slug - ); + const path = resolveURL({ + currentPathname: router.history.location.pathname, + currentSearch: router.history.location.search, + nextDataset: stateless + ? getDatasetName() + : payload.state.dataset ?? null, + nextView: stateless + ? getSavedViewName() + : payload.state.saved_view_slug, + }); if (readyStateRef.current !== AppReadyState.OPEN) { router.history.replace(path, state); router.load().then(() => setReadyState(AppReadyState.OPEN)); diff --git a/app/packages/app/src/useSetters/onSetDataset.ts b/app/packages/app/src/useSetters/onSetDataset.ts index 4614adc9d2..3902d05a27 100644 --- a/app/packages/app/src/useSetters/onSetDataset.ts +++ b/app/packages/app/src/useSetters/onSetDataset.ts @@ -39,9 +39,16 @@ const onSetDataset: RegisteredSetter = entry.data.dataset?.defaultGroupSlice || undefined; }); - router.history.push(resolveURL(router, datasetName || null), { - view: [], - }); + router.history.push( + resolveURL({ + currentPathname: router.history.location.pathname, + currentSearch: router.history.location.search, + nextDataset: datasetName || null, + }), + { + view: [], + } + ); }; export default onSetDataset; diff --git a/app/packages/app/src/useSetters/onSetFieldVisibilityStage.ts b/app/packages/app/src/useSetters/onSetFieldVisibilityStage.ts index 36d47bc0a3..55a7e36055 100644 --- a/app/packages/app/src/useSetters/onSetFieldVisibilityStage.ts +++ b/app/packages/app/src/useSetters/onSetFieldVisibilityStage.ts @@ -31,10 +31,16 @@ const onSetFieldVisibilityStage: RegisteredSetter = }); // reload page query with new extended view - router.history.replace(resolveURL(router), { - ...router.get().state, - fieldVisibility: stage, - }); + router.history.replace( + resolveURL({ + currentPathname: router.history.location.pathname, + currentSearch: router.history.location.search, + }), + { + ...router.get().state, + fieldVisibility: stage, + } + ); // send event as side effect commitMutation(environment, { diff --git a/app/packages/app/src/useSetters/onSetView.ts b/app/packages/app/src/useSetters/onSetView.ts index 5fe623e6f0..5143cbbd03 100644 --- a/app/packages/app/src/useSetters/onSetView.ts +++ b/app/packages/app/src/useSetters/onSetView.ts @@ -43,9 +43,16 @@ const onSetView: RegisteredSetter = sessionRef.current.selectedLabels = []; sessionRef.current.selectedSamples = new Set(); sessionRef.current.fieldVisibilityStage = undefined; - router.history.push(resolveURL(router, dataset), { - view, - }); + router.history.push( + resolveURL({ + currentPathname: router.history.location.pathname, + currentSearch: router.history.location.search, + nextDataset: dataset, + }), + { + view, + } + ); }, }); }; diff --git a/app/packages/app/src/useSetters/onSetViewName.ts b/app/packages/app/src/useSetters/onSetViewName.ts index f6743677e1..5cbc2c1be5 100644 --- a/app/packages/app/src/useSetters/onSetViewName.ts +++ b/app/packages/app/src/useSetters/onSetViewName.ts @@ -33,9 +33,17 @@ const onSetViewName: RegisteredSetter = sessionRef.current.selectedLabels = []; sessionRef.current.selectedSamples = new Set(); sessionRef.current.fieldVisibilityStage = undefined; - router.history.push(resolveURL(router, dataset, slug || undefined), { - view: [], - }); + router.history.push( + resolveURL({ + currentPathname: router.history.location.pathname, + currentSearch: router.history.location.search, + nextDataset: dataset, + nextView: slug || undefined, + }), + { + view: [], + } + ); }; export default onSetViewName; diff --git a/app/packages/app/src/utils.test.ts b/app/packages/app/src/utils.test.ts new file mode 100644 index 0000000000..20057d462b --- /dev/null +++ b/app/packages/app/src/utils.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from "vitest"; +import { resolveURL } from "./utils"; + +describe("resolves datasets", () => { + it("resolves to /", () => { + expect( + resolveURL({ + currentPathname: "/datasets/my-dataset", + currentSearch: "", + nextDataset: null, + }) + ).toBe("/"); + expect( + resolveURL({ currentPathname: "/datasets/my-dataset", currentSearch: "" }) + ).toBe("/datasets/my-dataset"); + }); +}); + +describe("resolves wih proxy", () => { + it("resolves to /", () => { + expect( + resolveURL({ + currentPathname: "/datasets/my-dataset", + currentSearch: "?proxy=/my/proxy", + nextDataset: null, + }) + ).toBe(`/my/proxy?proxy=${encodeURIComponent("/my/proxy")}`); + expect( + resolveURL({ + currentPathname: "/my/proxy/datasets/my-dataset", + currentSearch: "?proxy=/my/proxy", + }) + ).toBe( + `/my/proxy/datasets/my-dataset?proxy=${encodeURIComponent("/my/proxy")}` + ); + }); +}); + +describe("resolves views", () => { + it("throws error", () => { + expect(() => + resolveURL({ + currentPathname: "", + currentSearch: "", + nextDataset: null, + nextView: "view", + }) + ).toThrowError(); + }); + + it("throws error", () => { + expect(() => + resolveURL({ + currentPathname: "", + currentSearch: "", + nextView: "view", + }) + ).toThrowError(); + }); + + it("resolves with saved view", () => { + expect( + resolveURL({ + currentPathname: "/datasets/my-dataset", + currentSearch: "", + nextDataset: "my-dataset", + nextView: "view", + }) + ).toBe(`/datasets/my-dataset?view=view`); + }); +}); + +describe("resolves current", () => { + it("does not change location", () => { + expect( + resolveURL({ + currentPathname: "/datasets/my-dataset", + currentSearch: "?view=view", + }) + ).toBe("/datasets/my-dataset?view=view"); + }); +}); diff --git a/app/packages/app/src/utils.ts b/app/packages/app/src/utils.ts index 3ac2a552dd..b10dfa1390 100644 --- a/app/packages/app/src/utils.ts +++ b/app/packages/app/src/utils.ts @@ -1,4 +1,3 @@ -import { Router } from "./makeRoutes"; import { matchPath } from "./routing"; export const getDatasetName = (pathname?: string) => { @@ -28,33 +27,42 @@ export const getSavedViewName = (search?: string) => { return null; }; -export function resolveURL(router: Router): string; -export function resolveURL( - router: Router, - dataset: string | null, - view?: string -): string; -export function resolveURL( - router: Router, - dataset?: string | null, - view?: string -): string { - const params = new URLSearchParams(router.history.location.search); - - if (dataset === undefined) { - return `${window.location.pathname}${params.toString()}`; +export function resolveURL(params: { + currentSearch: string; + currentPathname: string; + nextDataset?: string | null; + nextView?: string; +}): string { + const searchParams = new URLSearchParams(params.currentSearch); + + if (!params.nextDataset && params.nextView) { + throw new Error("a view cannot be provided without a dataset"); + } + + if (params.nextDataset) { + params.nextView + ? searchParams.set("view", encodeURIComponent(params.nextView)) + : searchParams.delete("view"); + } + + let newSearch = searchParams.toString(); + if (newSearch.length) { + newSearch = `?${newSearch}`; } - view ? params.set("view", encodeURIComponent(view)) : params.delete("view"); - let search = params.toString(); - if (search.length) { - search = `?${search}`; + if (params.nextDataset === undefined) { + // update search params only + return `${params.currentPathname}${newSearch}`; } - const path = decodeURIComponent(params.get("proxy") ?? ""); - if (dataset) { - return `${path}/datasets/${encodeURIComponent(dataset)}${search}`; + const path = decodeURIComponent(searchParams.get("proxy") ?? ""); + if (params.nextDataset === null) { + // go to index page + return `${path.length ? path : "/"}${newSearch}`; } - return `${path.length ? path : "/"}${search}`; + // go to dataset + return `${path}/datasets/${encodeURIComponent( + params.nextDataset + )}${newSearch}`; } From 6573a175c76a260e45c31437da9de50bc2a21333 Mon Sep 17 00:00:00 2001 From: Stuart Wheaton Date: Thu, 7 Dec 2023 13:39:35 -0500 Subject: [PATCH 02/10] remove PIL.ExifTags usage to support 6.2 --- fiftyone/core/metadata.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/fiftyone/core/metadata.py b/fiftyone/core/metadata.py index eba781d5ae..86737ebd7d 100644 --- a/fiftyone/core/metadata.py +++ b/fiftyone/core/metadata.py @@ -11,7 +11,7 @@ import os import requests -from PIL import ExifTags, Image +from PIL import Image import eta.core.utils as etau import eta.core.video as etav @@ -299,7 +299,12 @@ def _image_has_flipped_dimensions(img): Returns: True if image width/height should be flipped """ - exif_orientation = img.getexif().get(ExifTags.Base.Orientation) + # Value from PIL.ExifTags.Base.Orientation == 274 + # We hard-code the value directly here so we can support older Pillow + # versions that don't have ExifTags.Base. + # It's ok because this value will never change. + orientation_tag = 0x0112 + exif_orientation = img.getexif().get(orientation_tag) # 5, 6, 7, 8 --> TRANSPOSE, ROTATE_270, TRANSVERSE, ROTATE_90 is_rotated = exif_orientation in {5, 6, 7, 8} return is_rotated From f10accdec22316c038040c34e8ccdf320a89ed0e Mon Sep 17 00:00:00 2001 From: brimoor Date: Wed, 6 Dec 2023 20:32:02 -0500 Subject: [PATCH 03/10] ensure group indexes exist when importing FiftyOneDataset format --- fiftyone/utils/data/importers.py | 5 ++++ tests/unittests/group_tests.py | 47 ++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/fiftyone/utils/data/importers.py b/fiftyone/utils/data/importers.py index 6829668fac..3886e5a25d 100644 --- a/fiftyone/utils/data/importers.py +++ b/fiftyone/utils/data/importers.py @@ -1832,6 +1832,11 @@ def _import_samples(self, dataset, dataset_dict, tags=None): conn.datasets.replace_one({"name": name}, dataset_dict) dataset._reload(hard=True) + + if dataset.media_type == fomm.GROUP: + fod._create_group_indexes( + dataset._sample_collection_name, dataset.group_field + ) else: # # The dataset we're merging into is non-empty, but it is safe to diff --git a/tests/unittests/group_tests.py b/tests/unittests/group_tests.py index 9bba38883f..7ee999f7e5 100644 --- a/tests/unittests/group_tests.py +++ b/tests/unittests/group_tests.py @@ -1332,6 +1332,53 @@ def test_group_import_export(self): set(flat_view2.values("filepath")), ) + @drop_datasets + def test_fiftyone_dataset_group_indexes(self): + dataset = _make_group_dataset() + + group_indexes = { + "id", + "filepath", + "frames.id", + "frames._sample_id_1_frame_number_1", + "group_field.id", + "group_field.name", + } + + export_dir = self._new_dir() + + dataset.export( + export_dir=export_dir, + dataset_type=fo.types.FiftyOneDataset, + export_media=False, + ) + + dataset2 = fo.Dataset.from_dir( + dataset_dir=export_dir, + dataset_type=fo.types.FiftyOneDataset, + ) + + self.assertEqual(len(dataset), len(dataset2)) + self.assertSetEqual(set(dataset.list_indexes()), group_indexes) + self.assertSetEqual(set(dataset2.list_indexes()), group_indexes) + + export_dir = self._new_dir() + + dataset.export( + export_dir=export_dir, + dataset_type=fo.types.LegacyFiftyOneDataset, + export_media=False, + ) + + dataset2 = fo.Dataset.from_dir( + dataset_dir=export_dir, + dataset_type=fo.types.LegacyFiftyOneDataset, + ) + + self.assertEqual(len(dataset), len(dataset2)) + self.assertSetEqual(set(dataset.list_indexes()), group_indexes) + self.assertSetEqual(set(dataset2.list_indexes()), group_indexes) + @drop_datasets def test_disjoint_groups(self): dataset, first, second = make_disjoint_groups_dataset() From 5da9442ca37064ef6ca80a88e2ce24244530a008 Mon Sep 17 00:00:00 2001 From: brimoor Date: Wed, 6 Dec 2023 10:14:16 -0500 Subject: [PATCH 04/10] fixing bug when inferring doubly-nested dynamic list field types --- fiftyone/core/collections.py | 24 ++++++++++--- tests/unittests/dataset_tests.py | 60 ++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/fiftyone/core/collections.py b/fiftyone/core/collections.py index 84466e49d7..dfb06c210a 100644 --- a/fiftyone/core/collections.py +++ b/fiftyone/core/collections.py @@ -1249,8 +1249,9 @@ def _get_dynamic_field_schema( else: fields = list(fields) + unwind_cache = [] dynamic_schema = self._do_get_dynamic_field_schema( - schema, frames=frames, fields=fields + schema, unwind_cache, frames=frames, fields=fields ) # Recurse into new dynamic fields @@ -1258,7 +1259,7 @@ def _get_dynamic_field_schema( s = dynamic_schema while True: s = self._do_get_dynamic_field_schema( - s, frames=frames, new=True + s, unwind_cache, frames=frames, new=True ) if s: dynamic_schema.update(s) @@ -1292,7 +1293,7 @@ def _get_dynamic_field_schema( return dynamic_schema def _do_get_dynamic_field_schema( - self, schema, frames=False, fields=None, new=False + self, schema, unwind_cache, frames=False, fields=None, new=False ): if frames: prefix = self._FRAMES_PREFIX @@ -1313,6 +1314,14 @@ def _do_get_dynamic_field_schema( for path, field in schema.items(): _path = prefix + path + # When recursively getting schemas, we may have previously needed + # to manually unwind an undeclared list field at a higher level. + # This injects any matching unwinds so that we can properly handle + # the shape of the data + for _clean_path, _unwind_path in unwind_cache: + if _path.startswith(_clean_path): + _path = _unwind_path + _path[len(_clean_path) :] + is_list_field = False while isinstance(field, fof.ListField): field = field.field @@ -1322,6 +1331,12 @@ def _do_get_dynamic_field_schema( if is_list_field and not _path.endswith("[]"): _path += "[]" + # Cache the manual unwind in case we need it recursively + # Insert rather than append so that nested paths are found + # first when using this cache + _clean_path = _path.replace("[]", "") + unwind_cache.insert(0, (_clean_path, _path)) + if new: # This field hasn't been declared yet, so we must provide # it's document type so that `Schema()` can distinguish @@ -1332,11 +1347,12 @@ def _do_get_dynamic_field_schema( agg = foa.Schema(_path, dynamic_only=True, _doc_type=_doc_type) elif is_list_field: - # Processing untyped default list fields is not allowed _clean_path = _path.replace("[]", "") if field is None and not self._is_default_field(_clean_path): agg = foa.ListSchema(_path) else: + # Found a default list field with no element type declared. + # Don't infer types here; the elements must stay untyped agg = None else: agg = None diff --git a/tests/unittests/dataset_tests.py b/tests/unittests/dataset_tests.py index 2bacb1b073..a674a35bb7 100644 --- a/tests/unittests/dataset_tests.py +++ b/tests/unittests/dataset_tests.py @@ -4537,6 +4537,66 @@ def test_add_dynamic_fields_nested(self): self.assertIsInstance(field, fo.ListField) self.assertIsInstance(field.field, fo.Field) + @drop_datasets + def test_dynamic_fields_nested_lists(self): + dataset = fo.Dataset() + + detection = fo.Detection(list_attr=["one", "two", "three"]) + detections = fo.Detections(detections=[detection]) + + sample = fo.Sample( + filepath="image.jpg", + ground_truth=detections, + embedded_field=fo.DynamicEmbeddedDocument(detections=detections), + ) + + dataset.add_sample(sample) + + dynamic_schema = dataset.get_dynamic_field_schema() + self.assertSetEqual( + set(dynamic_schema.keys()), + { + "ground_truth.detections.list_attr", + "embedded_field.detections", + "embedded_field.detections.detections.list_attr", + }, + ) + + list_field1 = dynamic_schema["ground_truth.detections.list_attr"] + self.assertIsInstance(list_field1, fo.ListField) + self.assertIsInstance(list_field1.field, fo.StringField) + + list_field2 = dynamic_schema[ + "embedded_field.detections.detections.list_attr" + ] + self.assertIsInstance(list_field2, fo.ListField) + self.assertIsInstance(list_field2.field, fo.StringField) + + dataset.add_dynamic_sample_fields() + + new_paths = [ + "ground_truth.detections.list_attr", + "embedded_field.detections", + "embedded_field.detections.detections.list_attr", + ] + schema = dataset.get_field_schema(flat=True) + for path in new_paths: + self.assertIn(path, schema) + + list_field1 = dataset.get_field("ground_truth.detections.list_attr") + self.assertIsInstance(list_field1, fo.ListField) + self.assertIsInstance(list_field1.field, fo.StringField) + + list_field2 = dataset.get_field( + "embedded_field.detections.detections.list_attr" + ) + self.assertIsInstance(list_field2, fo.ListField) + self.assertIsInstance(list_field2.field, fo.StringField) + + dynamic_schema = dataset.get_dynamic_field_schema() + + self.assertDictEqual(dynamic_schema, {}) + @drop_datasets def test_dynamic_frame_fields_nested(self): sample = fo.Sample(filepath="video.mp4") From 4e62e9a2b4a5d606d0e685b3aaf28fb7709ff208 Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Thu, 7 Dec 2023 16:35:42 -0600 Subject: [PATCH 05/10] Handle App query errors (#3903) * handle query errors * lint --- .../app/src/pages/datasets/DatasetPage.tsx | 5 --- app/packages/app/src/routing/RouterContext.ts | 18 ++++++----- app/packages/app/src/routing/useRouter.ts | 4 ++- app/packages/state/src/utils.ts | 32 +++++++++++++------ 4 files changed, 36 insertions(+), 23 deletions(-) diff --git a/app/packages/app/src/pages/datasets/DatasetPage.tsx b/app/packages/app/src/pages/datasets/DatasetPage.tsx index a07a0ba7bb..0fb7a3432a 100644 --- a/app/packages/app/src/pages/datasets/DatasetPage.tsx +++ b/app/packages/app/src/pages/datasets/DatasetPage.tsx @@ -6,7 +6,6 @@ import { OperatorCore } from "@fiftyone/operators"; import "@fiftyone/relay"; import * as fos from "@fiftyone/state"; import { datasetQueryContext } from "@fiftyone/state"; -import { NotFoundError } from "@fiftyone/utilities"; import React, { useEffect } from "react"; import { usePreloadedQuery } from "react-relay"; import { useRecoilValue } from "recoil"; @@ -110,10 +109,6 @@ const DatasetPage: Route = ({ prepared }) => { ?.classList.toggle("modalon", isModalActive); }, [isModalActive]); - if (!data.dataset?.name) { - throw new NotFoundError({ path: `/datasets/${prepared.variables.name}` }); - } - return ( <>