Improve performance for Selection tree component #5185
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 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: 1
🧹 Outside diff range and nitpick comments (21)
app/packages/state/src/recoil/options.ts (2)
58-68: Consider extracting the view mode type to a type aliasThe atomFamily implementation looks good, with proper typing and persistence. However, the view mode type union is used in multiple places and could benefit from being defined as a reusable type alias.
+type ViewMode = "carousel" | "pagination" | "video" | null; -export const dynamicGroupsViewModeStore = atomFamily< - "carousel" | "pagination" | "video" | null, - boolean ->({ +export const dynamicGroupsViewModeStore = atomFamily<ViewMode, boolean>({
69-93: Add explicit type definitions for better type safetyWhile the implementation is correct, consider adding explicit type definitions for better maintainability and type safety.
-export const dynamicGroupsViewMode = selectorFamily({ +export const dynamicGroupsViewMode = selectorFamily<ViewMode, boolean>({ key: "dynamicGroupsViewMode", get: - (modal: boolean) => + (isModal: boolean) => ({ get }) => { - const value = get(dynamicGroupsViewModeStore(modal)); + const value = get(dynamicGroupsViewModeStore(isModal)); if (!value) { - return modal - ? get(dynamicGroupsViewModeStore(false)) ?? "pagination" + return isModal + ? get(dynamicGroupsViewModeStore(false)) ?? "pagination" : "pagination"; } return value; }, set: - (modal: boolean) => + (isModal: boolean) => ({ reset, set }, newValue) => { - const instance = dynamicGroupsViewModeStore(modal); + const instance = dynamicGroupsViewModeStore(isModal);app/packages/operators/src/SplitButton.tsx (3)
132-134: Consider using disabled state for cursor stylingWhile the current implementation works, consider using the disabled state for a more semantic approach to cursor styling.
sx={{ - cursor: option.onClick ? "pointer" : "default", + cursor: option.disabled ? "default" : "pointer", }}
151-160: Consider using theme-based typographyInstead of hardcoding font sizes, consider using theme typography variants for better maintainability and consistency.
secondary={ <Box sx={{ - fontSize: "11px", - "& *": { fontSize: "inherit" }, + typography: 'caption', + "& *": { typography: 'inherit' }, }} > {option.description} </Box> }
Line range hint
175-193: Add TypeScript types and use theme spacingThe function needs TypeScript types for better type safety, and should use theme spacing instead of magic numbers.
+interface PrimaryWithTagProps { + label: string; + tag?: string; + disabled?: boolean; +} -function PrimaryWithTag({ label, tag, disabled }) { +function PrimaryWithTag({ label, tag, disabled }: PrimaryWithTagProps) { const theme = useTheme(); const tagEl = tag ? ( <span style={{ - fontSize: "11px", + typography: 'caption', color: disabled ? theme.text.secondary : theme.custom.primarySoft, - marginLeft: "5px", + marginLeft: theme.spacing(0.5), }} > {tag} </span> ) : null;app/packages/embeddings/src/Embeddings.tsx (1)
95-106: Consider adding type safety for the callback functionThe implementation looks good and aligns with the performance improvements. However, there are a few suggestions to enhance type safety and maintainability:
Consider applying these improvements:
<PlotOption - to={() => { + to={(): void => { if (constants.IS_APP_MODE_FIFTYONE) { setShowCTA(true); } else { computeViz.prompt(); } }} title={"Compute visualization"} > <Add /> </PlotOption>This explicitly defines the return type of the callback function, making it more type-safe and self-documenting.
app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx (2)
42-42: Consider adding defensive checks for onChange callbackWhile handling undefined data is good, we should ensure the onChange callback exists before calling it.
- onChange(path, []); + if (typeof onChange === 'function') { + onChange(path, []); + } + return null;
Line range hint
191-270: Consider optimizing state updates for better performanceThe useEffect hook contains multiple iterations over the data structure that could be optimized:
- Consider memoizing the sample ID filtering logic
- Use Set operations more effectively for difference checking
- Consider splitting the state updates into smaller, more focused effects
Here's a suggested optimization for the difference checking:
- const dataSet: Set<string> = new Set(data); - const unboundSet: Set<string> = new Set(selectedIdsFromUnboundState); - const hasDifference = - dataSet.size !== unboundSet.size || - [...dataSet].some((id) => !unboundSet.has(id)); + const dataSet = new Set(data); + const unboundSet = new Set(selectedIdsFromUnboundState); + const hasDifference = + dataSet.size !== unboundSet.size || + !Array.from(dataSet).every(id => unboundSet.has(id));Consider extracting the state update logic into a memoized callback:
const updateCheckedState = React.useCallback((dataSet: Set<string>) => { setCheckedState(prevState => { // ... existing update logic }); }, [structure]);fiftyone/server/lightning.py (2)
48-48: Add field documentation for better clarityConsider adding a description field to document the purpose and usage of the
sliceparameter, similar to other fields in the class.- slice: t.Optional[str] = None + slice: t.Optional[str] = gql.field( + description="Filter results by dataset group slice name", + default=None + )
331-333: Consider adding type hints for filter parameter valuesWhile the filter parameter typing is correct, consider adding more specific type hints for the expected values to improve code maintainability.
- filter: t.Optional[t.Mapping[str, str]], + filter: t.Optional[t.Mapping[str, t.Union[str, t.Dict[str, str]]]],Also applies to: 340-340, 366-366, 369-371
fiftyone/factory/repos/delegated_operation.py (1)
187-188: Use direct attribute assignment instead of setattrWhile the initialization of metadata to an empty dictionary is a good change, we can simplify the code by using direct attribute assignment.
- else: - setattr(op, "metadata", {}) + else: + op.metadata = {}🧰 Tools
🪛 Ruff (0.7.0)
188-188: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
fiftyone/operators/delegated.py (1)
517-520: LGTM! Consider enhancing error handling.The changes improve performance by directly setting parameters in the context instead of creating a new dictionary. This aligns well with the PR's performance improvement goals.
Consider adding more specific error handling for the output type resolution:
- except (AttributeError, Exception): + except AttributeError as e: + logger.warning( + f"Failed to resolve output schema: attribute error - {str(e)}\n" + + traceback.format_exc(), + ) + except Exception as e: + logger.warning( + f"Failed to resolve output schema: unexpected error - {str(e)}\n" + + traceback.format_exc(), + )This would provide more detailed error information and make debugging easier.
fiftyone/operators/operations.py (1)
668-677: Improve parameter passing consistency inopen_samplemethod.The method implementation is correct, but the parameter passing style differs from other methods in the class. For consistency with the codebase, wrap the parameters in
params.- return self._ctx.trigger( - "open_sample", {"id": id, "group_id": group_id} - ) + return self._ctx.trigger( + "open_sample", + params={"id": id, "group_id": group_id} + )docs/source/plugins/using_plugins.rst (1)
894-901: Documentation addition looks good, but could be more detailed.The addition of the
FIFTYONE_ALLOW_LEGACY_ORCHESTRATORSconfiguration requirement is well-documented. However, consider enhancing it with:
- The default value of this flag
- The consequences of not setting this variable
- Whether this is a permanent requirement or temporary migration feature
Consider expanding the documentation with this additional context:
You must also ensure that the :ref:`allow_legacy_orchestrators <configuring-fiftyone>` config flag is set in the environment where you run the App, e.g. by setting: .. code-block:: shell export FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS=true + +.. note:: + + By default, this flag is set to `false`. If not set to `true`, delegated + operations will fail to execute in FiftyOne Open Source environments.app/packages/operators/src/state.ts (2)
331-349: Enhanced UX for unavailable orchestratorsGood implementation of user guidance when orchestrators are unavailable:
- Clear visual feedback with "NOT AVAILABLE" tag
- Helpful documentation link for users
- Proper use of React.createElement for dynamic Markdown rendering
However, consider extracting the markdown URL to a constant to maintain consistency and ease updates.
+const DELEGATED_OPERATIONS_DOC_URL = "https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations"; + const markdownDesc = React.createElement( Markdown, null, - "[Learn how](https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations) to run this operation in the background" + `[Learn how](${DELEGATED_OPERATIONS_DOC_URL}) to run this operation in the background` );
395-399: Consistent warning message implementationThe warning message implementation follows the same pattern as the documentation link. Consider using the same constant for the URL here as well.
+const DELEGATED_OPERATIONS_DOC_URL = "https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations"; + const warningStr = - "This operation requires [delegated execution](https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations)"; + `This operation requires [delegated execution](${DELEGATED_OPERATIONS_DOC_URL})`;docs/source/user_guide/config.rst (1)
113-115: Consider enhancing the security implications in the description.The new configuration field is well-documented and properly formatted. However, consider expanding the description to:
- Highlight any security implications of allowing local scheduling
- Provide more specific guidance on when users should enable this setting
- Include a direct link to the documentation section about delegated orchestrators
docs/source/plugins/developing_plugins.rst (3)
995-996: Improve clarity of thectx.delegatedproperty documentation.The documentation should clarify that
ctx.delegatedis a boolean property that indicates whether the current operation is being executed in delegated mode.- `ctx.delegated` - whether the operation was delegated + `ctx.delegated` - a boolean indicating whether the operation is currently being executed in delegated mode (True) or immediate mode (False)
Line range hint
26-89: Consider adding a diagram for the plugin architecture.The design overview section effectively explains the components of plugins. Consider enhancing it with a visual diagram showing how panels, operators, and components interact.
Line range hint
159-1024: Consider adding troubleshooting section.The documentation thoroughly covers development aspects but could benefit from a troubleshooting section addressing common issues developers might encounter.
Consider adding a section like:
Troubleshooting -------------- Common issues and their solutions: 1. Plugin not appearing in the App 2. Operator execution failures 3. Panel state management issues 4. Debugging delegated operationstests/unittests/lightning_tests.py (1)
1096-1139: Consider refactoring repetitive code in test casesThe code between lines 1096-1139 repeats similar logic for querying and asserting results for different slices. To improve maintainability and reduce duplication, consider refactoring this code into a helper function or parameterizing the test cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
app/packages/relay/src/queries/__generated__/lightningQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**docs/source/images/plugins/operators/operator-user-delegation.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (23)
app/packages/components/src/components/PanelCTA/index.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx(3 hunks)app/packages/embeddings/src/Embeddings.tsx(1 hunks)app/packages/operators/src/SplitButton.tsx(4 hunks)app/packages/operators/src/built-in-operators.ts(2 hunks)app/packages/operators/src/state.ts(8 hunks)app/packages/state/src/hooks/useSetModalState.ts(0 hunks)app/packages/state/src/recoil/options.ts(2 hunks)app/packages/state/src/recoil/queryPerformance.ts(2 hunks)app/schema.graphql(1 hunks)docs/source/plugins/developing_plugins.rst(1 hunks)docs/source/plugins/using_plugins.rst(1 hunks)docs/source/user_guide/app.rst(0 hunks)docs/source/user_guide/config.rst(3 hunks)fiftyone/core/config.py(1 hunks)fiftyone/factory/repos/delegated_operation.py(1 hunks)fiftyone/operators/delegated.py(1 hunks)fiftyone/operators/executor.py(4 hunks)fiftyone/operators/operations.py(1 hunks)fiftyone/server/lightning.py(5 hunks)tests/unittests/lightning_tests.py(3 hunks)tests/unittests/operators/delegated_tests.py(1 hunks)
💤 Files with no reviewable changes (2)
- app/packages/state/src/hooks/useSetModalState.ts
- docs/source/user_guide/app.rst
✅ Files skipped from review due to trivial changes (1)
- app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx
🧰 Additional context used
📓 Path-based instructions (8)
app/packages/components/src/components/PanelCTA/index.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/core/src/plugins/SchemaIO/components/TreeSelectionView.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/embeddings/src/Embeddings.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/SplitButton.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/built-in-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/state/src/recoil/options.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/state/src/recoil/queryPerformance.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 (0.7.0)
fiftyone/factory/repos/delegated_operation.py
188-188: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
fiftyone/operators/executor.py
402-402: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (32)
app/packages/state/src/recoil/options.ts (2)
7-7: LGTM: Import changes are correct
The DefaultValue import from Recoil is properly added and grouped with other Recoil imports.
69-93: LGTM: Implementation aligns with performance objectives
The selectorFamily implementation correctly:
- Provides fallback logic for undefined states
- Implements proper reset functionality
- Maintains state consistency between modal and non-modal views
- Aligns with the PR's goal of improving performance by defaulting to not selected state
app/packages/operators/src/SplitButton.tsx (1)
14-14: LGTM: Box import is properly organized
The Box import is correctly placed with other Material-UI imports and is used appropriately for description text wrapping.
app/packages/embeddings/src/Embeddings.tsx (1)
97-102: Verify the app mode constant usage
The conditional logic relies on constants.IS_APP_MODE_FIFTYONE. Let's ensure this constant is properly defined and used consistently across the codebase.
✅ Verification successful
IS_APP_MODE_FIFTYONE constant is properly defined and used consistently
The constant is correctly defined in app/packages/utilities/src/constants.ts and is being used consistently across the codebase for conditional rendering of CTAs and demo-related content. The constant is derived from APP_MODE which is set to "fiftyone", making the implementation correct and reliable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of IS_APP_MODE_FIFTYONE to verify consistency
rg "IS_APP_MODE_FIFTYONE" --type ts --type tsx
# Check the constant definition
fd -e ts -e tsx --exec ast-grep --pattern 'const IS_APP_MODE_FIFTYONE = $_'
Length of output: 256
Script:
#!/bin/bash
# Check the APP_MODE constant definition and usage
ast-grep --pattern 'const APP_MODE = $_'
# Search for other usages of APP_MODE
rg "APP_MODE" -A 2 --type ts
Length of output: 2745
app/packages/state/src/recoil/queryPerformance.ts (2)
14-14: LGTM!
The import statement follows the existing pattern and is correctly placed among other local imports.
38-38: Verify the integration with group slicing functionality.
The addition of slice parameter to the lightning query input looks correct. However, we should ensure that this change is properly integrated with the group slicing functionality.
Let's verify the integration:
✅ Verification successful
Integration with group slicing functionality is properly implemented
The codebase analysis confirms that the slice parameter in the lightning query is correctly integrated with the group slicing functionality:
- The
groupSliceatom is properly imported and used inqueryPerformance.ts - The slice parameter is consistently used across related components and queries
- The integration follows the same pattern as other features using group slices (distributions, aggregations, etc.)
- The GraphQL schema and types properly handle the slice parameter in the
LightningInput
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration between lightning query and group slicing
# Check for any direct usage of groupSlice in components
rg -t tsx -t ts "groupSlice" -A 3
# Check for any GraphQL schema definitions related to slice
rg -t graphql "slice.*LightningInput" -A 3
# Check for any tests covering this integration
fd -e test.ts -e test.tsx | xargs rg "lightningQuery.*slice"
Length of output: 201
Script:
#!/bin/bash
# Let's try a different approach to verify the integration
# Search for groupSlice usage in TypeScript files
rg "groupSlice" --type ts -A 3
# Search for lightning query related tests
rg "lightningQuery" --type ts -A 3
# Look for slice-related configurations in the state package
fd "query.*\.ts$" -t f app/packages/state/src | xargs rg "slice"
Length of output: 51266
app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx (1)
76-76: Improved initial render performance with collapsed state
The change to initialize nodes in a collapsed state aligns with the performance improvement goals. This prevents unnecessary rendering of child nodes on initial load.
Let's verify the performance impact:
Also applies to: 85-85
✅ Verification successful
Initial collapsed state change verified as performance optimization
The code changes are well-justified based on the evidence:
- The component uses a recursive tree structure where each node can render multiple children
- Starting with nodes collapsed (
state[parentId] = true) prevents the immediate rendering of all child nodes - The implementation properly maintains collapsed state through React's useState and allows for dynamic expansion when needed
- The change aligns with common performance patterns for tree structures by reducing initial render complexity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any performance tests or benchmarks related to this component
rg -g '*.{test,spec}.{ts,tsx}' -l 'TreeSelectionView.*performance|benchmark'
# Look for any related performance issues
gh issue list --label performance --search "TreeSelectionView in:title,body"
Length of output: 157
Script:
#!/bin/bash
# Let's check the component's usage and state management patterns
ast-grep --pattern 'const TreeSelectionView = $_' -A 20
# Check for any related components or tests
rg "TreeSelectionView" -l
# Look for any performance-related comments or documentation
rg -g "!*.{lock,svg}" "performance|render" -C 2 -l app/packages/core/src/plugins/SchemaIO/
Length of output: 915
Script:
#!/bin/bash
# Let's examine the performance-related content in TreeSelectionView
rg -g "*.tsx" "performance|render" -C 3 app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx
# Check for any state management patterns and their impact
ast-grep --pattern 'useState($$$)' app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx
# Look for any comments or documentation about the collapsed state
rg "collapsed|expanded" -C 2 app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx
Length of output: 2094
fiftyone/server/lightning.py (2)
302-302: LGTM! Filter propagation is well implemented
The filter parameter is correctly propagated through the query execution chain and properly applied in the aggregation pipeline.
Also applies to: 306-306, 316-316, 320-322, 324-325
142-146: Consider adding validation for slice parameter
The current implementation doesn't validate if the provided slice exists in the dataset. This could lead to silent failures where no data is returned.
Let's verify the potential for invalid slices:
Consider refactoring to validate the slice:
filter = (
- {f"{dataset.group_field}.name": input.slice}
- if dataset.group_field and input.slice
- else None
+ {f"{dataset.group_field}.name": input.slice}
+ if (
+ dataset.group_field
+ and input.slice
+ and input.slice in dataset.distinct(f"{dataset.group_field}.name")
+ ) else None
)app/schema.graphql (1)
447-447: LGTM: The new slice field addition looks good!
The nullable String field aligns with the PR's objective of improving performance by controlling the initial selection state of the tree component.
Let's verify the usage of this new field:
✅ Verification successful
The slice field addition to LightningInput is safe and properly structured
The field is:
- Correctly typed as nullable String
- Added to the appropriate input type
- Consistent with the input type's structure
- Properly aligned with similar slice fields in other input types seen in the schema
The change is minimal and follows GraphQL schema best practices. While we couldn't find direct query usages, this is expected for a new optional field that maintains backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new slice field in Lightning queries
# Expected: Find GraphQL queries using the LightningInput type with the slice field
# Search for GraphQL queries using LightningInput
rg -l 'lightning.*\(.*\$input.*LightningInput\)' --type ts --type tsx
# Search for direct usages of the slice field
rg 'slice:\s*[^,}\s]+' --type ts --type tsx
Length of output: 173
Script:
#!/bin/bash
# Let's try a different approach to find Lightning-related queries and slice usage
# First, let's find all GraphQL files
fd -e graphql -x cat {} | rg -l 'lightning.*\(.*input.*LightningInput\)'
# Then search for any files containing 'slice' field in queries
fd -e graphql -x cat {} | rg 'slice:'
# Also search for TypeScript/JavaScript files mentioning LightningInput
rg -t js -t jsx 'LightningInput'
# And look for slice field usage in JS/TS files
rg -t js -t jsx 'slice:'
Length of output: 432
Script:
#!/bin/bash
# Let's examine the schema more closely to understand the context of slice usage
fd -e graphql -x cat {} | rg -A 5 -B 5 'slice:'
# Also search for JavaScript files with a more compatible ripgrep syntax
rg -t js 'LightningInput'
# And look for slice-related code in JavaScript files
rg -t js 'slice[^a-zA-Z]'
Length of output: 91274
Script:
#!/bin/bash
# Let's search for GraphQL files and examine the context around the slice field
fd -e graphql -x cat {} | rg -B 10 -A 10 'input LightningInput'
# Also search for Lightning-related queries in JavaScript files
rg -t js 'lightning.*\{.*\$input.*LightningInput\)'
Length of output: 480
fiftyone/factory/repos/delegated_operation.py (1)
Line range hint 1-500: LGTM! The simplified update logic improves code maintainability
The removal of the conditional pipeline update logic while maintaining the core functionality makes the code more maintainable. The changes ensure that metadata is always initialized and the update logic is more straightforward.
🧰 Tools
🪛 Ruff (0.7.0)
180-180: Use kwargs.get("delegation_target") instead of kwargs.get("delegation_target", None)
Replace kwargs.get("delegation_target", None) with kwargs.get("delegation_target")
(SIM910)
182-182: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
184-184: Use kwargs.get("metadata") instead of kwargs.get("metadata", None)
Replace kwargs.get("metadata", None) with kwargs.get("metadata")
(SIM910)
186-186: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
188-188: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
fiftyone/operators/operations.py (2)
679-681: LGTM!
The close_sample method is well-implemented and follows the established patterns in the codebase.
668-681: Verify relationship with PR objectives.
The PR objectives mention improving performance for the Selection tree component, but these new methods (open_sample and close_sample) appear to be focused on modal management functionality. Could you clarify how these changes contribute to the performance improvements mentioned in the PR objectives?
docs/source/plugins/using_plugins.rst (1)
894-901: Verify the impact on existing deployments.
This configuration change might affect existing FiftyOne Open Source deployments that use delegated operations.
✅ Verification successful
Configuration change impact is limited and well-documented
Based on the codebase analysis, this configuration change is properly documented across the system:
- It's defined in the core configuration (
fiftyone/core/config.py) with a default value ofFalse - The configuration is documented in the main config reference (
docs/source/user_guide/config.rst) - The change affects delegated operations scheduling, which is an optional feature as indicated in the integrations documentation
- The configuration is properly guarded in the executor implementation (
fiftyone/operators/executor.py)
The impact is minimal since:
- The default value is
False, meaning existing deployments aren't affected unless they explicitly use delegated operations - The documentation clearly states when this flag is needed
- The feature appears to be opt-in rather than a breaking change to existing functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing documentation or migration guides that mention this configuration
# Expected results: Find any related documentation about this change to ensure consistent messaging
# Search for any existing mentions of this configuration
rg -i "FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS|allow_legacy_orchestrators"
# Look for migration guides or upgrade notes
fd -g "*MIGRATION*" -g "*UPGRADE*" -g "CHANGELOG*"
Length of output: 1197
Script:
#!/bin/bash
# Let's check for any recent changes or commits related to this configuration
# to understand when it was introduced and its purpose
# Search for any recent changes in git history related to this config
git log --grep="legacy.*orchestrator\|orchestrator.*legacy" --pretty=format:"%h %s" -n 5
# Check for any tests or code that might be affected by this configuration
rg -l "LegacyOrchestrator"
# Look for any documentation about orchestrators or execution modes
rg -i "orchestrator.*mode|execution.*mode" docs/
Length of output: 438
app/packages/operators/src/state.ts (2)
241-242: Type definition improvement enhances flexibility
The changes to OperatorExecutionOption improve type safety and flexibility:
- Making
onClickoptional allows for disabled options - Supporting
React.ReactNodefor description enables rich content rendering
261-261: Variable naming improvement for clarity
The renamed variable hasAvailableOrchestrators better reflects its purpose and maintains consistency with the codebase terminology.
Also applies to: 310-310
app/packages/operators/src/built-in-operators.ts (3)
1364-1391: LGTM! Well-structured operator implementation.
The OpenSample operator is well-implemented with:
- Clear input validation for sample and group IDs
- Proper use of hooks for expanded state management
- Consistent operator pattern implementation
1393-1410: LGTM! Clean and focused implementation.
The CloseSample operator provides a clean counterpart to OpenSample with:
- Appropriate use of modal clearing hook
- Minimal and focused implementation
1463-1464: LGTM! Proper operator registration.
Both operators are correctly registered as built-in operators with appropriate error handling.
tests/unittests/operators/delegated_tests.py (2)
215-215: LGTM: Improved metadata initialization
The change from None to {} for default metadata is a good practice as it ensures consistent type handling.
Line range hint 217-224: LGTM: Good test coverage for custom metadata
The addition of a test case for custom metadata handling improves the test coverage by verifying both default and custom metadata scenarios.
docs/source/user_guide/config.rst (1)
183-183: LGTM! Example outputs are consistent.
The new configuration field is properly added to both Python and CLI example outputs, maintaining consistency with the table definition.
Also applies to: 233-233
docs/source/plugins/developing_plugins.rst (3)
Line range hint 1-24: LGTM! Well-structured introduction.
The introduction section provides a clear overview of plugin development and includes helpful references to example plugins. The note about the FiftyOne plugins repository is particularly useful for developers.
Line range hint 91-157: LGTM! Clear explanation of plugin types.
The section on plugin types effectively explains the differences between Python and JavaScript plugins, including their capabilities and requirements.
Line range hint 1026-1124: LGTM! Comprehensive advanced usage section.
The advanced usage section effectively covers custom runs and their management, providing clear examples and explanations.
tests/unittests/lightning_tests.py (3)
1056-1139: LGTM!
The new test class TestGroupDatasetLightningQueries is well-implemented and effectively tests querying grouped datasets with different slices. The test cases correctly verify the functionality and expected results for "one" and "two" slices.
Line range hint 1155-1170: LGTM!
The modifications to the _execute function, including the addition of frames and slice parameters, enhance flexibility and align with the updated testing requirements. The default parameter values maintain backward compatibility.
1178-1194: LGTM!
The _get_paths function is appropriately updated to conditionally include frame fields based on the frames parameter. This change ensures that paths are correctly constructed for both sample and frame fields.
fiftyone/operators/executor.py (4)
261-261: Appropriate setting of 'delegated' flag in request parameters
The addition of ctx.request_params["delegated"] = True ensures that the delegated status is clearly communicated within the execution context.
695-695: Addition of delegated property to ExecutionContext
The new property method delegated correctly retrieves the 'delegated' status from request_params.
700-700: Addition of requesting_delegated_execution property to ExecutionContext
The property requesting_delegated_execution appropriately accesses the 'request_delegation' flag from request_params, allowing operators to determine if delegated execution was requested.
1278-1278: Update orchestrator_registration_enabled to reflect configuration
The property now returns not fo.config.allow_legacy_orchestrators, correctly aligning the behavior with the configuration setting and ensuring orchestrator registration is enabled only when legacy orchestrators are not allowed.
|
Yeah, picked the wrong branch. |
What changes are proposed in this pull request?
Previously, the python panel component would initialize with all children selected on render. This cause flashes and unstable performance issues when syncing the selected state among the grid, python panel state, and the selection tree component.
This PR reverted the initial setting to not selected state.
How is this patch tested? If it is not, please explain why.
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
Bug Fixes