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

add spaces context to operators #4902

Merged
merged 3 commits into from
Oct 22, 2024
Merged
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
2 changes: 2 additions & 0 deletions app/packages/operators/src/CustomPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
}, []);

useEffect(() => {
setLoading(count > 0);

Check warning on line 40 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', 'setPanelCloseEffect', and 'trackEvent'. Either include them or remove the dependency array
}, [setLoading, count]);

if (pending && !panelSchema && !onLoadError) {
Expand Down Expand Up @@ -101,7 +101,7 @@
}, []);

return children;
}

Check warning on line 104 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

export function defineCustomPanel({
on_load,
Expand All @@ -115,6 +115,7 @@
on_change_selected_labels,
on_change_extended_selection,
on_change_group_slice,
on_change_spaces,
panel_name,
panel_label,
}) {
Expand All @@ -132,6 +133,7 @@
onChangeSelectedLabels={on_change_selected_labels}
onChangeExtendedSelection={on_change_extended_selection}
onChangeGroupSlice={on_change_group_slice}
onChangeSpaces={on_change_spaces}
dimensions={dimensions}
panelName={panel_name}
panelLabel={panel_label}
Expand Down
6 changes: 6 additions & 0 deletions app/packages/operators/src/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ function useOperatorThrottledContextSetter() {
const groupSlice = useRecoilValue(fos.groupSlice);
const currentSample = useCurrentSample();
const setContext = useSetRecoilState(operatorThrottledContext);
const spaces = useRecoilValue(fos.sessionSpaces);
const workspaceName = spaces._name;
const setThrottledContext = useMemo(() => {
return debounce(
(context) => {
Expand All @@ -49,6 +51,8 @@ function useOperatorThrottledContextSetter() {
currentSample,
viewName,
groupSlice,
spaces,
workspaceName,
});
}, [
setThrottledContext,
Expand All @@ -61,6 +65,8 @@ function useOperatorThrottledContextSetter() {
currentSample,
viewName,
groupSlice,
spaces,
workspaceName,
]);
}

Expand Down
21 changes: 20 additions & 1 deletion app/packages/operators/src/operators.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { AnalyticsInfo, usingAnalytics } from "@fiftyone/analytics";
import { ServerError, getFetchFunction, isNullish } from "@fiftyone/utilities";
import { SpaceNode, spaceNodeFromJSON, SpaceNodeJSON } from "@fiftyone/spaces";
import { getFetchFunction, isNullish, ServerError } from "@fiftyone/utilities";
import { CallbackInterface } from "recoil";
import { QueueItemStatus } from "./constants";
import * as types from "./types";
Expand Down Expand Up @@ -92,6 +93,8 @@ export type RawContext = {
};
groupSlice: string;
queryPerformance?: boolean;
spaces: SpaceNodeJSON;
workspaceName: string;
};

export class ExecutionContext {
Expand Down Expand Up @@ -140,6 +143,12 @@ export class ExecutionContext {
public get queryPerformance(): boolean {
return Boolean(this._currentContext.queryPerformance);
}
public get spaces(): SpaceNode {
return spaceNodeFromJSON(this._currentContext.spaces);
}
public get workspaceName(): string {
return this._currentContext.workspaceName;
}
Comment on lines +146 to +151
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential undefined values in getters

The new getter methods for spaces and workspaceName look good, but they might throw an error if the corresponding properties in _currentContext are undefined.

Consider adding null checks or default values to prevent potential runtime errors:

public get spaces(): SpaceNode {
-  return spaceNodeFromJSON(this._currentContext.spaces);
+  return this._currentContext.spaces ? spaceNodeFromJSON(this._currentContext.spaces) : new SpaceNode();
}

public get workspaceName(): string {
-  return this._currentContext.workspaceName;
+  return this._currentContext.workspaceName || '';
}

This change ensures that the getters always return a valid value, even if the properties are undefined in the context.

📝 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
public get spaces(): SpaceNode {
return spaceNodeFromJSON(this._currentContext.spaces);
}
public get workspaceName(): string {
return this._currentContext.workspaceName;
}
public get spaces(): SpaceNode {
return this._currentContext.spaces ? spaceNodeFromJSON(this._currentContext.spaces) : new SpaceNode();
}
public get workspaceName(): string {
return this._currentContext.workspaceName || '';
}


getCurrentPanelId(): string | null {
return this.params.panel_id || this.currentPanel?.id || null;
Expand Down Expand Up @@ -548,6 +557,8 @@ async function executeOperatorAsGenerator(
view: currentContext.view,
view_name: currentContext.viewName,
group_slice: currentContext.groupSlice,
spaces: currentContext.spaces,
workspace_name: currentContext.workspaceName,
Comment on lines +560 to +561
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

Consider refactoring repeated payload construction

The addition of spaces and workspace_name to the payload in multiple functions is correct and consistent. However, this leads to code duplication across executeOperatorAsGenerator, executeOperatorWithContext, and resolveRemoteType functions.

Consider creating a helper function to construct the common parts of the payload:

function buildCommonPayload(currentContext: RawContext): object {
  return {
    current_sample: currentContext.currentSample,
    dataset_name: currentContext.datasetName,
    delegation_target: currentContext.delegationTarget,
    extended: currentContext.extended,
    extended_selection: currentContext.extendedSelection,
    filters: currentContext.filters,
    selected: currentContext.selectedSamples
      ? Array.from(currentContext.selectedSamples)
      : [],
    selected_labels: formatSelectedLabels(currentContext.selectedLabels),
    view: currentContext.view,
    view_name: currentContext.viewName,
    group_slice: currentContext.groupSlice,
    spaces: currentContext.spaces,
    workspace_name: currentContext.workspaceName,
  };
}

Then use this helper function in each of the affected functions:

const payload = {
  ...buildCommonPayload(currentContext),
  operator_uri: operatorURI,
  params: ctx.params,
  // Add any function-specific properties here
};

This refactoring will reduce code duplication and make it easier to maintain consistency across these functions in the future.

Also applies to: 726-727, 831-832

},
"json-stream"
);
Expand Down Expand Up @@ -712,6 +723,8 @@ export async function executeOperatorWithContext(
view_name: currentContext.viewName,
group_slice: currentContext.groupSlice,
query_performance: currentContext.queryPerformance,
spaces: currentContext.spaces,
workspace_name: currentContext.workspaceName,
}
);
result = serverResult.result;
Expand Down Expand Up @@ -815,6 +828,8 @@ export async function resolveRemoteType(
view: currentContext.view,
view_name: currentContext.viewName,
group_slice: currentContext.groupSlice,
spaces: currentContext.spaces,
workspace_name: currentContext.workspaceName,
}
);

Expand Down Expand Up @@ -889,6 +904,8 @@ export async function resolveExecutionOptions(
view: currentContext.view,
view_name: currentContext.viewName,
group_slice: currentContext.groupSlice,
spaces: currentContext.spaces,
workspace_name: currentContext.workspaceName,
Copy link
Contributor

Choose a reason for hiding this comment

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

I do see that we have both view and view_name here, so it is consistent to have both spaces and workspace_name defined here, and also to use both of them in the implementation of ExecutionContext.spaces 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! This is needed to load a workspace instead of building spaces from raw JSON

}
);

Expand Down Expand Up @@ -920,6 +937,8 @@ export async function fetchRemotePlacements(ctx: ExecutionContext) {
current_sample: currentContext.currentSample,
view_name: currentContext.viewName,
group_slice: currentContext.groupSlice,
spaces: currentContext.spaces,
workspace_name: currentContext.workspaceName,
}
);
if (result && result.error) {
Expand Down
10 changes: 10 additions & 0 deletions app/packages/operators/src/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ const globalContextSelector = selector({
const extendedSelection = get(fos.extendedSelection);
const groupSlice = get(fos.groupSlice);
const queryPerformance = typeof get(fos.lightningThreshold) === "number";
const spaces = get(fos.sessionSpaces);
const workspaceName = spaces?._name;

return {
datasetName,
Expand All @@ -107,6 +109,8 @@ const globalContextSelector = selector({
extendedSelection,
groupSlice,
queryPerformance,
spaces,
workspaceName,
};
},
});
Expand Down Expand Up @@ -148,6 +152,8 @@ const useExecutionContext = (operatorName, hooks = {}) => {
extendedSelection,
groupSlice,
queryPerformance,
spaces,
workspaceName,
} = curCtx;
const [analyticsInfo] = useAnalyticsInfo();
const ctx = useMemo(() => {
Expand All @@ -166,6 +172,8 @@ const useExecutionContext = (operatorName, hooks = {}) => {
analyticsInfo,
groupSlice,
queryPerformance,
spaces,
workspaceName,
},
hooks
);
Expand All @@ -182,6 +190,8 @@ const useExecutionContext = (operatorName, hooks = {}) => {
currentSample,
groupSlice,
queryPerformance,
spaces,
workspaceName,
]);

return ctx;
Expand Down
9 changes: 9 additions & 0 deletions app/packages/operators/src/useCustomPanelHooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ export interface CustomPanelProps {
onChangeSelectedLabels?: string;
onChangeExtendedSelection?: string;
onChangeGroupSlice?: string;
onChangeSpaces?: string;
onChangeWorkspace?: string;
dimensions: DimensionsType | null;
panelName?: string;
panelLabel?: string;
Expand Down Expand Up @@ -136,6 +138,13 @@ export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks {
ctx.groupSlice,
props.onChangeGroupSlice
);
useCtxChangePanelEvent(isLoaded, panelId, ctx.spaces, props.onChangeSpaces);
useCtxChangePanelEvent(
isLoaded,
panelId,
ctx.workspaceName,
props.onChangeWorkspace
);

useEffect(() => {
onLoad();
Expand Down
14 changes: 14 additions & 0 deletions docs/source/plugins/developing_plugins.rst
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,7 @@ contains the following properties:
- `ctx.dataset_name`: the name of the current dataset
- `ctx.dataset` - the current |Dataset| instance
- `ctx.view` - the current |DatasetView| instance
- `ctx.spaces` - the current :ref:`Spaces layout <app-spaces>` in the App
- `ctx.current_sample` - the ID of the active sample in the App modal, if any
- `ctx.selected` - the list of currently selected samples in the App, if any
- `ctx.selected_labels` - the list of currently selected labels in the App,
Expand Down Expand Up @@ -1991,6 +1992,19 @@ subsequent sections.
ctx.panel.set_state("event", "on_change_view")
ctx.panel.set_data("event_data", event)

def on_change_spaces(self, ctx):
"""Implement this method to set panel state/data when the current
spaces layout changes.

The current spaces layout will be available via ``ctx.spaces``.
"""
event = {
"data": ctx.spaces,
"description": "the current spaces layout",
}
ctx.panel.set_state("event", "on_change_spaces")
ctx.panel.set_data("event_data", event)

def on_change_current_sample(self, ctx):
"""Implement this method to set panel state/data when a new sample
is loaded in the Sample modal.
Expand Down
54 changes: 25 additions & 29 deletions fiftyone/operators/builtin.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import fiftyone.core.storage as fos
import fiftyone.operators as foo
import fiftyone.operators.types as types
from fiftyone.core.odm.workspace import default_workspace_factory


class EditFieldInfo(foo.Operator):
Expand Down Expand Up @@ -1939,28 +1940,6 @@ def resolve_input(self, ctx):
),
)

# @todo infer this automatically from current App spaces
spaces_prop = inputs.oneof(
"spaces",
[types.String(), types.Object()],
default=None,
required=True,
label="Spaces",
description=(
"JSON description of the workspace to save: "
"`print(session.spaces.to_json(True))`"
),
view=types.CodeView(),
)

spaces = ctx.params.get("spaces", None)
if spaces is not None:
try:
_parse_spaces(spaces)
except:
spaces_prop.invalid = True
spaces_prop.error_message = "Invalid workspace definition"

name = ctx.params.get("name", None)

if name in workspaces:
Expand All @@ -1979,7 +1958,11 @@ def execute(self, ctx):
color = ctx.params.get("color", None)
spaces = ctx.params.get("spaces", None)

spaces = _parse_spaces(spaces)
curr_spaces = spaces is None
if curr_spaces:
spaces = ctx.spaces
else:
spaces = _parse_spaces(ctx, spaces)

ctx.dataset.save_workspace(
name,
Expand All @@ -1989,6 +1972,9 @@ def execute(self, ctx):
overwrite=True,
)

if curr_spaces:
ctx.ops.set_spaces(name=name)


class EditWorkspaceInfo(foo.Operator):
@property
Expand All @@ -2014,9 +2000,14 @@ def execute(self, ctx):
description = ctx.params.get("description", None)
color = ctx.params.get("color", None)

curr_name = ctx.spaces.name
info = dict(name=new_name, description=description, color=color)

ctx.dataset.update_workspace_info(name, info)

if curr_name is not None and curr_name != new_name:
ctx.ops.set_spaces(name=new_name)


def _edit_workspace_info_inputs(ctx, inputs):
workspaces = ctx.dataset.list_workspaces()
Expand All @@ -2034,10 +2025,10 @@ def _edit_workspace_info_inputs(ctx, inputs):
for key in workspaces:
workspace_selector.add_choice(key, label=key)

# @todo default to current workspace name, if one is currently open
inputs.enum(
"name",
workspace_selector.values(),
default=ctx.spaces.name,
required=True,
label="Workspace",
description="The workspace to edit",
Expand Down Expand Up @@ -2106,7 +2097,7 @@ def resolve_input(self, ctx):
inputs.enum(
"name",
workspace_selector.values(),
default=None,
default=ctx.spaces.name,
required=True,
label="Workspace",
description="The workspace to delete",
Expand All @@ -2127,8 +2118,12 @@ def resolve_input(self, ctx):
def execute(self, ctx):
name = ctx.params["name"]

curr_spaces = name == ctx.spaces.name
ctx.dataset.delete_workspace(name)

if curr_spaces:
ctx.ops.set_spaces(spaces=default_workspace_factory())


class SyncLastModifiedAt(foo.Operator):
@property
Expand Down Expand Up @@ -2292,10 +2287,11 @@ def _get_non_default_frame_fields(dataset):
return schema


def _parse_spaces(spaces):
if isinstance(spaces, dict):
return fo.Space.from_dict(spaces)
return fo.Space.from_json(spaces)
def _parse_spaces(ctx, spaces):
if isinstance(spaces, str):
spaces = json.loads(spaces)

return fo.Space.from_dict(spaces)


BUILTIN_OPERATORS = [
Expand Down
13 changes: 13 additions & 0 deletions fiftyone/operators/executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,19 @@ def has_custom_view(self):
)
return has_stages or has_filters or has_extended

@property
def spaces(self):
"""The current spaces layout in the FiftyOne App."""
workspace_name = self.request_params.get("workspace_name", None)
if workspace_name is not None:
return self.dataset.load_workspace(workspace_name)

spaces_dict = self.request_params.get("spaces", None)
if spaces_dict is not None:
return fo.Space.from_dict(spaces_dict)

return None

@property
def selected(self):
"""The list of selected sample IDs (if any)."""
Expand Down
12 changes: 8 additions & 4 deletions fiftyone/operators/operations.py
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,9 @@ def register_panel(
on_unload=None,
on_change=None,
on_change_ctx=None,
on_change_view=None,
on_change_dataset=None,
on_change_view=None,
on_change_spaces=None,
on_change_current_sample=None,
on_change_selected=None,
on_change_selected_labels=None,
Expand Down Expand Up @@ -342,10 +343,12 @@ def register_panel(
changes
on_change_ctx (None): an operator to invoke when the panel
execution context changes
on_change_view (None): an operator to invoke when the current view
changes
on_change_dataset (None): an operator to invoke when the current
dataset changes
on_change_view (None): an operator to invoke when the current view
changes
on_change_spaces (None): an operator to invoke when the current
spaces layout changes
on_change_current_sample (None): an operator to invoke when the
current sample changes
on_change_selected (None): an operator to invoke when the current
Expand All @@ -372,8 +375,9 @@ def register_panel(
"on_unload": on_unload,
"on_change": on_change,
"on_change_ctx": on_change_ctx,
"on_change_view": on_change_view,
"on_change_dataset": on_change_dataset,
"on_change_view": on_change_view,
"on_change_spaces": on_change_spaces,
"on_change_current_sample": on_change_current_sample,
"on_change_selected": on_change_selected,
"on_change_selected_labels": on_change_selected_labels,
Expand Down
Loading
Loading