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

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
4 changes: 4 additions & 0 deletions app/packages/operators/src/CustomPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
setPanelCloseEffect(() => {
clearUseKeyStores(panelId);
});
}, []);

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

View workflow job for this annotation

GitHub Actions / lint / eslint

React Hook useEffect has missing dependencies: 'panelId' and 'setPanelCloseEffect'. Either include them or remove the dependency array

useEffect(() => {
setLoading(count > 0);
Expand Down Expand Up @@ -98,7 +98,7 @@

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

Check warning on line 101 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 @@ -115,6 +115,8 @@
on_change_selected_labels,
on_change_extended_selection,
on_change_group_slice,
on_change_spaces,
on_change_workspace,
panel_name,
panel_label,
}) {
Expand All @@ -132,6 +134,8 @@
onChangeSelectedLabels={on_change_selected_labels}
onChangeExtendedSelection={on_change_extended_selection}
onChangeGroupSlice={on_change_group_slice}
onChangeSpaces={on_change_spaces}
onChangeWorkspace={on_change_workspace}
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 👍

}
);

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
27 changes: 27 additions & 0 deletions docs/source/plugins/developing_plugins.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1001,6 +1001,7 @@ contains the following properties:
if any
- `ctx.results` - a dict containing the outputs of the `execute()` method, if
it has been called
- `ctx.spaces` - The current workspace or the state of spaces in the app.
- `ctx.hooks` **(JS only)** - the return value of the operator's `useHooks()`
method

Expand Down Expand Up @@ -2060,6 +2061,32 @@ subsequent sections.
ctx.panel.set_state("event", "on_change_group_slice")
ctx.panel.set_data("event_data", event)
def on_change_spaces(self, ctx):
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating the docs! One more place to document ctx.spaces is the Execution context section 😄

"""Implement this method to set panel state/data when the current
state of spaces changes in the app.
The current state of spaces will be available via ``ctx.spaces``.
"""
event = {
"data": ctx.spaces,
"description": "the current state of spaces",
}
ctx.panel.set_state("event", "on_change_spaces")
ctx.panel.set_data("event_data", event)
def on_change_workspace(self, ctx):
Copy link
Contributor

Choose a reason for hiding this comment

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

Potentially this should be deleted if you agree with my inclination to remove the on_change_workspace event.

If it remains, then I believe ctx.workspace here should now be ctx.spaces.name.

"""Implement this method to set panel state/data when the current
workspace changes in the app.
The current workspace will be available via ``ctx.workspace``.
"""
event = {
"data": ctx.workspace,
"description": "the current workspace",
}
ctx.panel.set_state("event", "on_change_workspace")
ctx.panel.set_data("event_data", event)
#######################################################################
# Custom events
# These events are defined by user code above and, just like builtin
Expand Down
13 changes: 7 additions & 6 deletions fiftyone/operators/builtin.py
Original file line number Diff line number Diff line change
Expand Up @@ -1939,18 +1939,18 @@ def resolve_input(self, ctx):
),
)

# @todo infer this automatically from current App spaces
current_spaces = ctx.spaces.to_json(True) if ctx.spaces else None
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, to clarify: here I was thinking that, now that we have ctx.spaces, we can just delete the spaces parameter entirely from resolve_input() and just use ctx.spaces in execute().

Pasting in a spaces configuration as JSON was not something I wanted to support, it was just a placeholder until ctx.spaces was available 😄

To support programmatic execution, we could still allow the spaces parameter to be optionally provided like so:

def execute(self, ctx):
    spaces = ctx.params.get("spaces", None)

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

spaces_prop = inputs.oneof(
"spaces",
[types.String(), types.Object()],
default=None,
default=current_spaces,
required=True,
label="Spaces",
description=(
"JSON description of the workspace to save: "
"`print(session.spaces.to_json(True))`"
),
view=types.CodeView(),
view=types.CodeView(language="json"),
)

spaces = ctx.params.get("spaces", None)
Expand Down Expand Up @@ -2034,11 +2034,12 @@ 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
current_workspace = ctx.spaces.name if ctx.spaces else None
inputs.enum(
"name",
workspace_selector.values(),
required=True,
default=current_workspace,
label="Workspace",
description="The workspace to edit",
view=workspace_selector,
Expand Down Expand Up @@ -2102,11 +2103,11 @@ def resolve_input(self, ctx):
workspace_selector = types.AutocompleteView()
for key in workspaces:
workspace_selector.add_choice(key, label=key)

current_workspace = ctx.spaces.name if ctx.spaces else None
inputs.enum(
"name",
workspace_selector.values(),
default=None,
default=current_workspace,
required=True,
label="Workspace",
description="The workspace to delete",
Expand Down
12 changes: 12 additions & 0 deletions fiftyone/operators/executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,18 @@ def query_performance(self):
"""Whether query performance is enabled."""
return self.request_params.get("query_performance", None)

@property
def spaces(self):
"""The current workspace or the state of spaces in the 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 None:
return None
return fo.Space.from_dict(spaces_dict)

def prompt(
self,
operator_uri,
Expand Down
2 changes: 2 additions & 0 deletions fiftyone/operators/operations.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,8 @@ def register_panel(
current extended selection changes
on_change_group_slice (None): an operator to invoke when the group
slice changes
on_change_spaces (None): an operator to invoke when the current spaces changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Is on_change_workspace needed? Comparing with view-related events for context, we only have on_change_view, not on_change_view_name.

on_change_spaces should fire every time that on_change_workspace fires, plus it would also fire when changing between two different unsaved spaces configurations.

on_change_workspace (None): an operator to invoke when the current workspace changes
allow_duplicates (False): whether to allow multiple instances of
the panel to the opened
"""
Expand Down
2 changes: 2 additions & 0 deletions fiftyone/operators/panel.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ def on_startup(self, ctx):
"on_change_selected_labels",
"on_change_extended_selection",
"on_change_group_slice",
"on_change_spaces",
"on_change_workspace",
]
for method in methods + ctx_change_events:
if hasattr(self, method) and callable(getattr(self, method)):
Expand Down
Loading