-
Notifications
You must be signed in to change notification settings - Fork 552
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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"; | ||
|
@@ -92,6 +93,8 @@ export type RawContext = { | |
}; | ||
groupSlice: string; | ||
queryPerformance?: boolean; | ||
spaces: SpaceNodeJSON; | ||
workspaceName: string; | ||
}; | ||
|
||
export class ExecutionContext { | ||
|
@@ -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; | ||
} | ||
|
||
getCurrentPanelId(): string | null { | ||
return this.params.panel_id || this.currentPanel?.id || null; | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider refactoring repeated payload construction The addition of 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" | ||
); | ||
|
@@ -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; | ||
|
@@ -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, | ||
} | ||
); | ||
|
||
|
@@ -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, | ||
} | ||
); | ||
|
||
|
@@ -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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for updating the docs! One more place to document |
||
"""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): | ||
"""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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential undefined values in getters
The new getter methods for
spaces
andworkspaceName
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:
This change ensures that the getters always return a valid value, even if the properties are undefined in the context.
📝 Committable suggestion