Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Utility for adding a path to a sidebar group #5251

Merged
merged 1 commit into from
Dec 12, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
add_path_to_sidebar_group util
brimoor committed Dec 10, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit fe56fb367a275d4642c06cba0741d30e1fe6bb27
38 changes: 7 additions & 31 deletions fiftyone/core/dataset.py
Original file line number Diff line number Diff line change
@@ -45,8 +45,7 @@
import fiftyone.core.labels as fol
import fiftyone.core.media as fom
import fiftyone.core.metadata as fome
from fiftyone.core.odm.dataset import SampleFieldDocument
from fiftyone.core.odm.dataset import DatasetAppConfig, SidebarGroupDocument
from fiftyone.core.odm.dataset import DatasetAppConfig
import fiftyone.migrations as fomi
import fiftyone.core.odm as foo
import fiftyone.core.sample as fos
@@ -1866,35 +1865,12 @@ def create_summary_field(
if sidebar_group is None:
sidebar_group = "summaries"

if self.app_config.sidebar_groups is None:
sidebar_groups = DatasetAppConfig.default_sidebar_groups(self)
self.app_config.sidebar_groups = sidebar_groups
else:
sidebar_groups = self.app_config.sidebar_groups

index_group = None
for group in sidebar_groups:
if group.name == sidebar_group:
index_group = group
else:
if field_name in group.paths:
group.paths.remove(field_name)

if index_group is None:
index_group = SidebarGroupDocument(name=sidebar_group)

insert_after = None
for i, group in enumerate(sidebar_groups):
if group.name == "labels":
insert_after = i

if insert_after is None:
sidebar_groups.append(index_group)
else:
sidebar_groups.insert(insert_after + 1, index_group)

if field_name not in index_group.paths:
index_group.paths.append(field_name)
self.app_config._add_path_to_sidebar_group(
field_name,
sidebar_group,
after_group="labels",
dataset=self,
)

if create_index:
for _field_name in index_fields:
38 changes: 38 additions & 0 deletions fiftyone/core/odm/dataset.py
Original file line number Diff line number Diff line change
@@ -624,6 +624,44 @@ def _rename_paths(self, paths, new_paths):
for path, new_path in zip(paths, new_paths):
self._rename_path(path, new_path)

def _add_path_to_sidebar_group(
self,
path,
sidebar_group,
after_group=None,
dataset=None,
):
if self.sidebar_groups is None:
if dataset is None:
return

Comment on lines +627 to +637
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add parameter validation and improve error handling

The method should validate the path parameter and provide a more informative error message when dataset is required but not provided.

     def _add_path_to_sidebar_group(
         self,
         path,
         sidebar_group,
         after_group=None,
         dataset=None,
     ):
+        if not path or not isinstance(path, str):
+            raise ValueError("path must be a non-empty string")
+        if not sidebar_group or not isinstance(sidebar_group, str):
+            raise ValueError("sidebar_group must be a non-empty string")
         if self.sidebar_groups is None:
             if dataset is None:
-                return
+                raise ValueError("dataset parameter is required when sidebar_groups is not initialized")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def _add_path_to_sidebar_group(
self,
path,
sidebar_group,
after_group=None,
dataset=None,
):
if self.sidebar_groups is None:
if dataset is None:
return
def _add_path_to_sidebar_group(
self,
path,
sidebar_group,
after_group=None,
dataset=None,
):
if not path or not isinstance(path, str):
raise ValueError("path must be a non-empty string")
if not sidebar_group or not isinstance(sidebar_group, str):
raise ValueError("sidebar_group must be a non-empty string")
if self.sidebar_groups is None:
if dataset is None:
raise ValueError("dataset parameter is required when sidebar_groups is not initialized")

self.sidebar_groups = self.default_sidebar_groups(dataset)

index_group = None
for group in self.sidebar_groups:
if group.name == sidebar_group:
index_group = group
else:
if path in group.paths:
group.paths.remove(path)

if index_group is None:
index_group = SidebarGroupDocument(name=sidebar_group)

insert_after = None
if after_group is not None:
for i, group in enumerate(self.sidebar_groups):
if group.name == after_group:
insert_after = i

if insert_after is None:
self.sidebar_groups.append(index_group)
else:
self.sidebar_groups.insert(insert_after + 1, index_group)

if path not in index_group.paths:
index_group.paths.append(path)

Comment on lines +627 to +664
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the search results, I can now generate the final response:

Direct manipulation of sidebar groups found in mutation.py

The new utility method _add_path_to_sidebar_group should be used instead of direct manipulation of sidebar_groups found in fiftyone/server/mutation.py. This ensures consistent handling of sidebar group management across the codebase.

  • fiftyone/server/mutation.py: Replace direct assignment of sidebar_groups with calls to _add_path_to_sidebar_group
🔗 Analysis chain

Verify usage pattern in the codebase

Let's ensure this new utility method is being used consistently across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for direct manipulation of sidebar_groups that should use this new utility.
# Test: Search for direct manipulation of sidebar_groups. Expect: No direct manipulation.

# Look for direct manipulation of sidebar_groups
rg -A 5 'sidebar_groups\s*=' --type py

# Look for path manipulation in sidebar groups
rg -A 5 'paths\.append|paths\.remove' --type py

Length of output: 18187


def _make_default_sidebar_groups(sample_collection):
# Possible sidebar groups

Unchanged files with check annotations Beta

clearUseKeyStores(panelId);
trackEvent("close_panel", { panel: panelName });
});
}, []);

