better group slice support for operators #4850
Conversation
Merge `release/v1.0.0` to `develop`
WalkthroughThe changes in this pull request introduce enhancements related to group slicing functionality across multiple components. Key modifications include the addition of new parameters and properties to various functions and classes, enabling better handling of group slice changes. These updates affect how context is managed and improve the responsiveness of the application to group slice events. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
![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 (17)
app/packages/operators/src/CustomPanel.tsx (1)
117-117: LGTM! Consider adding type annotations for improved type safety.The addition of the
on_change_group_sliceparameter and its corresponding proponChangeGroupSliceis well-implemented and aligns with the PR objectives. The changes follow the existing naming conventions and code structure.For improved type safety, consider adding type annotations to the
defineCustomPanelfunction parameters. This would help catch potential type-related issues early and improve code maintainability. For example:export function defineCustomPanel({ // ... other parameters on_change_group_slice, panel_name, panel_label, }: { // ... other parameter types on_change_group_slice?: (groupSlice: GroupSlice) => void, panel_name: string, panel_label: string, }) { // ... function body }Replace
GroupSlicewith the actual type of the group slice data structure you're using.Also applies to: 134-134
app/packages/operators/src/hooks.ts (2)
28-28: Consider adding a type annotation forgroupSlice.To improve code clarity and prevent potential type inference issues, consider adding an explicit type annotation for the
groupSlicevariable. This will make the expected type clear to other developers and help catch any type-related errors early.- const groupSlice = useRecoilValue(fos.groupSlice); + const groupSlice: YourGroupSliceType = useRecoilValue(fos.groupSlice);Replace
YourGroupSliceTypewith the actual type of thegroupSlicestate.
28-28: Document the purpose and impact ofgroupSlicein the context.The addition of
groupSliceto the operator context is a significant change. To ensure clarity for other developers and maintain code readability, please add a comment explaining:
- The purpose of
groupSlicein the context.- How it affects the behavior of operators or other components consuming this context.
- Any considerations for when
groupSlicechanges.Add a comment above the
groupSlicedeclaration, for example:// groupSlice represents the current group slice state. // It's used by operators to adjust their behavior based on the active group slice. // Changes to groupSlice may trigger re-evaluation of operator placements. const groupSlice: YourGroupSliceType = useRecoilValue(fos.groupSlice);Also applies to: 51-51, 63-63
app/packages/operators/src/useCustomPanelHooks.ts (2)
28-28: LGTM! Consider adding JSDoc comment for clarity.The addition of the
onChangeGroupSliceproperty to theCustomPanelPropsinterface is consistent with the existing pattern and naming convention. It enhances the flexibility of the custom panel by allowing it to respond to group slice changes.Consider adding a JSDoc comment to describe the purpose and usage of this new property, for example:
/** Callback fired when the group slice changes */ onChangeGroupSlice?: string;
133-138: LGTM! Consider refactoring for improved readability.The addition of the
useCtxChangePanelEventhook forctx.groupSliceandprops.onChangeGroupSliceis consistent with the existing pattern and enhances the functionality of the custom panel to react to group slice changes.Consider refactoring the repeated
useCtxChangePanelEventcalls into a more concise format to improve readability. For example:const ctxChangeEvents = [ { value: ctx._currentContext, handler: props.onChangeCtx }, { value: ctx.view, handler: props.onChangeView }, // ... other existing events ... { value: ctx.groupSlice, handler: props.onChangeGroupSlice } ]; ctxChangeEvents.forEach(({ value, handler }) => useCtxChangePanelEvent(isLoaded, panelId, value, handler) );This refactoring would make it easier to add or modify context change events in the future.
fiftyone/operators/operations.py (2)
319-319: LGTM! Consider a minor docstring improvement.The addition of the
on_change_group_sliceparameter is well-implemented and consistent with other similar parameters. It's correctly added to the method signature, docstring, and params dictionary.Consider adding a brief explanation of what a "group slice" is in the docstring, as it might not be immediately clear to all users. For example:
- on_change_group_slice (None): an operator to invoke when the group slice changes + on_change_group_slice (None): an operator to invoke when the group slice (subset of samples within a group) changesAlso applies to: 357-357, 380-380
Line range hint
638-644: LGTM! Consider adding a docstring.The addition of the
set_group_slicemethod is a good complement to the newon_change_group_sliceparameter inregister_panel. The implementation looks correct.Consider adding a docstring to the
set_group_slicemethod to provide more context and usage information. For example:def set_group_slice(self, slice): """Set the active group slice in the App. This method triggers an event to update the group slice, which represents a subset of samples within a group. Args: slice: the group slice to activate """ return self._ctx.trigger("set_group_slice", {"slice": slice})app/packages/operators/src/state.ts (4)
Line range hint
96-107: LGTM! Consider sorting the properties alphabetically.The addition of
groupSliceto theglobalContextSelectoris implemented correctly and aligns with the existing pattern. This change supports the PR objective of adding group slice support.For better readability and consistency, consider sorting the properties in the returned object alphabetically. This would make it easier to locate specific properties in the future.
return { datasetName, + extendedSelection, + groupSlice, view, extended, filters, selectedSamples, selectedLabels, viewName, - extendedSelection, - groupSlice, };
Line range hint
1-1000: Consider adding JSDoc comments to complex functions.To improve code readability and maintainability, consider adding JSDoc comments to complex functions throughout the file. This will help other developers understand the purpose, parameters, and return values of these functions more easily.
For example, you could add a JSDoc comment to the
useOperatorExecutorfunction:/** * Hook for executing an operator with the given URI. * @param {string} uri - The URI of the operator to execute. * @param {Object} handlers - Optional handlers for success and error cases. * @returns {Object} An object containing execution state and methods. */ export function useOperatorExecutor(uri, handlers: any = {}) { // ... existing implementation }
Line range hint
1-1000: Consider breaking down long functions for better maintainability.Some functions in this file are quite long and complex. Consider breaking them down into smaller, more focused functions to improve readability and maintainability.
For example, the
useOperatorPromptfunction (starting around line 330) could be split into smaller functions, each handling a specific aspect of the operator prompt logic.
Line range hint
1000-1100: Improve error handling in theexecutefunction.In the
executefunction ofuseOperatorExecutor, consider improving the error handling to provide more specific error messages and potentially handle different types of errors differently.You could enhance the error handling like this:
try { // ... existing code ... } catch (e) { callback?.(new OperatorResult(operator, null, ctx.executor, e, false)); const isAbortError = e.name === "AbortError" || e instanceof DOMException; if (!isAbortError) { setError(e); setResult(null); handlers.onError?.(e); console.error("Error executing operator", operator, ctx); console.error(e); let errorMessage = "Failed to execute an operation"; if (e instanceof TypeError) { errorMessage = "Type error occurred during operation execution"; } else if (e instanceof RangeError) { errorMessage = "Range error occurred during operation execution"; } // Add more specific error types as needed notify({ msg: errorMessage, variant: "error" }); } }This approach provides more specific error messages based on the type of error that occurred, which can be helpful for debugging and user feedback.
app/packages/operators/src/operators.ts (3)
93-93: LGTM. Consider adding documentation for the newgroupSliceproperty.The addition of the
groupSliceproperty to theRawContexttype is a good enhancement for supporting group slice functionality. To improve maintainability and clarity for other developers, consider adding a brief comment explaining the purpose and expected format of this property.
136-138: LGTM. Consider specifying the return type for thegroupSlicegetter.The addition of the
groupSlicegetter is consistent with the class structure and provides access to the new property. To improve type safety, consider specifying the return type asstringinstead ofany:- public get groupSlice(): any { + public get groupSlice(): string { return this._currentContext.groupSlice; }This change would align the getter's type with the
RawContexttype definition.
811-811: LGTM. Consider refactoring to reduce duplication in server request objects.The addition of the
group_sliceproperty to the server requests inresolveRemoteType,resolveExecutionOptions, andfetchRemotePlacementsfunctions is consistent and completes the integration of the group slice functionality across all relevant API calls.To improve maintainability and reduce duplication, consider creating a helper function that constructs the common parts of these server request objects, including the
group_sliceproperty. This would centralize the logic and make future changes easier to implement across all API calls.Example:
function createBaseServerRequest(ctx: ExecutionContext) { const currentContext = ctx._currentContext; return { current_sample: currentContext.currentSample, dataset_name: currentContext.datasetName, // ... other common properties ... group_slice: currentContext.groupSlice, }; } // Usage in functions: const serverRequest = { ...createBaseServerRequest(ctx), // Add function-specific properties here };This refactoring would make the code more DRY and easier to maintain.
Also applies to: 885-885, 916-916
fiftyone/operators/builtin.py (2)
1522-1523: LGTM! Consider adding a comment for clarity.The added conditional statement correctly updates the current context's group slice when the renamed slice is active. This change aligns well with the PR objectives of enhancing group slice support.
Consider adding a brief comment explaining the purpose of this condition for better code readability:
if ctx.group_slice == name: + # Update current context if the renamed slice was active ctx.ops.set_group_slice(ctx.dataset.default_group_slice)
1570-1571: LGTM! Consider adding a comment for consistency.The added conditional statement correctly updates the current context's group slice when the deleted slice is active. This change is consistent with the implementation in the
RenameGroupSliceclass and aligns well with the PR objectives.For consistency with the suggested change in the
RenameGroupSliceclass, consider adding a brief comment explaining the purpose of this condition:if ctx.group_slice == name: + # Update current context if the deleted slice was active ctx.ops.set_group_slice(ctx.dataset.default_group_slice)docs/source/plugins/developing_plugins.rst (1)
2050-2062: Ensure consistency with otheron_changemethodsThe new
on_change_group_slicemethod follows a similar structure to otheron_changemethods in this section, which is good for consistency. However, to fully align with the existing pattern:
- Consider adding a code example within the method docstring, similar to other methods like
on_change_datasetandon_change_view.- The indentation of the method body (lines 2057-2062) appears to be one level too deep compared to other methods. Adjust the indentation to match the surrounding code.
These small adjustments will ensure the new method seamlessly integrates with the existing documentation style.
📜 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 (7 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/builtin.py (2 hunks)
- fiftyone/operators/executor.py (1 hunks)
- fiftyone/operators/operations.py (3 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)
28-28: LGTM! Note on potential performance impact.The addition of
groupSliceto the context and dependency array looks good and aligns with the PR objectives. However, be aware that this change might increase the frequency of context updates ifgroupSlicechanges often. Monitor the performance impact, especially ifgroupSliceupdates frequently or if the context is used by many components.To help assess the potential impact, you can run the following script to check how often
groupSliceis used or updated in the codebase:Also applies to: 51-51, 63-63
✅ Verification successful
Performance Impact Monitoring Recommended
The verification confirms that
groupSliceis referenced in multiple files across the codebase. While the number of potential update locations is limited, it's advisable to monitor the application's performance to ensure that addinggroupSliceto the context and dependency array does not lead to excessive context updates or re-renders.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage and updates of groupSlice in the codebase echo "Files referencing groupSlice:" rg --type typescript --type tsx "groupSlice" -l echo "\nPotential update locations for groupSlice:" rg --type typescript --type tsx "set.*groupSlice|groupSlice.*set" -nLength of output: 356
Script:
#!/bin/bash # Description: Check usage and updates of groupSlice in the codebase using file extensions echo "Files referencing groupSlice:" rg "groupSlice" --glob "*.ts" --glob "*.tsx" -l echo "\nPotential update locations for groupSlice:" rg "set.*groupSlice|groupSlice.*set" --glob "*.ts" --glob "*.tsx" -nLength of output: 15657
app/packages/operators/src/useCustomPanelHooks.ts (1)
28-28: Overall impact: Enhances group slice support as intended.The changes successfully implement group slice support in custom panels, aligning with the PR objectives. The modifications are consistent with existing patterns and are well-integrated into the codebase.
To ensure proper adoption of this new feature:
- Update any relevant documentation to include information about the new
onChangeGroupSliceproperty and its usage.- Consider adding or updating examples that demonstrate how to use the group slice functionality in custom panels.
- Ensure that any related components or hooks that might interact with custom panels are aware of this new capability.
To verify the impact and usage of the new group slice functionality, you can run the following script:
This will help identify any existing usage and potential places where the new functionality could be applied.
Also applies to: 133-138
✅ Verification successful
Confirmation:
onChangeGroupSliceusage detected and correctly implemented.The
onChangeGroupSliceproperty is properly defined inuseCustomPanelHooks.tsand utilized withinCustomPanel.tsx, ensuring that group slice support is effectively integrated as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of onChangeGroupSlice in the codebase echo "Searching for onChangeGroupSlice usage:" rg --type typescript --type typescript "onChangeGroupSlice" -C 3 # Search for potential places where group slice functionality might be relevant echo "\nSearching for potential places to use group slice functionality:" rg --type typescript --type typescript "groupSlice" -C 3Length of output: 423
Script:
#!/bin/bash # Search for usage of onChangeGroupSlice in TypeScript files echo "Searching for onChangeGroupSlice usage in .ts and .tsx files:" rg "onChangeGroupSlice" -g "*.ts" -g "*.tsx" -C 3 # Search for potential places where group slice functionality might be relevant in TypeScript files echo -e "\nSearching for potential places to use group slice functionality in .ts and .tsx files:" rg "groupSlice" -g "*.ts" -g "*.tsx" -C 3Length of output: 63624
fiftyone/operators/operations.py (1)
Line range hint
1-644: Summary: Group slice support well-implementedThe changes to add group slice support in the
Operationsclass have been implemented correctly and consistently. The newon_change_group_sliceparameter in theregister_panelmethod and theset_group_slicemethod work together to provide this functionality.These changes align well with the PR objectives of enhancing group slice functionality within the FiftyOne application. The implementation is consistent with existing patterns in the code, making it easy to understand and maintain.
To further improve the code:
- Consider adding brief explanations of "group slice" in the docstrings to enhance clarity for users.
- Add a docstring to the new
set_group_slicemethod to provide usage information.These minor documentation improvements will help users better understand and utilize the new group slice features.
app/packages/operators/src/state.ts (1)
Line range hint
147-179: LGTM! Changes are consistent and follow best practices.The addition of
groupSliceto theuseExecutionContexthook is implemented correctly. The changes are consistent with the addition to the global context, and follow React best practices by includinggroupSlicein the dependency array of theuseMemohook.app/packages/operators/src/operators.ts (2)
545-545: LGTM. Thegroup_sliceproperty is correctly included in the server request.The addition of the
group_sliceproperty to the server request in theexecuteOperatorAsGeneratorfunction is consistent with the earlier changes and ensures that the group slice information is properly communicated to the server.
708-708: LGTM. Thegroup_sliceproperty is correctly included in the server request.The addition of the
group_sliceproperty to the server request in theexecuteOperatorWithContextfunction is consistent with the earlier changes and ensures that the group slice information is properly communicated to the server for context-based operator execution.fiftyone/operators/executor.py (1)
700-703: LGTM with minor suggestions.The new
group_sliceproperty is a good addition to theExecutionContextclass. It provides a convenient way to access the group slice information. Here are a few suggestions for improvement:
- Consider expanding the docstring to provide more context about what a "group slice" represents and its potential values.
- You might want to add type hinting for the return value to improve code readability and IDE support.
Example with improvements:
@property def group_slice(self) -> Optional[Any]: """ The group slice of the view. Returns: The group slice value if present in the request parameters, otherwise None. The type of the group slice depends on how it's set in the request. """ return self.request_params.get("group_slice", None)fiftyone/operators/builtin.py (1)
1522-1523: Overall, the changes improve group slice handling.The modifications in both the
RenameGroupSliceandDeleteGroupSliceclasses enhance the handling of group slices by ensuring that the current context is updated when an active slice is renamed or deleted. These changes align well with the PR objectives of improving group slice support and contribute to a more robust implementation.Also applies to: 1570-1571
docs/source/plugins/developing_plugins.rst (1)
2050-2062: New methodon_change_group_sliceadded to Panel interfaceThe new
on_change_group_slicemethod has been added to handle changes in the current group slice. This addition enhances the Panel interface by providing a way to respond to group slice changes.A few suggestions to improve the documentation:
- Consider adding a brief explanation of what a "group slice" is, as it might not be immediately clear to all users.
- It would be helpful to provide an example of how this method might be used in practice.
- Ensure that this new method is mentioned in the overview or summary of Panel methods, if such a section exists in this document.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
fiftyone/operators/executor.py (1)
511-536: LGTM! Consider adding type hints and docstring.The addition of the
group_sliceproperty and the related changes in thedatasetproperty are well-implemented. They enhance the functionality by properly handling group slices for grouped media.Consider adding type hints and a docstring to the
group_sliceproperty for better code clarity:@property - def group_slice(self): + def group_slice(self) -> Optional[str]: + """ + The current group slice of the view (if any). + + Returns: + Optional[str]: The group slice value or None if not set. + """ return self.request_params.get("group_slice", None)Also applies to: 711-715
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- docs/source/plugins/developing_plugins.rst (2 hunks)
- fiftyone/operators/builtin.py (2 hunks)
- fiftyone/operators/executor.py (3 hunks)
- fiftyone/operators/operations.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/source/plugins/developing_plugins.rst
- fiftyone/operators/builtin.py
- fiftyone/operators/operations.py
Yah, totally! I am happy to rebase this if there are no objections from the release POV. This should be a clean rebase. I think it might be as simple as updating the base branch of the PR. 🙂 |
What changes are proposed in this pull request?
How is this patch tested? If it is not, please explain why.
Using an example operator and panel
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
on_change_group_sliceparameter to enhance group slice handling in the Custom Panel.groupSliceto various contexts, improving the management of group slice data.on_change_group_sliceto respond to group slice changes during startup.Documentation
on_change_group_sliceparameter in relevant functions.Bug Fixes