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

python panel state stability enhancements #5390

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
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
41 changes: 28 additions & 13 deletions app/packages/core/src/components/MainSpace/MainSpace.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import { SpacesRoot, usePanelsState, useSpaces } from "@fiftyone/spaces";
import { constants, useSessionSpaces } from "@fiftyone/state";
import {
SpaceNodeJSON,
SpacesRoot,
SpaceTree,
usePanelsState,
useSpaces,
} from "@fiftyone/spaces";
import { constants, useSessionSpaces, useUnboundState } from "@fiftyone/state";
import { isEqual, size } from "lodash";
import React, { useEffect, useRef } from "react";

Expand All @@ -13,29 +19,45 @@ function MainSpace() {
sessionSpaces
);
const [panelsState, setPanelsState] = usePanelsState();
const oldSpaces = useRef(spaces);
const oldSpaces = useRef<SpaceTree | SpaceNodeJSON>(spaces);
const oldPanelsState = useRef(panelsState);
const isMounted = useRef(false);
const unboundState = useUnboundState({
spaces,
sessionSpaces,
panelsState,
sessionPanelsState,
updateSpaces,
setPanelsState,
setSessionSpaces,
});

useEffect(() => clearSpaces, [clearSpaces]);

// Update local spaces layout to latest session spaces layout
useEffect(() => {
const { spaces, updateSpaces } = unboundState;
if (!spaces.equals(sessionSpaces)) {
updateSpaces(sessionSpaces);
}
}, [sessionSpaces]);
}, [unboundState, sessionSpaces]);

// Update local panels state to latest session panels state
useEffect(() => {
const { panelsState, setPanelsState } = unboundState;
if (size(sessionPanelsState) && !isEqual(sessionPanelsState, panelsState)) {
setPanelsState(sessionPanelsState);
}
}, [sessionPanelsState]);
}, [unboundState, sessionPanelsState]);

// Update session spaces layout and panels state to latest local spaces layout and panels state
useEffect(() => {
if (!isMounted.current) {
isMounted.current = true;
return;
}
const { sessionSpaces, sessionPanelsState, setSessionSpaces } =
unboundState;
const serializedSpaces = spaces.toJSON();
const spacesUpdated =
!spaces.equals(sessionSpaces) && !spaces.equals(oldSpaces.current);
Expand All @@ -47,14 +69,7 @@ function MainSpace() {
}
oldSpaces.current = serializedSpaces;
oldPanelsState.current = panelsState;
}, [
oldSpaces,
panelsState,
sessionSpaces,
sessionPanelsState,
setSessionSpaces,
spaces,
]);
}, [unboundState, oldSpaces, panelsState, spaces]);

return <SpacesRoot id={FIFTYONE_GRID_SPACES_ID} />;
}
Expand Down
22 changes: 12 additions & 10 deletions app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
import { PluginComponentType, useActivePlugins } from "@fiftyone/plugins";
import { useUnboundState } from "@fiftyone/state";
import { isNullish } from "@fiftyone/utilities";
import { get, isEqual, set } from "lodash";
import { useEffect, useMemo } from "react";
import React, { useEffect, useMemo } from "react";
import { isPathUserChanged } from "../hooks";
import {
getComponent,
getErrorsForView,
isCompositeView,
isEditableView,
isInitialized,
} from "../utils";
import { AncestorsType, SchemaType, ViewPropsType } from "../utils/types";
import ContainerizedComponent from "./ContainerizedComponent";
import { useUnboundState } from "@fiftyone/state";

export default function DynamicIO(props: ViewPropsType) {
const { schema, onChange } = props;
Expand Down Expand Up @@ -73,28 +72,31 @@ function useStateInitializer(props: ViewPropsType) {
const computedSchema = getComputedSchema(props);
const { default: defaultValue } = computedSchema;
const shouldInitialize = useMemo(() => {
return !isCompositeView(computedSchema) && isEditableView(computedSchema);
return !isCompositeView(computedSchema);
}, [computedSchema]);
const basicData = useMemo(() => {
if (shouldInitialize) {
return data;
}
}, [shouldInitialize, data]);
const unboundState = useUnboundState({ computedSchema, props });
const unboundState = useUnboundState({ computedSchema, props, data });

useEffect(() => {
const { computedSchema, props } = unboundState;
const { data, path, root_id } = props || {};
const { data, path, root_id, otherProps = {} } = props || {};
const { updatableDefaultValue = true } = otherProps;
const updateToDefault = updatableDefaultValue ? true : isNullish(data);
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is getting out of hand!

shouldInitialize &&
!isEqual(data, defaultValue) &&
!isPathUserChanged(path, root_id) &&
updateToDefault &&
!isNullish(defaultValue) &&
!isInitialized(props)
!isPathUserChanged(path, root_id) &&
!isInitialized(props) &&
!isEqual(data, defaultValue)
) {
onChange(path, defaultValue, computedSchema);
}
}, [defaultValue, onChange, unboundState]);
}, [shouldInitialize, defaultValue, onChange, unboundState]);