Check warning on line 40 in app/packages/operators/src/CustomPanel.tsx

GitHub Actions / lint / eslint

React Hook useEffect has missing dependencies: 'panelId', 'panelName', 'setPanelCloseEffect', and 'trackEvent'. Either include them or remove the dependency array
useEffect(() => {
setLoading(count > 0);
useEffect(() => {
dimensions?.refresh();
}, []);

Check warning on line 105 in app/packages/operators/src/CustomPanel.tsx

GitHub Actions / lint / eslint

React Hook useEffect has a missing dependency: 'dimensions'. Either include it or remove the dependency array
return children;
}
unlisted: true,
});
}
useHooks(ctx: ExecutionContext): {} {

Check warning on line 815 in app/packages/operators/src/built-in-operators.ts

GitHub Actions / lint / eslint

'ctx' is defined but never used. Allowed unused args must match /^_/u
return { updatePanelState: useUpdatePanelStatePartial() };
}
async execute(ctx: ExecutionContext): Promise<void> {
import { test as base } from "src/oss/fixtures";

Check failure on line 1 in e2e-pw/src/oss/specs/groups/dynamic-groups.spec.ts

GitHub Actions / e2e / test-e2e

[chromium] › oss/specs/groups/dynamic-groups.spec.ts:70:3 › pcd dynamic group pagination bar

1) [chromium] › oss/specs/groups/dynamic-groups.spec.ts:70:3 › pcd dynamic group pagination bar ── Test timeout of 60000ms exceeded.

Check failure on line 1 in e2e-pw/src/oss/specs/groups/dynamic-groups.spec.ts

GitHub Actions / e2e / test-e2e

[chromium] › oss/specs/groups/dynamic-groups.spec.ts:70:3 › png dynamic group pagination bar

2) [chromium] › oss/specs/groups/dynamic-groups.spec.ts:70:3 › png dynamic group pagination bar ── Test timeout of 60000ms exceeded.
import { GridPom } from "src/oss/poms/grid";
import { ModalPom } from "src/oss/poms/modal";
import { getUniqueDatasetNameWithPrefix } from "src/oss/utils";
}
async openNthSample(n: number) {
await this.getNthLooker(n).click({ position: { x: 10, y: 80 } });

Check failure on line 55 in e2e-pw/src/oss/poms/grid/index.ts

GitHub Actions / e2e / test-e2e

[chromium] › oss/specs/groups/dynamic-groups.spec.ts:70:3 › pcd dynamic group pagination bar

1) [chromium] › oss/specs/groups/dynamic-groups.spec.ts:70:3 › pcd dynamic group pagination bar ── Error: locator.click: Test timeout of 60000ms exceeded. Call log: - waiting for getByTestId('fo-grid').getByTestId('looker').first() - locator resolved to <div tabindex="-1" data-cy="looker" title="Click to expand" class="_looker_1i9j2_16">…</div> - attempting click action - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker3d" class="_loading_1wcd9_1">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #1 - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker3d" class="_loading_1wcd9_1">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #2 - waiting 20ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker3d" class="_loading_1wcd9_1">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #3 - waiting 100ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker3d" class="_loading_1wcd9_1">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #4 - waiting 100ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker3d" class="_loading_1wcd9_1">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #5 - waiting 500ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker3d" class="_loading_1wcd9_1">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #6 - waiting 500ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker3d" class="_loading_1wcd9_1">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #7 - waiting 500ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker3d" class="_loading_1wcd9_1">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #8 - waiting 500ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker3d" class="_loading_1wcd9_1">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #9 - waiting 500ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker

