add spaces context to operators #4902
Conversation
WalkthroughThe changes in this pull request enhance the functionality of the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
imanjra
force-pushed
the
feat/ctx-spaces
branch
from
65d88cf
to
2c1e4c5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
app/packages/operators/src/CustomPanel.tsx (2)
118-119: LGTM! Consider adding JSDoc comments for new parameters.The addition of
on_change_spacesandon_change_workspaceparameters to thedefineCustomPanelfunction and their subsequent passing to theCustomPanelcomponent is well-implemented and aligns with the PR objectives. The changes follow the existing code structure and naming conventions.To improve code documentation, consider adding JSDoc comments for these new parameters. For example:
/** * @param {Function} on_change_spaces - Callback function triggered when spaces change * @param {Function} on_change_workspace - Callback function triggered when workspace changes */Also applies to: 137-138
Line range hint
1-143: Overall changes look good and align with PR objectives.The modifications to
defineCustomPanelfunction successfully extend its interface to include space and workspace change handlers. These changes are well-integrated and maintain consistency with the existing code structure. The core functionality of theCustomPanelcomponent remains intact, which is a positive aspect of this implementation.As the application evolves to include more context-related parameters, consider refactoring the parameter list into a configuration object in the future. This would make the function more scalable and easier to maintain. For example:
defineCustomPanel({ handlers: { onLoad, onChange, // ... other handlers }, panelConfig: { name: panel_name, label: panel_label, } })This structure would be more flexible for future additions and easier to read.
app/packages/operators/src/useCustomPanelHooks.ts (2)
29-30: LGTM with a minor suggestion for improvementThe addition of
onChangeSpacesandonChangeWorkspaceproperties to theCustomPanelPropsinterface is consistent with the existing pattern and naming convention. This change enhances the flexibility of the custom panel by allowing it to respond to changes in spaces and workspace contexts.For improved type safety, consider using a more specific type for these properties, such as a function type that matches the expected event handler signature. For example:
onChangeSpaces?: (spaces: SpacesType) => void; onChangeWorkspace?: (workspace: string) => void;This would provide better type checking and autocompletion for users of this interface.
141-147: LGTM with a suggestion for improved readabilityThe addition of
useCtxChangePanelEventcalls forctx.spacesandctx.workspaceNameis consistent with the existing pattern and correctly implements the new functionality for handling changes in spaces and workspace contexts.To improve readability and reduce repetition, consider refactoring the
useCtxChangePanelEventcalls into a more concise format. For example:const contextChanges = [ { value: ctx.spaces, handler: props.onChangeSpaces }, { value: ctx.workspaceName, handler: props.onChangeWorkspace }, // ... other existing context changes ]; contextChanges.forEach(({ value, handler }) => { useCtxChangePanelEvent(isLoaded, panelId, value, handler); });This refactoring would make it easier to add or modify context change handlers in the future while keeping the code DRY.
fiftyone/operators/panel.py (1)
129-130: LGTM! Consider grouping related events for better readability.The addition of
"on_change_spaces"and"on_change_workspace"events aligns well with the PR objectives. This change will enable panels to respond to changes in spaces and workspaces, enhancing the operator context as intended.For improved readability, consider grouping related events together. You could move these new events next to other context-related events like
"on_change_ctx". Here's a suggested reordering:ctx_change_events = [ "on_change_ctx", "on_change_spaces", "on_change_workspace", "on_change_view", "on_change_dataset", # ... (other events) ]This grouping makes it easier to understand the different types of context changes at a glance.
fiftyone/operators/operations.py (1)
359-360: LGTM! Consider updating theparamsdictionary.The addition of
on_change_spacesandon_change_workspaceparameters aligns well with the PR objectives and follows the existing naming conventions for event handlers. The docstring updates clearly describe the new parameters.Consider updating the
paramsdictionary in the method body to include these new parameters, ensuring they are properly passed along to the trigger function:params = { "panel_name": name, "panel_label": label, # ... other existing parameters ... "on_change_group_slice": on_change_group_slice, + "on_change_spaces": on_change_spaces, + "on_change_workspace": on_change_workspace, "allow_duplicates": allow_duplicates, }This will ensure that the new parameters are properly utilized when the panel is registered.
app/packages/operators/src/state.ts (1)
193-194: LGTM: Dependency array updated correctly.The addition of
spacesandworkspaceNameto the dependency array of theuseMemohook ensures that theExecutionContextis properly updated when these values change. This is crucial for maintaining consistency and is in line with React best practices.Consider memoizing the
spacesandworkspaceNamevalues if they are derived from complex computations to optimize performance. For example:const memoizedSpaces = useMemo(() => spaces, [JSON.stringify(spaces)]); const memoizedWorkspaceName = useMemo(() => workspaceName, [workspaceName]);Then use these memoized values in the
ExecutionContextconstructor and the dependency array.fiftyone/operators/executor.py (1)
729-735: LGTM: New 'workspace' property added to ExecutionContextThe new 'workspace' property is a valuable addition to the ExecutionContext class. It provides a convenient way to access the current workspace state in the app and correctly handles the case when workspace_name is not present in the request_params.
Consider adding a check for self.dataset before calling get_workspace_info to prevent potential AttributeError if the dataset is not available. Here's a suggested improvement:
@property def workspace(self): """The current workspace state in the app.""" workspace_name = self.request_params.get("workspace_name", None) if workspace_name is None: return None - return self.dataset.get_workspace_info(workspace_name) + return self.dataset.get_workspace_info(workspace_name) if self.dataset else None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- app/packages/operators/src/CustomPanel.tsx (2 hunks)
- app/packages/operators/src/hooks.ts (3 hunks)
- app/packages/operators/src/operators.ts (8 hunks)
- app/packages/operators/src/state.ts (5 hunks)
- app/packages/operators/src/useCustomPanelHooks.ts (2 hunks)
- docs/source/plugins/developing_plugins.rst (1 hunks)
- fiftyone/operators/executor.py (1 hunks)
- fiftyone/operators/operations.py (1 hunks)
- fiftyone/operators/panel.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/operators/src/CustomPanel.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/operators/src/hooks.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/operators/src/operators.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/operators/src/state.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/operators/src/useCustomPanelHooks.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (9)
app/packages/operators/src/hooks.ts (1)
31-32: LGTM! Consider adding a comment forworkspaceName.The addition of
spacesandworkspaceNameto the operator context aligns well with the PR objectives. The implementation is consistent with the existing code patterns.Consider adding a brief comment explaining the derivation of
workspaceNamefromspaces._namefor better code clarity.Verify the usage of new context properties.
Ensure that the new
spacesandworkspaceNameproperties added to the context are properly utilized in the components or functions that consume this context.Run the following script to verify the usage:
Also applies to: 54-55, 68-69
app/packages/operators/src/useCustomPanelHooks.ts (1)
Line range hint
1-247: Summary: Effective implementation of spaces and workspace context supportThe changes in this file successfully implement support for handling spaces and workspace context changes in the custom panel functionality. The modifications are well-integrated into the existing code structure and follow established patterns.
Key points:
- The
CustomPanelPropsinterface has been extended with new properties for space and workspace change handlers.- The
useCustomPanelHooksfunction now includes logic to trigger events for space and workspace context changes.- The implementation is consistent with existing code and maintains good TypeScript and React practices.
These changes enhance the flexibility and functionality of the custom panel system, allowing it to respond to a broader range of context changes within the application.
app/packages/operators/src/state.ts (2)
98-99: LGTM: New context properties added correctly.The addition of
spacesandworkspaceNameto theglobalContextSelectorenhances the context information available to operators. This change aligns well with the PR objectives and follows the existing code pattern.Also applies to: 112-113
155-156: LGTM: Execution context updated with new properties.The
useExecutionContextfunction has been correctly updated to include the newspacesandworkspaceNameproperties. This ensures that the execution context has access to the newly added session information, which is crucial for the enhanced operator context functionality.Also applies to: 175-176
fiftyone/operators/executor.py (2)
721-727: LGTM: New 'spaces' property added to ExecutionContextThe new 'spaces' property is a well-implemented addition to the ExecutionContext class. It correctly handles the case when spaces are not present in the request_params and provides a convenient way to access the current state of spaces in the app. The conversion from dictionary to fo.Space object ensures type consistency.
721-735: Summary: Valuable additions to ExecutionContext for spaces and workspace supportThe changes in this file enhance the ExecutionContext class by adding two new properties: 'spaces' and 'workspace'. These additions align well with the PR objectives of incorporating spaces and workspace functionalities into the operator context.
- The 'spaces' property provides access to the current state of spaces in the app, converting the data to a fo.Space object for consistency.
- The 'workspace' property allows access to the current workspace state, utilizing the dataset object to retrieve workspace information.
These changes will improve the contextual information available to operators, potentially enhancing their functionality and user experience within the FiftyOne application.
docs/source/plugins/developing_plugins.rst (1)
2063-2088: New methods added to handle space and workspace changesTwo new methods have been added to the
ExamplePanelclass to handle changes in spaces and workspaces:
on_change_spaces(self, ctx): This method is called when the current state of spaces changes in the app.on_change_workspace(self, ctx): This method is called when the current workspace changes in the app.Both methods follow the existing pattern of updating the panel's state and data with the new information. This addition enhances the panel's ability to respond to changes in the app's structure and organization.
The new methods are well-documented and consistent with the existing code style. They provide clear examples of how to handle space and workspace changes in a panel.
app/packages/operators/src/operators.ts (2)
2-2: Importing necessary modules from@fiftyone/spacesThe new import statement correctly includes
SpaceNode,spaceNodeFromJSON, andSpaceNodeJSONfrom@fiftyone/spaces, which are utilized in the subsequent code changes.
96-97: AddingspacesandworkspaceNametoRawContextextends context appropriatelyThe addition of
spaces: SpaceNodeJSONandworkspaceName: stringto theRawContexttype effectively incorporatesspacesandworkspaceinformation into the execution context.
| public get spaces(): SpaceNode { | ||
| return spaceNodeFromJSON(this._currentContext.spaces); | ||
| } | ||
| public get workspaceName(): string { | ||
| return this._currentContext.workspaceName; | ||
| } |
There was a problem hiding this comment.
Handle potential undefined spaces and workspaceName in getters
Consider ensuring that this._currentContext.spaces and this._currentContext.workspaceName are defined before usage to prevent possible runtime errors due to undefined values.
You might add default values or null checks as follows:
public get spaces(): SpaceNode {
return this._currentContext.spaces
? spaceNodeFromJSON(this._currentContext.spaces)
: new SpaceNode(); // Provide an appropriate default or handle the undefined case
}
public get workspaceName(): string {
return this._currentContext.workspaceName || '';
}| spaces: currentContext.spaces, | ||
| workspace_name: currentContext.workspaceName, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated request payload construction into a helper function
The construction of the request payload, including spaces and workspace_name, is duplicated across multiple functions. To enhance maintainability and reduce code duplication, consider abstracting the common parameters into a shared helper function.
For example, you could create a utility function:
function buildRequestPayload(
context: RawContext,
additionalParams: object = {}
): object {
return {
current_sample: context.currentSample,
dataset_name: context.datasetName,
delegation_target: context.delegationTarget,
extended: context.extended,
extended_selection: context.extendedSelection,
filters: context.filters,
group_slice: context.groupSlice,
operator_uri: operatorURI,
params: ctx.params,
request_delegation: ctx.requestDelegation,
selected: context.selectedSamples ? Array.from(context.selectedSamples) : [],
selected_labels: formatSelectedLabels(context.selectedLabels),
view: context.view,
view_name: context.viewName,
query_performance: context.queryPerformance,
spaces: context.spaces,
workspace_name: context.workspaceName,
...additionalParams,
};
}Then, use this helper in your functions:
const payload = buildRequestPayload(currentContext, {
results: results ? results.result : null,
});
// Use payload in your fetch functionAlso applies to: 726-727, 831-832, 907-908, 940-941
There was a problem hiding this comment.
@imanjra thanks for adding this! 🙌
What do you think about deleting ctx.workspace from the Python interface in favor of just ensuring that, if the current spaces are a saved workspace, then ctx.spaces.name will be set to the current workspace name? This is how session.spaces.name works btw. It would also be consistent with how saved views work: we have ctx.view with ctx.view.name populated when the current view is saved.
Can you also add ctx.ops.set_spaces(spaces=spaces, name=name) to programmatically load a spaces configuration or a saved workspace by name? Again for consistency with saved views.
And one more thing: can you update the builtin operators to remove the @todos:
@voxel51/save_workspace: remove this and instead just use the currentctx.spacesinexecute()@voxel51/edit_workspace_info: usedefault=ctx.spaces.namefor thenameprop@voxel51/delete_workspace: usedefault=ctx.spaces.namefor thenameprop
| @@ -2060,6 +2060,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.
Thanks for updating the docs! One more place to document ctx.spaces is the Execution context section 😄
|
@brimoor great suggestions! I'll make the changes to merge the two and update the docs. Thank you! |
What changes are proposed in this pull request?
spacesandworkspaceto operator contextHow is this patch tested? If it is not, please explain why.
Using a test panel and operator
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
See above
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Release Notes
New Features
CustomPanelto handle changes in spaces and workspace contexts.ExecutionContextfor improved context management during operator execution.register_panelmethod to respond to changes in spaces and workspace.Documentation
These updates improve user interaction and flexibility within the application.