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.
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! |
imanjra
force-pushed
the
feat/ctx-spaces
branch
2 times, most recently
from
40c472d
to
e5a2dc1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
app/packages/operators/src/state.ts (1)
155-156: LGTM: Execution context updated with spaces and workspace information.The changes correctly incorporate
spacesandworkspaceNameinto the execution context, ensuring that operators have access to this session-specific information. The dependency array update is crucial for proper memoization.Consider using object destructuring in the dependency array for a more concise representation:
], [ - params, - datasetName, - view, - extended, - filters, - selectedSamples, - selectedLabels, - hooks, - viewName, - currentSample, - groupSlice, - queryPerformance, - spaces, - workspaceName, + ...Object.values({ params, datasetName, view, extended, filters, selectedSamples, selectedLabels, hooks, viewName, currentSample, groupSlice, queryPerformance, spaces, workspaceName }), ]);This change would make the code more maintainable as it automatically includes all properties without the need to manually update the array when new properties are added.
Also applies to: 175-176, 193-194
app/packages/operators/src/operators.ts (1)
146-151: LGTM! New getters for spaces and workspaceName.The addition of getter methods for
spacesandworkspaceNamein theExecutionContextclass provides appropriate access to the new context properties. This change is consistent with the PR objectives.Consider adding null checks or default values to handle cases where these properties might be undefined:
public get spaces(): SpaceNode | null { return this._currentContext.spaces ? spaceNodeFromJSON(this._currentContext.spaces) : null; } public get workspaceName(): string { return this._currentContext.workspaceName || ''; }This would prevent potential runtime errors if these properties are accessed when undefined.
fiftyone/operators/builtin.py (3)
1942-1953: LGTM! Improved user experience for workspace saving.The changes to the
spacesinput in theSaveWorkspaceclass are well-implemented:
- Pre-populating the input with the current workspace configuration (
ctx.spaces.to_json(True)) improves usability.- Using
CodeViewwith JSON language specification enhances readability and editing capabilities.These modifications will make it easier for users to save and edit workspace configurations.
Consider adding a comment explaining the purpose of
to_json(True)for future maintainers, e.g.:# Convert spaces to JSON with `pretty=True` for better readability current_spaces = ctx.spaces.to_json(True) if ctx.spaces else None
2106-2110: LGTM! Consistent improvement for workspace deletion.The change to set the default value of the
nameinput toctx.spaces.name if ctx.spaces else Nonein theDeleteWorkspaceclass is a good improvement:
- It pre-selects the current workspace name when deleting a workspace.
- This change maintains consistency with the similar modification in the
EditWorkspaceInfoclass.- It enhances the overall user experience by providing context-aware defaults across workspace-related operations.
Consider adding a confirmation step or warning message when deleting the currently active workspace, as this could have significant implications for the user's current session. For example:
if name == current_workspace: inputs.view( "warning", types.Notice( label="Warning: You are about to delete the currently active workspace", description="This action may affect your current session." ), )
Line range hint
1942-2110: Overall, excellent improvements to workspace management functionality!The changes made to the
SaveWorkspace,EditWorkspaceInfo, andDeleteWorkspaceclasses collectively enhance the user experience for workspace-related operations:
- Pre-populating inputs with current workspace data improves usability.
- Consistent use of
ctx.spaces.nameas the default value across classes maintains a cohesive user interface.- The use of
CodeViewwith JSON language specification inSaveWorkspaceenhances readability and editing capabilities.These modifications will make workspace management more intuitive and efficient for users. The consistency across different operations is particularly commendable, as it reduces cognitive load for users interacting with various workspace functions.
As the workspace management features continue to evolve, consider creating a shared utility function for getting the current workspace name. This would centralize the logic and make future updates easier:
def get_current_workspace_name(ctx): return ctx.spaces.name if ctx.spaces else NoneThis function could then be used across all workspace-related classes, further enhancing consistency and maintainability.
docs/source/plugins/developing_plugins.rst (2)
2064-2075: LGTM! Consider enhancing the docstring.The implementation of
on_change_spaceslooks good and follows the established pattern for context change handlers in this class. It correctly updates the panel state and data when the state of spaces changes.To improve clarity, consider adding a brief explanation of what "spaces" refers to in the context of the FiftyOne app in the docstring.
def on_change_spaces(self, ctx): """Implement this method to set panel state/data when the current - state of spaces changes in the app. + state of spaces changes in the app. Spaces refer to the different + workspaces or contexts within the FiftyOne application. 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)
2077-2089: LGTM! Consider enhancing the docstring.The implementation of
on_change_workspaceis well-done and consistent with other context change handlers in this class. It properly updates the panel state and data when the current workspace changes.To improve clarity, consider adding a brief explanation of what a "workspace" represents in the context of the FiftyOne app in the docstring.
def on_change_workspace(self, ctx): """Implement this method to set panel state/data when the current - workspace changes in the app. + workspace changes in the app. A workspace represents a specific + configuration or view within the FiftyOne application. 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
- 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 (2 hunks)
- fiftyone/operators/builtin.py (3 hunks)
- fiftyone/operators/executor.py (1 hunks)
- fiftyone/operators/operations.py (1 hunks)
- fiftyone/operators/panel.py (1 hunks)
- fiftyone/server/decorators.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- app/packages/operators/src/CustomPanel.tsx
- app/packages/operators/src/hooks.ts
- app/packages/operators/src/useCustomPanelHooks.ts
- fiftyone/operators/executor.py
- fiftyone/operators/operations.py
- fiftyone/operators/panel.py
🧰 Additional context used
📓 Path-based instructions (2)
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.
🔇 Additional comments (12)
fiftyone/server/decorators.py (1)
37-38: Excellent addition of explicit media type!The addition of
media_type="application/json"to theResponseobject is a great improvement. It explicitly sets the content type of the response, which ensures that clients correctly interpret the response as JSON. This change adheres to best practices for HTTP responses and enhances the clarity of the API.app/packages/operators/src/state.ts (2)
98-99: LGTM: Enhanced global context with spaces and workspace information.The addition of
spacesandworkspaceNameto the global context aligns well with the PR objectives. This change will provide valuable session-specific information to operators and other components that rely on the global context.Also applies to: 112-113
Line range hint
1-1168: Consider utilizing new context information in other relevant parts of the file.The changes to incorporate
spacesandworkspaceNameare well-implemented in the global and execution contexts. However, there might be opportunities to leverage this new information in other parts of the file. For example, consider if any operator-related functions or state management logic could benefit from access to space or workspace data.To identify potential areas for improvement, we can search for usage of similar context properties:
Review the results to determine if any of these areas could be enhanced with the new
spacesorworkspaceNameinformation.✅ Verification successful
To further evaluate potential enhancements using
spacesorworkspaceName, let's examine the contexts wheredatasetNameandviewNameare utilized.
To thoroughly assess the potential for utilizing
spacesorworkspaceNamein the identified areas, let's examine the surrounding contexts of each usage ofdatasetNameandviewName.
Verified: The
spacesandworkspaceNamecontexts are appropriately utilized within theglobalContextSelectoranduseExecutionContextfunctions. No further areas in the file require additional integration of these contexts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of existing context properties that might benefit from spaces or workspace information rg -n '(datasetName|viewName)' app/packages/operators/src/state.tsLength of output: 328
Script:
#!/bin/bash # Extract the lines with datasetName and viewName along with their surrounding context rg -C 2 '(datasetName|viewName)' app/packages/operators/src/state.tsLength of output: 1190
Script:
#!/bin/bash # Extract detailed surrounding context for each occurrence of datasetName and viewName rg -C 5 '(datasetName|viewName)' app/packages/operators/src/state.tsLength of output: 2038
app/packages/operators/src/operators.ts (6)
1-3: LGTM! New imports for spaces functionality.The addition of
SpaceNode,spaceNodeFromJSON, andSpaceNodeJSONimports from "@fiftyone/spaces" indicates the introduction of spaces-related functionality to the operators. This aligns with the PR objectives of incorporating spaces context into operators.
96-97: LGTM! RawContext extended with spaces and workspace.The addition of
spaces: SpaceNodeJSONandworkspaceName: stringto theRawContexttype aligns perfectly with the PR objectives. This change enables the incorporation of spaces and workspace context into the operator execution flow.
560-561: LGTM! Spaces and workspace context added to generator execution.The addition of
spacesandworkspace_nameto the payload inexecuteOperatorAsGeneratorfunction ensures that the spaces and workspace context is properly included in the generator execution request. This change is consistent with the PR objectives and earlier modifications to the context.
726-727: LGTM! Consistent addition of spaces and workspace context.The addition of
spacesandworkspace_nameto the payload inexecuteOperatorWithContextfunction maintains consistency with the changes made inexecuteOperatorAsGenerator. This ensures that the spaces and workspace context is included in all operator execution methods, aligning with the PR objectives.
831-832: LGTM! Spaces and workspace context added to remote type resolution.The addition of
spacesandworkspace_nameto the payload inresolveRemoteTypefunction demonstrates a comprehensive application of the new context. This change ensures that the spaces and workspace information is available when resolving remote types, maintaining consistency with other parts of the operator execution flow.
940-941: LGTM! Comprehensive integration of spaces and workspace context.The addition of
spacesandworkspace_nameto the payload infetchRemotePlacementsfunction completes the integration of the new context across all relevant parts of the operator execution flow. This change, along with the previous modifications, ensures that the spaces and workspace information is consistently available throughout the entire process of operator execution, type resolution, and placement fetching.These changes collectively fulfill the PR objectives of incorporating spaces and workspace functionalities into the operator context. The implementation is thorough and consistent, which should greatly enhance the contextual awareness of operators within the FiftyOne application.
fiftyone/operators/builtin.py (1)
2037-2042: LGTM! Enhanced default selection for workspace editing.The modification to set the default value of the
nameinput toctx.spaces.name if ctx.spaces else Noneis a good improvement. This change:
- Pre-selects the current workspace name when editing workspace info.
- Provides a more intuitive user experience by aligning with the context of the current workspace.
- Maintains consistency with similar context-aware defaults implemented elsewhere in the file.
This small enhancement will make the workspace editing process more user-friendly and efficient.
docs/source/plugins/developing_plugins.rst (2)
1004-1005: LGTM! Consistent additions to ExecutionContext.The new properties
ctx.spacesandctx.workspacehave been correctly added to theExecutionContextclass description. These additions align well with the newly implementedon_change_spacesandon_change_workspacemethods, providing developers with access to the current state of spaces and the active workspace.
Line range hint
1-2089: LGTM! Past comment addressed and overall changes look good.The additions of
on_change_spacesandon_change_workspacemethods, along with the correspondingctx.spacesandctx.workspaceproperties in theExecutionContext, have been implemented consistently throughout the file. These changes enhance the plugin's ability to respond to changes in the FiftyOne app's workspace and space contexts.The past review comment requesting documentation for
ctx.spaceshas been adequately addressed with the addition of thectx.spacesproperty in theExecutionContextdescription.Overall, these changes improve the plugin's functionality and documentation, making it more robust and easier for developers to work with spaces and workspaces in the FiftyOne app.
fiftyone/server/decorators.py
Outdated
| await run_sync_task(lambda: json_util.dumps(response, cls=Encoder)), | ||
| media_type="application/json", |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Usage of media_type in Response Creations
The verification script identified multiple instances where Response( is used without an explicit media_type. For consistency and clarity in API responses, it's recommended to explicitly set the media_type or use JSONResponse where appropriate.
Affected Files:
tests/unittests/server_decorators_tests.pyfiftyone/operators/server.pyfiftyone/server/aggregate.pyfiftyone/server/decorators.pyfiftyone/server/routes/events.pyfiftyone/server/routes/screenshot.pyfiftyone/server/routes/frames.pyfiftyone/server/app.pyfiftyone/server/routes/media.py
🔗 Analysis chain
Verify consistency of media type usage across the codebase
While this change is beneficial, it's important to ensure consistency across the entire codebase. Consider reviewing other response creation points to verify if they also explicitly set the media type.
To check for consistency, you can run the following script:
This will help identify any inconsistencies in how responses are created throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for Response object creations without explicit media_type
# Test: Look for Response creations without media_type
rg --type python 'Response\(' -A 3 | rg -v 'media_type'
# Test: Look for JSONResponse creations (which implicitly set media_type to application/json)
rg --type python 'JSONResponse\('
Length of output: 153
Script:
#!/bin/bash
# Description: Search for Response object creations without explicit media_type
# Test: Look for Response creations without media_type
rg --type py 'Response\(' -A 3 | rg -v 'media_type'
# Test: Look for JSONResponse creations (which implicitly set media_type to application/json)
rg --type py 'JSONResponse\('
Length of output: 7194
imanjra
force-pushed
the
feat/ctx-spaces
branch
from
e5a2dc1
to
041a2b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/packages/operators/src/operators.ts (2)
2-3: LGTM! Consider grouping related imports.The addition of
SpaceNode,spaceNodeFromJSON, andSpaceNodeJSONimports from "@fiftyone/spaces" is appropriate for the new functionality. TheRawContexttype has been correctly updated to includespacesandworkspaceName.Consider grouping related imports together. You could move the new import statement next to other FiftyOne-related imports for better organization.
Also applies to: 96-97
940-941: LGTM! Consider adding JSDoc commentsThe addition of
spacesandworkspace_nameto thefetchRemotePlacementsfunction is consistent with the changes made elsewhere in the file. This completes the integration of spaces and workspace functionality across the relevant parts of the codebase.Consider adding JSDoc comments to the functions that have been modified, explaining the new
spacesandworkspace_nameparameters. This will help other developers understand the purpose and usage of these new properties. For example:/** * Fetches remote placements based on the current context. * @param ctx The execution context * @returns A promise that resolves to an array of placement objects */ export async function fetchRemotePlacements(ctx: ExecutionContext) { // ... existing implementation ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- 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 (2 hunks)
- fiftyone/operators/builtin.py (3 hunks)
- fiftyone/operators/executor.py (1 hunks)
- fiftyone/operators/operations.py (1 hunks)
- fiftyone/operators/panel.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- app/packages/operators/src/CustomPanel.tsx
- app/packages/operators/src/hooks.ts
- app/packages/operators/src/state.ts
- app/packages/operators/src/useCustomPanelHooks.ts
- fiftyone/operators/builtin.py
- fiftyone/operators/executor.py
- fiftyone/operators/operations.py
- fiftyone/operators/panel.py
🧰 Additional context used
📓 Path-based instructions (1)
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.
🔇 Additional comments (2)
docs/source/plugins/developing_plugins.rst (2)
2064-2089: LGTM! New methods for handling spaces and workspace changes.The addition of
on_change_spacesandon_change_workspacemethods enhances the panel's ability to respond to changes in the FiftyOne app's spaces and workspace. These methods are well-implemented and consistent with the existing structure.
1004-1004: Documentation forctx.spacesadded as suggested.The
ctx.spacesproperty has been properly documented in the Execution context section, addressing the suggestion from the previous review. This addition improves the completeness of the documentation.
| spaces: currentContext.spaces, | ||
| workspace_name: currentContext.workspaceName, |
There was a problem hiding this comment.
🛠️ 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
| 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 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.
| 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 || ''; | |
| } |
fiftyone/operators/operations.py
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed. Removing on_change_workspace as it's little redundant
| @@ -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, | |||
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
Yup! This is needed to load a workspace instead of building spaces from raw JSON
| ctx.panel.set_state("event", "on_change_spaces") | ||
| ctx.panel.set_data("event_data", event) | ||
|
|
||
| def on_change_workspace(self, ctx): |
There was a problem hiding this comment.
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.
fiftyone/operators/builtin.py
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.spacesThere was a problem hiding this comment.
Ahh I see. Yah, that makes sense 😄
There was a problem hiding this comment.
Perhaps, for programmatic execution, I will leave the support. However, I'll remove from resolve_input
imanjra
force-pushed
the
feat/ctx-spaces
branch
from
041a2b6
to
b01dd09
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
app/packages/operators/src/state.ts (2)
98-99: LGTM! Consider adding null check forspaces.The addition of
spacesandworkspaceNameto the global context enhances the session-specific information available to operators. This change aligns well with the PR objectives of incorporating spaces and workspace functionalities.Consider adding a null check when accessing
spaces._nameto improve robustness:- const workspaceName = spaces?._name; + const workspaceName = spaces?._name ?? null;Also applies to: 112-113
155-156: LGTM! Consider memoizingspacesandworkspaceName.The integration of
spacesandworkspaceNameinto theExecutionContextis well-implemented, ensuring that operators have access to this information during execution. This change aligns with the PR objectives and enhances the context available to operators.Consider memoizing
spacesandworkspaceNameto optimize performance, especially if these values are expected to change infrequently:+ const memoizedSpaces = useMemo(() => spaces, [spaces]); + const memoizedWorkspaceName = useMemo(() => workspaceName, [workspaceName]); const ctx = useMemo(() => { return new ExecutionContext( params, { // ... other properties - spaces, - workspaceName, + spaces: memoizedSpaces, + workspaceName: memoizedWorkspaceName, }, hooks ); }, [ // ... other dependencies - spaces, - workspaceName, + memoizedSpaces, + memoizedWorkspaceName, ]);This optimization can help reduce unnecessary re-renders if these values are used in child components.
Also applies to: 175-176, 193-194
app/packages/operators/src/operators.ts (1)
146-151: LGTM with a minor suggestion: New getters for spaces and workspaceNameThe new getter methods for
spacesandworkspaceNameare correctly implemented. However, consider adding null checks to prevent potential runtime errors:public get spaces(): SpaceNode { return this._currentContext.spaces ? spaceNodeFromJSON(this._currentContext.spaces) : null; } public get workspaceName(): string { return this._currentContext.workspaceName || ''; }This change ensures that the getters always return a valid value, even if the properties are undefined in the context.
fiftyone/operators/executor.py (1)
613-624: LGTM! Consider adding type hints and docstring examples.The new
spacesproperty implementation looks correct. It retrieves the current spaces layout in the FiftyOne App, either from a workspace or a spaces dictionary. However, there are a few suggestions for improvement:
- Consider adding type hints to improve code readability and maintainability.
- The docstring could benefit from examples of usage and return types.
Here's a suggested improvement with type hints and an expanded docstring:
from typing import Optional import fiftyone as fo @property def spaces(self) -> Optional[fo.Space]: """ The current spaces layout in the FiftyOne App. Returns: fo.Space or None: The current spaces layout, or None if not available. Examples: >>> ctx.spaces <fo.Space: {'name': 'My Workspace', 'children': [...]}> >>> ctx.spaces is None True # when no workspace or spaces are set """ workspace_name = self.request_params.get("workspace_name") if workspace_name is not None: return self.dataset.load_workspace(workspace_name) spaces_dict = self.request_params.get("spaces") if spaces_dict is not None: return fo.Space.from_dict(spaces_dict) return Nonefiftyone/operators/builtin.py (2)
Line range hint
1961-1977: LGTM with a minor optimization suggestion.The changes to the
SaveWorkspace.executemethod look good. They add flexibility by allowing the use of either the current spaces or parsed spaces. However, there's a small optimization opportunity in theif-elseblock.Consider using a ternary operator to simplify the
if-elseblock:- curr_spaces = spaces is None - if curr_spaces: - spaces = ctx.spaces - else: - spaces = _parse_spaces(ctx, spaces) + curr_spaces = spaces is None + spaces = ctx.spaces if curr_spaces else _parse_spaces(ctx, spaces)This change makes the code more concise without sacrificing readability.
🧰 Tools
🪛 Ruff
1962-1965: Use ternary operator
spaces = ctx.spaces if curr_spaces else _parse_spaces(ctx, spaces)instead ofif-else-blockReplace
if-else-block withspaces = ctx.spaces if curr_spaces else _parse_spaces(ctx, spaces)(SIM108)
2290-2294: LGTM with a suggestion for improved error handling.The new
_parse_spacesfunction looks good and serves its purpose well. However, we could improve its robustness by adding some error handling.Consider adding error handling for JSON parsing and Space creation:
def _parse_spaces(ctx, spaces): if isinstance(spaces, str): - spaces = json.loads(spaces) + try: + spaces = json.loads(spaces) + except json.JSONDecodeError as e: + raise ValueError(f"Invalid JSON string for spaces: {e}") - return fo.Space.from_dict(spaces) + try: + return fo.Space.from_dict(spaces) + except Exception as e: + raise ValueError(f"Failed to create Space object: {e}")This change will provide more informative error messages if the input is invalid or if there's an issue creating the Space object.
docs/source/plugins/developing_plugins.rst (2)
Line range hint
977-1007: Approved addition with a suggestion for improvementThe new content about
ctx.spacesis a valuable addition to the documentation, providing developers with information about accessing the current Spaces layout in the App. This is consistent with the existing documentation style and adds important context for plugin development.To improve clarity, consider adding a brief example of how
ctx.spacesmight be used in a typical scenario. This could help developers better understand its practical application.
1995-2007: Approved addition with a suggestion for enhancementThe new
on_change_spacesmethod is a welcome addition to the Panel interface, providing a way for panels to react to changes in the Spaces layout. The documentation style is consistent with other method descriptions in this section.To enhance the documentation further, consider adding a brief example of how this method might be used in practice. This could help developers understand common use cases for responding to space layout changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- 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 (2 hunks)
- fiftyone/operators/builtin.py (8 hunks)
- fiftyone/operators/executor.py (1 hunks)
- fiftyone/operators/operations.py (3 hunks)
- fiftyone/operators/panel.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/packages/operators/src/CustomPanel.tsx
- app/packages/operators/src/hooks.ts
- app/packages/operators/src/useCustomPanelHooks.ts
- fiftyone/operators/operations.py
- fiftyone/operators/panel.py
🧰 Additional context used
📓 Path-based instructions (2)
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.
🪛 Ruff
fiftyone/operators/builtin.py
1962-1965: Use ternary operator
spaces = ctx.spaces if curr_spaces else _parse_spaces(ctx, spaces)instead ofif-else-blockReplace
if-else-block withspaces = ctx.spaces if curr_spaces else _parse_spaces(ctx, spaces)(SIM108)
🔇 Additional comments (10)
app/packages/operators/src/state.ts (1)
98-99: Verify the impact of new properties on dependent components.The addition of
spacesandworkspaceNameto the global and execution contexts is well-implemented. However, it's important to ensure that components and operators that depend on these contexts are updated to handle these new properties correctly.Run the following script to check for potential usage of these new properties:
Please review the output to ensure that all necessary updates have been made in components and operators that rely on the execution context.
Also applies to: 112-113, 155-156, 175-176, 193-194
✅ Verification successful
Update script to correctly include
.tsxfiles for comprehensive verification.The previous script failed to recognize
.tsxfiles, potentially missing usages ofuseGlobalExecutionContextoruseExecutionContext. Please run the updated script below to ensure all relevant files are properly checked:
Impact of new properties on dependent components is verified and no issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential usage of new properties in components and operators. # Test: Search for components that might need updates. echo "Checking for potential component updates:" rg -i -t tsx -t ts 'useGlobalExecutionContext|useExecutionContext' # Test: Search for operators that might need updates. echo "Checking for potential operator updates:" rg -i -t py 'class \w+Operator'Length of output: 1740
Script:
#!/bin/bash # Description: Check for potential usage of new properties in components and operators. # Test: Search for components that might need updates. echo "Checking for potential component updates:" rg -i --type-add 'tsx:*.tsx' -t tsx -t ts 'useGlobalExecutionContext|useExecutionContext' # Test: Search for operators that might need updates. echo "Checking for potential operator updates:" rg -i -t py 'class \w+Operator'Length of output: 2375
app/packages/operators/src/operators.ts (7)
2-3: LGTM: New imports added for spaces functionalityThe new imports from "@fiftyone/spaces" are correctly added to support the new spaces functionality. The reordering of imports from "@fiftyone/utilities" is a minor change that doesn't affect functionality.
96-97: LGTM: RawContext type updated with spaces and workspace propertiesThe
RawContexttype has been correctly updated with two new properties:spacesof typeSpaceNodeJSONandworkspaceNameof typestring. These additions are consistent with the new functionality for spaces and workspaces.
560-561: LGTM: Payload updated with spaces and workspace_nameThe
executeOperatorAsGeneratorfunction has been correctly updated to includespacesandworkspace_namein the payload. This is consistent with the changes in RawContext and ExecutionContext, and the property names are correctly formatted for API communication.
726-727: LGTM: Consistent update of payload with spaces and workspace_nameThe
executeOperatorWithContextfunction has been correctly updated to includespacesandworkspace_namein the server request payload. This change is consistent with the previous updates in RawContext, ExecutionContext, and theexecuteOperatorAsGeneratorfunction.
831-832: LGTM: Consistent update of payload with spaces and workspace_nameThe
resolveRemoteTypefunction has been correctly updated to includespacesandworkspace_namein the request payload. This change maintains consistency with the previous updates in the file, including those in RawContext, ExecutionContext,executeOperatorAsGenerator, andexecuteOperatorWithContextfunctions.
940-941: LGTM: Consistent update of payload with spaces and workspace_nameThe
fetchRemotePlacementsfunction has been correctly updated to includespacesandworkspace_namein the request payload. This change maintains consistency with all previous updates in the file, ensuring that the new space and workspace information is properly communicated across all relevant functions.
Line range hint
1-1161: Summary: Successful implementation of spaces and workspace supportThe changes in this file consistently add support for spaces and workspaces across various functions and types related to operator execution. The modifications include:
- New imports for space-related types and functions.
- Updates to the
RawContexttype andExecutionContextclass.- Consistent additions of
spacesandworkspace_nameto payloads in multiple functions.These changes enhance the context available during operator execution, allowing for more fine-grained control and improved functionality related to spaces and workspaces. The implementation is thorough and consistent throughout the file.
One minor suggestion was made to improve error handling in the new getter methods of the
ExecutionContextclass.Overall, this update successfully integrates spaces and workspace support into the operator execution framework.
fiftyone/operators/builtin.py (1)
2003-2010: LGTM! Workspace renaming handled correctly.The changes to the
EditWorkspaceInfo.executemethod look good. They properly handle the case when a workspace is renamed by updating the current spaces if necessary. This ensures consistency between the workspace name and the current spaces.docs/source/plugins/developing_plugins.rst (1)
Line range hint
1-2007: Overall assessment of changesThe additions to this documentation file are well-integrated and provide valuable information for plugin developers. The new content about
ctx.spacesand theon_change_spacesmethod enhances the documentation by covering an important aspect of the FiftyOne App's interface.These changes maintain the existing documentation style and structure while expanding the coverage of FiftyOne's capabilities. The additions will help developers better understand how to interact with and respond to the App's Spaces layout in their plugins.
What changes are proposed in this pull request?
spacesto 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
Summary 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.Panelclass to handle additional context change events during startup.Documentation
These updates improve user interaction and flexibility within the application.