Check failure on line 55 in e2e-pw/src/oss/poms/grid/index.ts

GitHub Actions / e2e / test-e2e

[chromium] › oss/specs/groups/dynamic-groups.spec.ts:70:3 › png dynamic group pagination bar

2) [chromium] › oss/specs/groups/dynamic-groups.spec.ts:70:3 › png dynamic group pagination bar ── Error: locator.click: Test timeout of 60000ms exceeded. Call log: - waiting for getByTestId('fo-grid').getByTestId('looker').first() - locator resolved to <div tabindex="-1" data-cy="looker" title="Click to expand" class="_looker_1i9j2_16">…</div> - attempting click action - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker-error-info" class="_lookerErrorPage_sfdgp_5">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #1 - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker-error-info" class="_lookerErrorPage_sfdgp_5">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #2 - waiting 20ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker-error-info" class="_lookerErrorPage_sfdgp_5">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #3 - waiting 100ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker-error-info" class="_lookerErrorPage_sfdgp_5">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #4 - waiting 100ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker-error-info" class="_lookerErrorPage_sfdgp_5">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #5 - waiting 500ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker-error-info" class="_lookerErrorPage_sfdgp_5">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #6 - waiting 500ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker-error-info" class="_lookerErrorPage_sfdgp_5">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #7 - waiting 500ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker-error-info" class="_lookerErrorPage_sfdgp_5">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #8 - waiting 500ms - waiting for element to be visible, enabled and stable - element is visible, enabled and stable - scrolling into view if needed - done scrolling - <div data-cy="looker-error-info" class="_lookerErrorPage_sfdgp_5">…</div> from <div id="modal" class="modalon">…</div> subtree intercepts pointer events - retrying click action, attempt #9 - waiting 500ms - waiting for element to be visible, enabled an
}
async openFirstSample() {
}
async isEntryCountTextEqualTo(text: string) {
return this.gridPom.page.waitForFunction(

Check failure on line 149 in e2e-pw/src/oss/poms/grid/index.ts

GitHub Actions / e2e / test-e2e

[chromium] › oss/specs/groups/sparse-groups.spec.ts:96:3 › mp4 second slice

4) [chromium] › oss/specs/groups/sparse-groups.spec.ts:96:3 › mp4 second slice ─────────────────── TimeoutError: page.waitForFunction: Timeout 2000ms exceeded. at oss/poms/grid/index.ts:149 147 | 148 | async isEntryCountTextEqualTo(text: string) { > 149 | return this.gridPom.page.waitForFunction( | ^ 150 | (text_) => { 151 | return ( 152 | document.querySelector("[data-cy='entry-counts']").textContent === at GridAsserter.isEntryCountTextEqualTo (/home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/poms/grid/index.ts:149:30) at /home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/specs/groups/sparse-groups.spec.ts:103:23

Check failure on line 149 in e2e-pw/src/oss/poms/grid/index.ts

GitHub Actions / e2e / test-e2e

[chromium] › oss/specs/groups/sparse-groups.spec.ts:96:3 › png second slice

5) [chromium] › oss/specs/groups/sparse-groups.spec.ts:96:3 › png second slice ─────────────────── TimeoutError: page.waitForFunction: Timeout 2000ms exceeded. at oss/poms/grid/index.ts:149 147 | 148 | async isEntryCountTextEqualTo(text: string) { > 149 | return this.gridPom.page.waitForFunction( | ^ 150 | (text_) => { 151 | return ( 152 | document.querySelector("[data-cy='entry-counts']").textContent === at GridAsserter.isEntryCountTextEqualTo (/home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/poms/grid/index.ts:149:30) at /home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/specs/groups/sparse-groups.spec.ts:103:23
(text_) => {
return (
document.querySelector("[data-cy='entry-counts']").textContent ===
async verifySidebarEntryText(key: string, value: string) {
const text = await this.modalSidebarPom.getSidebarEntryText(key);
expect(text).toBe(value);

Check failure on line 88 in e2e-pw/src/oss/poms/modal/modal-sidebar.ts

GitHub Actions / e2e / test-e2e

[chromium] › oss/specs/groups/nested-dynamic-groups.spec.ts:116:1 › dynamic groups of groups works

3) [chromium] › oss/specs/groups/nested-dynamic-groups.spec.ts:116:1 › dynamic groups of groups works Error: expect(received).toBe(expected) // Object.is equality Expected: "2" Received: "1" at oss/poms/modal/modal-sidebar.ts:88 86 | async verifySidebarEntryText(key: string, value: string) { 87 | const text = await this.modalSidebarPom.getSidebarEntryText(key); > 88 | expect(text).toBe(value); | ^ 89 | } 90 | 91 | async waitUntilSidebarEntryTextEquals(key: string, value: string) { at SidebarAsserter.verifySidebarEntryText (/home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/poms/modal/modal-sidebar.ts:88:18) at SidebarAsserter.verifySidebarEntryTexts (/home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/poms/modal/modal-sidebar.ts:105:5) at /home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/specs/groups/nested-dynamic-groups.spec.ts:157:3
}
async waitUntilSidebarEntryTextEquals(key: string, value: string) {
) {
const actualTitle = await this.modalPom.modalSamplePluginTitle;
const expectedTitle = pinned ? `📌 ${title}` : title;
expect(actualTitle).toBe(expectedTitle);

Check failure on line 314 in e2e-pw/src/oss/poms/modal/index.ts

GitHub Actions / e2e / test-e2e

[chromium] › oss/specs/regression-tests/group-video/group-video-label.spec.ts:87:3 › groups video labels › video plays with correct label for each slice

6) [chromium] › oss/specs/regression-tests/group-video/group-video-label.spec.ts:87:3 › groups video labels › video plays with correct label for each slice Error: expect(received).toBe(expected) // Object.is equality Expected: "📌 v1" Received: "📌 v2" at oss/poms/modal/index.ts:314 312 | const actualTitle = await this.modalPom.modalSamplePluginTitle; 313 | const expectedTitle = pinned ? `📌 ${title}` : title; > 314 | expect(actualTitle).toBe(expectedTitle); | ^ 315 | } 316 | } 317 | at ModalAsserter.verifyModalSamplePluginTitle (/home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/poms/modal/index.ts:314:25) at checkVideo (/home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts:98:7) at /home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts:121:5
}
}
import { test as base } from "src/oss/fixtures";

Check failure on line 1 in e2e-pw/src/oss/specs/regression-tests/index-page.spec.ts

GitHub Actions / e2e / test-e2e

[chromium] › oss/specs/regression-tests/index-page.spec.ts:24:1 › index page

7) [chromium] › oss/specs/regression-tests/index-page.spec.ts:24:1 › index page ────────────────── Test timeout of 60000ms exceeded.
import { PagePom } from "src/oss/poms/page";
import { getUniqueDatasetNameWithPrefix } from "src/oss/utils";
await this.datasetSelector.openResults();
await this.datasetSelector.selectResult(dataset);
}
await this.page.waitForSelector(

Check failure on line 37 in e2e-pw/src/oss/poms/page.ts

GitHub Actions / e2e / test-e2e

[chromium] › oss/specs/regression-tests/index-page.spec.ts:24:1 › index page

7) [chromium] › oss/specs/regression-tests/index-page.spec.ts:24:1 › index page ────────────────── Error: page.waitForSelector: Test timeout of 60000ms exceeded. Call log: - waiting for locator('[data-cy=index-page]') to be visible at oss/poms/page.ts:37 35 | await this.datasetSelector.selectResult(dataset); 36 | } > 37 | await this.page.waitForSelector( | ^ 38 | `[data-cy=${dataset ? "dataset" : "index"}-page]`, 39 | { 40 | state: "visible", at PagePom.loadDataset (/home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/poms/page.ts:37:21) at /home/runner/work/fiftyone/fiftyone/e2e-pw/src/oss/specs/regression-tests/index-page.spec.ts:25:3
`[data-cy=${dataset ? "dataset" : "index"}-page]`,
{
state: "visible",
});
test("modal media field", async ({ grid, fiftyoneLoader, modal, page }) => {
test.skip(

Check warning on line 73 in e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts

GitHub Actions / e2e / test-e2e

Unexpected use of the `.skip()` annotation
true,
"TODO: FIX ME. MODAL SCREENSHOT COMPARISON IS OFF BY ONE-PIXEL"
);
test("change date field visibility works", async ({
eventUtils,
grid,

Check failure on line 74 in e2e-pw/src/oss/specs/regression-tests/sidebar/sidebar-datetime.spec.ts

GitHub Actions / e2e / test-e2e

'grid' is defined but never used
page,
sidebar,
}) => {