Merge release/v1.2.0 to develop
#5302
Conversation
fix load_evaluation bug with serialized_info
WalkthroughThe pull request introduces modifications across several components related to model evaluation. Key changes include repositioning the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🔇 Additional comments (1)setup.py (1)
The minimum required version of voxel51-eta has been bumped from 0.13.0 to 0.13.1. Please ensure that:
✅ Verification successfulPackage version 0.13.1 is available and usage is minimal The verification shows that:
Let's verify if there are any breaking changes by checking the changelog or release notes: Let's check the changelog for version 0.13.1 to verify if there are any breaking changes: Package version 0.13.1 is compatible and safe to use The verification shows that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify eta package version and changelog
# Check if the required version exists on PyPI
curl -s https://pypi.org/pypi/voxel51-eta/json | jq '.releases | keys[]' | grep "0.13.1"
# Look for imports of eta to understand usage
echo "Checking eta usage patterns..."
rg "from eta\." -A 2
Length of output: 391 Script: #!/bin/bash
# Check git repository for changelog or release notes
fd -i changelog
fd -i release
# Check for any potential breaking changes in the core modules we use
rg "ConfigError|core\.learning|core\.models" -A 2
Length of output: 63530 Script: #!/bin/bash
# Check for changelog in eta repository
curl -s https://raw.githubusercontent.com/voxel51/eta/v0.13.1/CHANGELOG.md || curl -s https://raw.githubusercontent.com/voxel51/eta/main/CHANGELOG.md
# Also check for any potential breaking changes in core functionality we use
rg "ConfigError|core\.learning|core\.models" -C 5 fiftyone/utils/eta.py
Length of output: 2115 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: 0
🔭 Outside diff range comments (1)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx (1)
Line range hint
25-29: Consider using a more specific permissions typeThe current
Record<string, boolean>type is too generic. Consider creating a specific type for the permissions:+type EvaluatePermissions = { + can_evaluate: boolean; + // Add other known permissions here +}; type EvaluateProps = { variant: "empty" | "overview"; onEvaluate: () => void; - permissions: Record<string, boolean>; + permissions: EvaluatePermissions; };This provides better type safety and auto-completion for permission keys.
🧹 Nitpick comments (6)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
614-633: Improved permission handling and user feedback for evaluation notes.The changes enhance the UX by:
- Visually indicating permission status through cursor styles
- Providing clear feedback via tooltip when user lacks permission
- Properly disabling the edit button when user lacks permission
Consider extracting this permission-aware button pattern into a reusable component since similar permission-based UI patterns might be needed elsewhere:
+interface PermissionAwareIconButtonProps { + canEdit: boolean; + onClick: () => void; + icon: React.ReactNode; + disabledTooltip: string; +} + +const PermissionAwareIconButton: React.FC<PermissionAwareIconButtonProps> = ({ + canEdit, + onClick, + icon, + disabledTooltip +}) => ( + <Box + title={canEdit ? "" : disabledTooltip} + sx={{ cursor: canEdit ? "pointer" : "not-allowed" }} + > + <IconButton + size="small" + color="secondary" + sx={{ borderRadius: 16 }} + onClick={onClick} + disabled={!canEdit} + > + {icon} + </IconButton> + </Box> +);Then use it like:
-<Box - title={can_edit_note ? "" : "You do not have permission to edit evaluation notes"} - sx={{ cursor: can_edit_note ? "pointer" : "not-allowed" }} -> - <IconButton - size="small" - color="secondary" - sx={{ borderRadius: 16 }} - onClick={() => { - setEditNoteState((note) => ({ ...note, open: true })); - }} - disabled={!can_edit_note} - > - <EditNote /> - </IconButton> -</Box> +<PermissionAwareIconButton + canEdit={can_edit_note} + onClick={() => setEditNoteState((note) => ({ ...note, open: true }))} + icon={<EditNote />} + disabledTooltip="You do not have permission to edit evaluation notes" +/>app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx (2)
7-8: Consider memoizing the permission checkFor better performance in cases of frequent parent re-renders, consider using
useMemo:- const { onEvaluate, permissions } = props; - const canEvaluate = permissions.can_evaluate; + const { onEvaluate, permissions } = props; + const canEvaluate = React.useMemo( + () => permissions.can_evaluate, + [permissions.can_evaluate] + );
10-13: Simplify the title conditionalThe empty string title when
canEvaluateis true is unnecessary. Consider simplifying:- title={canEvaluate ? "" : "You do not have permission to evaluate model"} + title={!canEvaluate ? "You do not have permission to evaluate model" : undefined}app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx (2)
7-7: Props and conditional rendering look good, consider making readOnly requiredThe separation between
readOnly(view mode) andcanEdit(permissions) is well-implemented. The conditional rendering and disabled state handling are clear and provide good user feedback.Consider making the
readOnlyprop required since it's used for core rendering logic:type StatusProps = { status?: string; canEdit?: boolean; - readOnly?: boolean; + readOnly: boolean; setStatusEvent: string; };Also applies to: 10-30
Line range hint
1-89: Consider performance and architecture improvementsWhile the component is well-structured, consider these improvements:
- Memoize the component since it's likely used in lists:
- Consider moving color constants to a shared theme file for consistency
Example memoization:
-export default function Status(props: StatusProps) { +const Status = React.memo(function Status(props: StatusProps) { // ... component implementation -} +}); + +export default Status;app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.tsx (1)
100-100: Status component integration looks good, consider performance optimizationThe
readOnlyprop is correctly passed to the Status component. Since this is rendered within a list, consider the following optimization:
- Memoize the EvaluationCard component to prevent unnecessary re-renders:
-function EvaluationCard(props: EvaluationCardProps) { +const EvaluationCard = React.memo(function EvaluationCard(props: EvaluationCardProps) { // ... component implementation -} +});
- Use a callback for the onClick handler to prevent recreation on each render:
-onClick={() => { - onSelect(eval_key, id); -}} +onClick={React.useCallback(() => { + onSelect(eval_key, id); +}, [eval_key, id, onSelect])}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.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/NativeModelEvaluationView/Evaluate.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/NativeModelEvaluationView/Status.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/NativeModelEvaluationView/Evaluation.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.
🔇 Additional comments (2)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx (1)
14-21: LGTM! Button implementation follows best practices
The button implementation correctly handles permissions and provides appropriate visual feedback through the disabled state.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx (1)
72-73: Type changes improve type safety
Making setStatusEvent required is a good change since it's used unconditionally in the component. The types are well-defined and properly reflect the component's requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
1666-1682: Consider improving code readability.While the logic is correct, the code could be more readable with destructuring and intermediate variables.
Consider this refactor:
- const originalClasses = matrices[`${sortBy}_classes`]; - const originalMatrix = matrices[`${sortBy}_matrix`]; - const classes = originalClasses.slice(0, parsedLimit); - const matrix = originalMatrix.slice(0, parsedLimit); + const { + [`${sortBy}_classes`]: originalClasses, + [`${sortBy}_matrix`]: originalMatrix, + [`${sortBy}_colorscale`]: colorscale + } = matrices; + + const classes = originalClasses.slice(0, parsedLimit); + const matrix = originalMatrix.slice(0, parsedLimit); + + const shouldAppendNoneClass = parsedLimit < originalClasses.length; + if (shouldAppendNoneClass) { + const noneIndex = originalClasses.indexOf(NONE_CLASS); + const noneLabel = compareMaskTargets?.[NONE_CLASS] || + maskTargets?.[NONE_CLASS] || + NONE_CLASS; + + labels.push(noneLabel); + matrix.push(originalMatrix[noneIndex]); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(4 hunks)fiftyone/operators/builtins/panels/model_evaluation/__init__.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fiftyone/operators/builtins/panels/model_evaluation/init.py
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.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.
🔇 Additional comments (3)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (3)
54-54: LGTM!
The constant follows TypeScript naming conventions and is appropriately placed with other constants.
615-634: LGTM! Well-implemented permission handling.
The changes improve user experience by:
- Providing clear visual feedback through cursor styling
- Adding a helpful tooltip when permission is denied
- Properly disabling the edit button
1708-1708: LGTM! Good defensive programming.
The addition of the fallback for undefined filter prevents potential runtime errors.
Fixes an issue where splitting the panel space wrongly triggers on_unload
[docs] update data lens documentation to include additional examples
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/packages/operators/src/CustomPanel.tsx (1)
Line range hint
36-43: Add missing dependency to useEffectThe cleanup effect uses
props.onUnLoadbut doesn't include it in the dependency array. This could lead to stale closures if the callback changes during the component's lifecycle.Apply this change:
useEffect(() => { setPanelCloseEffect(() => { clearUseKeyStores(panelId); trackEvent("close_panel", { panel: panelName }); triggerPanelEvent(panelId, { operator: props.onUnLoad }); }); - }, []); + }, [panelId, panelName, props.onUnLoad, setPanelCloseEffect, trackEvent, triggerPanelEvent]);🧰 Tools
🪛 GitHub Check: lint / eslint
[warning] 43-43:
React Hook useEffect has missing dependencies: 'panelId', 'panelName', 'props.onUnLoad', 'setPanelCloseEffect', 'trackEvent', and 'triggerPanelEvent'. Either include them or remove the dependency array
🧹 Nitpick comments (3)
app/packages/operators/src/CustomPanel.tsx (1)
41-41: Add type safety check for onUnLoad callbackConsider adding a type guard before triggering the panel event to ensure type safety and prevent potential runtime errors.
Apply this change:
- triggerPanelEvent(panelId, { operator: props.onUnLoad }); + if (typeof props.onUnLoad === 'function') { + triggerPanelEvent(panelId, { operator: props.onUnLoad }); + }docs/source/teams/data_lens.rst (2)
827-827: Consider making the polling interval configurable.The hardcoded sleep duration of 2.5 seconds in the polling loop could be made configurable to allow for different Databricks environments and workloads.
- time.sleep(2.5) + # Configure polling interval through operator config or environment + polling_interval = float(ctx.secret("DATABRICKS_POLLING_INTERVAL", "2.5")) + time.sleep(polling_interval)
746-762: Consider enhancing warehouse selection logic.The current warehouse selection logic is basic and could be improved to:
- Consider warehouse size/type for the workload
- Handle cases where all warehouses are busy
- Implement retry logic for warehouse startup failures
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/packages/operators/src/CustomPanel.tsx(2 hunks)app/packages/operators/src/useCustomPanelHooks.ts(0 hunks)docs/source/teams/data_lens.rst(4 hunks)
💤 Files with no reviewable changes (1)
- app/packages/operators/src/useCustomPanelHooks.ts
🧰 Additional context used
📓 Path-based instructions (1)
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.
🔇 Additional comments (1)
app/packages/operators/src/CustomPanel.tsx (1)
17-17: LGTM: Import follows React hook conventions
The import statement follows React hook naming conventions and is properly organized with other hook imports.
| while ( | ||
| statement_response.status.state in | ||
| (StatementState.PENDING, StatementState.RUNNING) | ||
| ): | ||
| statement_response = self.client.statement_execution.get_statement( | ||
| statement_response.statement_id | ||
| ) | ||
|
|
||
| time.sleep(2.5) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add retry mechanism for transient failures.
The polling loop should implement a retry mechanism with exponential backoff for transient failures, and handle cases where the statement execution takes longer than expected.
+ retry_count = 0
+ max_retries = 3
+ base_delay = 2.5
while (
statement_response.status.state in
(StatementState.PENDING, StatementState.RUNNING)
):
- statement_response = self.client.statement_execution.get_statement(
- statement_response.statement_id
- )
- time.sleep(2.5)
+ try:
+ statement_response = self.client.statement_execution.get_statement(
+ statement_response.statement_id
+ )
+ retry_count = 0 # Reset on success
+ time.sleep(base_delay)
+ except Exception as e:
+ retry_count += 1
+ if retry_count > max_retries:
+ raise
+ time.sleep(base_delay * (2 ** retry_count)) # Exponential backoff📝 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.
| while ( | |
| statement_response.status.state in | |
| (StatementState.PENDING, StatementState.RUNNING) | |
| ): | |
| statement_response = self.client.statement_execution.get_statement( | |
| statement_response.statement_id | |
| ) | |
| time.sleep(2.5) | |
| retry_count = 0 | |
| max_retries = 3 | |
| base_delay = 2.5 | |
| while ( | |
| statement_response.status.state in | |
| (StatementState.PENDING, StatementState.RUNNING) | |
| ): | |
| try: | |
| statement_response = self.client.statement_execution.get_statement( | |
| statement_response.statement_id | |
| ) | |
| retry_count = 0 # Reset on success | |
| time.sleep(base_delay) | |
| except Exception as e: | |
| retry_count += 1 | |
| if retry_count > max_retries: | |
| raise | |
| time.sleep(base_delay * (2 ** retry_count)) # Exponential backoff |
| def _remap_sample_fields(self, sample: dict, request: DataLensSearchRequest): | ||
| remappable_fields = ("field_a", "field_b") | ||
| for field_name in remappable_fields: | ||
| remapped_field_name = request.search_params.get(f"{field_name}_remap") | ||
| if remapped_field_name not in (None, ""): | ||
| sample[remapped_field_name] = sample[field_name] | ||
| del sample[field_name] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add field validation and security considerations.
The field remapping example should include:
- Validation of remapped field names to prevent injection attacks
- Warning about potential security implications
- Best practices for field name validation
def _remap_sample_fields(self, sample: dict, request: DataLensSearchRequest):
remappable_fields = ("field_a", "field_b")
for field_name in remappable_fields:
remapped_field_name = request.search_params.get(f"{field_name}_remap")
if remapped_field_name not in (None, ""):
+ # Validate remapped field name to prevent injection attacks
+ if not self._is_valid_field_name(remapped_field_name):
+ raise ValueError(f"Invalid field name: {remapped_field_name}")
sample[remapped_field_name] = sample[field_name]
del sample[field_name]
+ def _is_valid_field_name(self, field_name: str) -> bool:
+ """Validate field name against allowed patterns."""
+ import re
+ return bool(re.match(r'^[a-zA-Z][a-zA-Z0-9_]*$', field_name))Committable suggestion skipped: line range outside the PR's diff.
Bump to ETA v0.13.1
Merge
release/v1.2.0todevelopSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
StopIterationerrors during long-running API operations.Documentation
Chores
voxel51-eta.