useEffect(() => {
if (basicData) {
Expand Down
22 changes: 20 additions & 2 deletions app/packages/operators/src/CustomPanel.tsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
import { useTrackEvent } from "@fiftyone/analytics";
import { CenteredStack, CodeBlock, scrollable } from "@fiftyone/components";
import { clearUseKeyStores } from "@fiftyone/core/src/plugins/SchemaIO/hooks";
import {
PanelSkeleton,
usePanelLoading,
usePanelState,
useSetPanelCloseEffect,
} from "@fiftyone/spaces";
import * as fos from "@fiftyone/state";
import { Box, Typography } from "@mui/material";
import { useEffect } from "react";
import { useEffect, useRef } from "react";
import OperatorIO from "./OperatorIO";
import { PANEL_LOAD_TIMEOUT } from "./constants";
import { useActivePanelEventsCount } from "./hooks";
import { Property } from "./types";
import { CustomPanelProps, useCustomPanelHooks } from "./useCustomPanelHooks";
import { useTrackEvent } from "@fiftyone/analytics";
import usePanelEvent from "./usePanelEvent";
import { isNullish } from "@fiftyone/utilities";

export function CustomPanel(props: CustomPanelProps) {
const { panelId, dimensions, panelName, panelLabel, isModalPanel } = props;
Expand Down Expand Up @@ -42,7 +44,7 @@
triggerPanelEvent(panelId, { operator: props.onUnLoad });
}
});
}, []);

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

View workflow job for this annotation

GitHub Actions / lint / eslint

React Hook useEffect has missing dependencies: 'panelId', 'panelName', 'props.onUnLoad', 'setPanelCloseEffect', 'trackEvent', and 'triggerPanelEvent'. Either include them or remove the dependency array

useEffect(() => {
setLoading(count > 0);
Expand Down Expand Up @@ -95,6 +97,7 @@
onPathChange={handlePanelStatePathChange}
shouldClearUseKeyStores={false}
isModalPanel={isModalPanel}
updatableDefaultValue={false}
/>
</DimensionRefresher>
</Box>
Expand All @@ -107,7 +110,7 @@

useEffect(() => {
dimensions?.refresh();
}, []);

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

View workflow job for this annotation

GitHub Actions / lint / eslint

React Hook useEffect has a missing dependency: 'dimensions'. Either include it or remove the dependency array

return children;
}
Expand All @@ -128,9 +131,24 @@
on_change_spaces,
panel_name,
panel_label,
reset_state,
}) {
return (props) => {
const { dimensions, panelNode, isModalPanel } = props;
const [state, setState] = usePanelState();
Copy link
Contributor

Choose a reason for hiding this comment

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

There might be some logic in the new consts here that is causing state to reset on every render it seems @imanjra

I was playing around with DQ panel and confirmed that on main & develop state seems to remain, but on your latest branch regardless of if I set reset_state to True or False, state sometimes resets entirely but then goes back to what is expected

I recorded screenshots below of what is doing on with DQ panel on a basic scan

In main/develop (expected behavior):

Panel execution works fine. The state of the vars we set remain persistent, and do not reset/empty out

Screen.Recording.2025-01-27.at.5.33.13.PM.mov

In operator-state-im-x1 (bug):

Panel execution sets a state. The state does not persist and goes back to base (that's why it went back to homescreen), and then upon execution completion, state gets set once more (that's why it goest to Analysis) but then gets reset to base once more

Screen.Recording.2025-01-27.at.5.36.01.PM.mov

const [localState] = usePanelState(null, null, true);
const hasState = !isNullish(state?.state);
const isLoaded = localState?.loaded;
const isReset = useRef(false);

if (!isLoaded && hasState && reset_state) {
if (!isReset.current) {
setState({});
isReset.current = true;
}
return null;
}
Comment on lines +138 to +150
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

Improve state management implementation.

The current implementation has several areas for improvement:

  1. The state check and reset logic could race if multiple renders occur quickly
  2. The localState hook with null values could be simplified
  3. The isReset ref could be replaced with state for better predictability

Consider this implementation:

-    const [state, setState] = usePanelState();
-    const [localState] = usePanelState(null, null, true);
-    const hasState = !isNullish(state?.state);
-    const isLoaded = localState?.loaded;
-    const isReset = useRef(false);
-
-    if (!isLoaded && hasState && reset_state) {
-      if (!isReset.current) {
-        setState({});
-        isReset.current = true;
-      }
-      return null;
-    }
+    const [state, setState] = usePanelState();
+    const [isReset, setIsReset] = useState(false);
+    
+    useEffect(() => {
+      if (reset_state && !isReset && !isNullish(state?.state)) {
+        setState({});
+        setIsReset(true);
+      }
+    }, [reset_state, state?.state, isReset]);
+    
+    if (reset_state && !isReset) {
+      return null;
+    }
📝 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
const [state, setState] = usePanelState();
const [localState] = usePanelState(null, null, true);
const hasState = !isNullish(state?.state);
const isLoaded = localState?.loaded;
const isReset = useRef(false);
if (!isLoaded && hasState && reset_state) {
if (!isReset.current) {
setState({});
isReset.current = true;
}
return null;
}
const [state, setState] = usePanelState();
const [isReset, setIsReset] = useState(false);
useEffect(() => {
if (reset_state && !isReset && !isNullish(state?.state)) {
setState({});
setIsReset(true);
}
}, [reset_state, state?.state, isReset]);
if (reset_state && !isReset) {
return null;
}


return (
<CustomPanel
panelId={panelNode?.id}
Expand Down
4 changes: 4 additions & 0 deletions fiftyone/operators/operations.py
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@ def register_panel(
on_change_query_performance=None,
allow_duplicates=False,
priority=None,
reset_state=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this name is confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree on this, since this is supposed to be about persisting state on refresh, I think more descriptive is better here - something like persist_state_on_refresh or refresh_saving etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree. persist_state_on_refresh sounds more descriptive

_builtin=False,
):
"""Registers a panel with the given name and lifecycle callbacks.
Expand Down Expand Up @@ -371,6 +372,8 @@ def register_panel(
the panel to the opened
priority (None): the priority of the panel, used for sort order
_builtin (False): whether the panel is a builtin panel
reset_state (False): whether to reset the state of the panel prior to on load

"""
params = {
"panel_name": name,
Expand Down Expand Up @@ -398,6 +401,7 @@ def register_panel(
"on_change_query_performance": on_change_query_performance,
"allow_duplicates": allow_duplicates,
"priority": priority,
"reset_state": reset_state,
"_builtin": _builtin,
}
return self._ctx.trigger("register_panel", params=params)
Expand Down
5 changes: 5 additions & 0 deletions fiftyone/operators/panel.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class PanelConfig(OperatorConfig):
tooltip
category (Category): the category id of the panel
priority (None): the priority of the panel for sorting in the UI
reset_state (False): whether to reset the state of the panel prior to on load
"""

def __init__(
Expand All @@ -49,6 +50,7 @@ def __init__(
allow_multiple=False,
surfaces: PANEL_SURFACE = "grid",
priority=None,
reset_state=False,
**kwargs
):
super().__init__(name)
Expand All @@ -66,6 +68,7 @@ def __init__(
self.beta = beta
self.is_new = is_new
self.priority = priority
self.reset_state = reset_state
self.kwargs = kwargs # unused, placeholder for future extensibility

def to_json(self):
Expand All @@ -84,6 +87,7 @@ def to_json(self):
"unlisted": self.unlisted,
"surfaces": self.surfaces,
"priority": self.priority,
"reset_state": self.reset_state,
}


Expand Down Expand Up @@ -126,6 +130,7 @@ def on_startup(self, ctx):
"beta": self.config.beta,
"is_new": self.config.is_new,
"priority": self.config.priority,
"reset_state": self.config.reset_state,
"_builtin": self._builtin,
}
methods = ["on_load", "on_unload", "on_change"]
Expand Down
Loading