Merge release/v1.3.0 to develop
#5402
Conversation
WalkthroughThis pull request introduces significant changes across multiple packages, primarily focusing on removing cancel functionality from operator execution and standardizing device management for machine learning models. The changes span TypeScript/React components in the operators package, Python utilities for various ML frameworks, and core dataset handling in FiftyOne. The modifications simplify component interactions, remove explicit cancellation callbacks, and enhance device configuration flexibility for model execution. Changes
Sequence DiagramsequenceDiagram
participant User
participant OperatorExecutionButton
participant OperatorExecutionTrigger
participant Operator
User->>OperatorExecutionButton: Click
OperatorExecutionButton->>OperatorExecutionTrigger: Trigger execution
OperatorExecutionTrigger->>Operator: Execute
Operator-->>OperatorExecutionTrigger: Return result
OperatorExecutionTrigger-->>OperatorExecutionButton: Notify completion
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 comments (1)
app/packages/operators/src/built-in-operators.ts (1)
Line range hint
1048-1073: Improve type safety and error handling.The code could benefit from the following improvements:
- Add type safety:
- callback: (result: OperatorResult, opts: { ctx: ExecutionContext }) => { + callback: (result: OperatorResult & { error?: Error; result?: unknown }, opts: { ctx: ExecutionContext }) => {
- Add parameter validation:
const { params, operator_uri, on_success, on_error } = ctx.params; + if (!operator_uri) { + throw new Error("operator_uri is required"); + }
- Add error handling for callbacks:
callback: (result: OperatorResult, opts: { ctx: ExecutionContext }) => { const ctx = opts.ctx; + try { if (result.error) { if (on_error) { triggerEvent(panelId, { operator: on_error, params: { error: result.error }, }); } } else { if (on_success) { triggerEvent(panelId, { operator: on_success, params: { result: result.result, original_params: ctx.params }, }); } } + } catch (error) { + console.error("Error in operation callback:", error); + } },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/TooltipProvider.tsx(1 hunks)app/packages/operators/src/built-in-operators.ts(1 hunks)app/packages/operators/src/components/OperatorExecutionButton/index.tsx(1 hunks)app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx(3 hunks)app/packages/operators/src/state.ts(1 hunks)app/packages/operators/src/types-internal.ts(0 hunks)app/packages/operators/src/usePanelEvent.ts(2 hunks)fiftyone/operators/executor.py(0 hunks)
💤 Files with no reviewable changes (2)
- app/packages/operators/src/types-internal.ts
- fiftyone/operators/executor.py
✅ Files skipped from review due to trivial changes (1)
- app/packages/core/src/plugins/SchemaIO/components/TooltipProvider.tsx
🧰 Additional context used
📓 Path-based instructions (6)
app/packages/operators/src/usePanelEvent.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/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.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/components/OperatorExecutionTrigger/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/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/operators/src/components/OperatorExecutionButton/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.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (9)
app/packages/operators/src/components/OperatorExecutionButton/index.tsx (1)
Line range hint
1-63: LGTM! Clean and well-documented component.The simplified implementation improves maintainability by:
- Removing unnecessary props and complexity
- Maintaining clear TypeScript types
- Providing up-to-date documentation
app/packages/operators/src/usePanelEvent.ts (2)
23-23: LGTM! Consistent with cancellation handling removal.The removal of
onCancelfrom the destructured options aligns with the broader refactoring of cancellation handling across the codebase.
52-52: LGTM! Simplified promptForOperator call.The removal of the cancellation callback from the
promptForOperatorcall maintains consistency with the updated operator execution flow.app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (1)
100-101: LGTM! Improved icon handling logic.The direct assignment of
startIconandendIconprops simplifies the code by:
- Removing conditional icon prop handling
- Making the icon position logic more explicit
- Following React best practices for prop assignment
app/packages/operators/src/state.ts (1)
659-659: LGTM! Simplified cancellation handling.The direct assignment of
closetocancelsimplifies the code while maintaining proper state cleanup functionality.app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (4)
1-13: LGTM! Well-organized imports with proper type definitions.The imports are properly structured with clear separation between React core, MUI components, and internal modules.
Line range hint
40-63: Props interface is well-designed and documented.The component's props are:
- Well-typed with proper optional markers
- Clearly documented with comprehensive JSDoc
- Following TypeScript best practices
86-89: LGTM! Clean and focused hook implementation.The
useOperatorExecutionOptionshook is used effectively with only the essential parameters, aligning well with the refactoring goals of simplifying the operator execution flow.
Line range hint
102-120: LGTM! Clean and efficient render implementation.The component's render logic:
- Properly manages menu state and anchoring
- Correctly handles disabled state
- Efficiently passes props to child components
| // Pass onSuccess and onError through to the operator executor. | ||
| // These will be invoked on operator completion. | ||
| const operatorHandlers = useMemo(() => { | ||
| return { onSuccess, onError }; | ||
| }, [onSuccess, onError]); | ||
| const operator = useOperatorExecutor(operatorUri, operatorHandlers); | ||
|
|
||
| // This callback will be invoked when an execution target option is clicked | ||
| const onExecute = useCallback( | ||
| (options?: OperatorExecutorOptions) => { | ||
| const resolvedOptions = { | ||
| ...executorOptions, | ||
| ...options, | ||
| }; | ||
|
|
||
| return operator.execute(executionParams ?? {}, resolvedOptions); | ||
| }, | ||
| [executorOptions, operator, executionParams] | ||
| ); |
There was a problem hiding this comment.
Add missing dependencies to onExecute callback.
The onExecute callback should include onSuccess and onError in its dependency array as these could change between renders. Without these dependencies, stale callbacks might be used during execution.
Apply this diff to fix the dependency array:
const onExecute = useCallback(
(options?: OperatorExecutorOptions) => {
const resolvedOptions = {
...executorOptions,
...options,
};
return operator.execute(executionParams ?? {}, resolvedOptions);
},
- [executorOptions, operator, executionParams]
+ [executorOptions, operator, executionParams, onSuccess, onError]
);📝 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.
| // Pass onSuccess and onError through to the operator executor. | |
| // These will be invoked on operator completion. | |
| const operatorHandlers = useMemo(() => { | |
| return { onSuccess, onError }; | |
| }, [onSuccess, onError]); | |
| const operator = useOperatorExecutor(operatorUri, operatorHandlers); | |
| // This callback will be invoked when an execution target option is clicked | |
| const onExecute = useCallback( | |
| (options?: OperatorExecutorOptions) => { | |
| const resolvedOptions = { | |
| ...executorOptions, | |
| ...options, | |
| }; | |
| return operator.execute(executionParams ?? {}, resolvedOptions); | |
| }, | |
| [executorOptions, operator, executionParams] | |
| ); | |
| // Pass onSuccess and onError through to the operator executor. | |
| // These will be invoked on operator completion. | |
| const operatorHandlers = useMemo(() => { | |
| return { onSuccess, onError }; | |
| }, [onSuccess, onError]); | |
| const operator = useOperatorExecutor(operatorUri, operatorHandlers); | |
| // This callback will be invoked when an execution target option is clicked | |
| const onExecute = useCallback( | |
| (options?: OperatorExecutorOptions) => { | |
| const resolvedOptions = { | |
| ...executorOptions, | |
| ...options, | |
| }; | |
| return operator.execute(executionParams ?? {}, resolvedOptions); | |
| }, | |
| [executorOptions, operator, executionParams, onSuccess, onError] | |
| ); |
…ti-gpu-setups Bugfix: use device in all Torch models
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)
470-487: Consider using more descriptive id values.While the implementation is good, the boolean id values (
true/false) could be more descriptive. Consider using string identifiers that match the metrics they represent.- id: true, + id: "correct", property: "Correct", // ... }, { - id: false, + id: "incorrect", property: "Incorrect", // ... },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
fiftyone/zoo/models/manifest-torch.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(2 hunks)fiftyone/utils/clip/zoo.py(1 hunks)fiftyone/utils/open_clip.py(3 hunks)fiftyone/utils/super_gradients.py(1 hunks)fiftyone/utils/transformers.py(14 hunks)fiftyone/utils/ultralytics.py(3 hunks)plugins/panels/model_evaluation/__init__.py(2 hunks)
🔥 Files not summarized due to errors (1)
- fiftyone/utils/clip/zoo.py: Error: Disallowed special token found: <|endoftext|>
🧰 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 (15)
plugins/panels/model_evaluation/__init__.py (2)
328-332: Well implemented calculation of correct/incorrect predictions!The implementation efficiently uses numpy's vectorized operations to calculate the counts, which is good for performance.
371-379: Clean integration of the new metrics!The conditional block correctly identifies non-binary classification cases and integrates the new metrics into the existing metrics dictionary.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
213-214: Clear and descriptive boolean flag!The variable name and condition clearly express the intent to identify non-binary classification cases.
fiftyone/utils/super_gradients.py (1)
99-99: LGTM! Enhanced device flexibility.The change from
model.cuda()tomodel.to(self.device)improves code flexibility by allowing the model to be moved to any specified device rather than being limited to GPU.fiftyone/utils/open_clip.py (4)
109-109: LGTM! Enhanced device flexibility.The change from
.cuda()to.to(self.device)improves code flexibility by allowing tensors to be moved to any specified device.
121-121: LGTM! Consistent device handling.The change maintains consistency with the device handling pattern used throughout the file.
146-146: LGTM! Consistent tensor device allocation.The change follows the same pattern of flexible device allocation for tensors.
148-150: LGTM! Improved device type handling in autocast.The change enhances the
torch.amp.autocastcontext by dynamically setting the device type based on the current device, improving compatibility across different hardware configurations.fiftyone/utils/transformers.py (4)
326-328: LGTM! Added configurable device support.The addition of the
deviceconfiguration parameter with a smart default enhances flexibility by allowing users to specify the target device while maintaining backward compatibility.
457-458: LGTM! Improved device initialization.The change properly initializes the device and moves the model to it, following PyTorch best practices.
503-504: LGTM! Consistent device handling.The change maintains consistency with the device handling pattern used throughout the codebase.
589-589: LGTM! Unified device configuration across transformer classes.The changes consistently implement device configuration across all transformer classes, improving code maintainability and reducing duplication.
Also applies to: 701-701, 832-832, 886-886, 938-938
fiftyone/utils/ultralytics.py (3)
23-23: LGTM! Added PyTorch import.Added PyTorch import to support device management functionality.
382-384: LGTM! Added configurable device support.The addition of the
deviceconfiguration parameter with a smart default enhances flexibility by allowing users to specify the target device while maintaining backward compatibility.
397-398: LGTM! Improved device initialization.The change properly initializes the device and moves the model to it, following PyTorch best practices.
Use estimated document count when possible for `count()` operations
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fiftyone/core/collections.py (1)
10575-10590: Simplify boolean returnThe boolean expression can be returned directly instead of using an if-statement.
- if ( - isinstance(self, fov.DatasetView) - and self._dataset.media_type == fom.GROUP - and len(self._stages) == 1 - and isinstance(self._stages[0], fos.SelectGroupSlices) - and self._pipeline() == [] - ): - return True - - return False + return ( + isinstance(self, fov.DatasetView) + and self._dataset.media_type == fom.GROUP + and len(self._stages) == 1 + and isinstance(self._stages[0], fos.SelectGroupSlices) + and self._pipeline() == [] + )🧰 Tools
🪛 Ruff (0.8.2)
10580-10589: Return the condition directly
Inline condition
(SIM103)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fiftyone/core/collections.py(7 hunks)fiftyone/core/dataset.py(7 hunks)requirements/common.txt(1 hunks)requirements/extras.txt(1 hunks)requirements/test.txt(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements/common.txt
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/core/collections.py
10580-10589: Return the condition directly
Inline condition
(SIM103)
fiftyone/core/dataset.py
1241-1241: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
1256-1256: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (13)
requirements/extras.txt (1)
4-4: Consider maintaining an upper version bound for ipywidgets.Removing the upper version constraint (
<8) could lead to compatibility issues, as ipywidgets 8.x introduced breaking changes. Consider:
- Testing compatibility with newer versions
- Maintaining an upper bound for stability
- Documenting any known compatibility issues
Let's verify the compatibility implications:
✅ Verification successful
Requirements change is safe due to internal version enforcement
The codebase internally enforces
ipywidgets>=8,<9throughensure_package(), making the less restrictive requirement inrequirements/extras.txtsafe. The internal check prevents any compatibility issues with versions >=9.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes and compatibility issues with ipywidgets # Get latest version and changelog curl -s https://pypi.org/pypi/ipywidgets/json | jq -r '.releases | keys[]' | sort -V | tail -n 5 curl -s https://raw.githubusercontent.com/jupyter-widgets/ipywidgets/main/CHANGELOG.md # Search for ipywidgets usage in the codebase to assess impact rg -l 'ipywidgets' | grep '\.py$'Length of output: 295
Script:
#!/bin/bash # Check how ipywidgets is used in the plotly.py file cat fiftyone/core/plots/plotly.py | grep -A 5 -B 5 'ipywidgets' # Check if there are any imports or specific widget usage rg -A 5 -B 5 'from ipywidgets|import ipywidgets' fiftyone/core/plots/plotly.pyLength of output: 657
fiftyone/core/dataset.py (4)
363-370: LGTM! Efficient implementation for estimating document counts.The method provides a performant way to estimate collection sizes by leveraging MongoDB's
estimated_document_count().
Line range hint
1337-1350: LGTM! Well-implemented method with proper error handling.The method correctly handles edge cases and provides clear error messages when no matches are found or when multiple matches are found with
exact=True.
1236-1243: 🛠️ Refactor suggestionImprove error handling to preserve exception context.
Chain the exception to preserve the error context when the dataset is empty.
Apply this diff:
try: d = next(cursor) except StopIteration as err: - raise ValueError("%s is empty" % self.__class__.__name__) + raise ValueError("%s is empty" % self.__class__.__name__) from errLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
1241-1241: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
1251-1258: 🛠️ Refactor suggestionImprove error handling to preserve exception context.
Chain the exception to preserve the error context when the dataset is empty.
Apply this diff:
try: d = next(cursor) except StopIteration as err: - raise ValueError("%s is empty" % self.__class__.__name__) + raise ValueError("%s is empty" % self.__class__.__name__) from errLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
1256-1256: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
fiftyone/core/collections.py (6)
Line range hint
926-935: LGTM! Makingfirst()abstractGood refactoring to make this method abstract, requiring subclasses to provide their own implementation.
Line range hint
935-950: LGTM! Makinglast()abstractGood refactoring to make this method abstract, requiring subclasses to provide their own implementation.
947-950: LGTM! Makinghead()abstractGood refactoring to make this method abstract, requiring subclasses to provide their own implementation.
962-965: LGTM! Makingtail()abstractGood refactoring to make this method abstract, requiring subclasses to provide their own implementation.
982-985: LGTM! Makingone()abstractGood refactoring to make this method abstract, requiring subclasses to provide their own implementation.
7683-7698: Performance optimization for count()Good optimization to use estimated document count when possible, avoiding full collection scan.
requirements/test.txt (2)
2-2: Verify security advisories for packages with loose constraints.Let's check for known vulnerabilities in packages with loose version constraints (
open3d>=0.16.0,twine>=3).Also applies to: 11-11
✅ Verification successful
No security vulnerabilities found for packages with loose constraints
The GitHub Security Advisory Database shows no known vulnerabilities for either open3d (>=0.16.0) or twine (>=3).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check security advisories for packages with loose constraints # Check for security advisories for open3d and twine for package in "open3d" "twine"; do echo "Checking $package..." gh api graphql -f query=" { securityVulnerabilities(first: 5, ecosystem: PIP, package: \"$package\") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }" doneLength of output: 897
1-1: Verify compatibility of awscli version.Let's verify the stability and compatibility of the newly added awscli package.
✅ Verification successful
✓ awscli 1.37.2 is compatible with project requirements
- Python version requirement (>=3.9) is satisfied
- Compatible with existing boto3 1.36.2
- No security vulnerabilities found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check awscli package information and compatibility # Check PyPI for package info curl -s "https://pypi.org/pypi/awscli/1.37.2/json" | jq -r '.info.requires_python, .urls[].upload_time_iso_8601' | head -n 1 # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "awscli") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 470
Script:
#!/bin/bash # Check Python version requirements and AWS-related dependencies fd -e txt -e ini requirements | xargs cat rg -l "aws|boto" --type=python rg "python_requires|Python" pyproject.toml setup.py setup.cfg 2>/dev/null || trueLength of output: 473
Script:
#!/bin/bash # Check prod.txt for AWS-related dependencies cat requirements/prod.txtLength of output: 41
Script:
#!/bin/bash # Check common.txt for AWS-related dependencies cat requirements/common.txtLength of output: 576
* pin open3d * more than 16, less than 19 * remove itsdangerous
fix mime type error
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/packages/looker/src/worker/canvas-decoder.ts (1)
75-77: Consider additional image format validation.While using header validation is more reliable than MIME type checking, the current approach might attempt to process unsupported image formats. Consider:
- Adding explicit validation for supported formats
- Enhancing the comment to list all supported image formats
- if (blob.type !== "image/jpg" && blob.type !== "image/jpeg") { - // note that the following function doesn't rely on MIME type and instead reads the file header + // Supported formats: PNG, JPG/JPEG + // For PNGs: validate using file header + // For JPG/JPEG: rely on MIME type + if (!["image/jpg", "image/jpeg"].includes(blob.type)) {app/packages/operators/src/OperatorPalette.tsx (1)
160-160: Add JSDoc comment for the isExecuting prop.Consider adding documentation to explain the purpose and behavior of this prop:
+ /** Indicates whether an operation is in progress. When true, prevents the dialog from closing. */ isExecuting?: boolean;app/packages/looker/src/processOverlays.test.ts (2)
12-20: Add more test cases for comprehensive coverageWhile the existing tests are good, consider adding these scenarios:
- Invalid field names
- Empty bins object
- Multiple overlays in the same bin
- Edge cases for map/mask values (null, undefined)
Here's a suggested expansion of the test suite:
describe("test overlay processing", () => { // ... existing tests ... it("filters overlay with invalid field", () => { const overlay = new HeatmapOverlay("test", { ...EMPTY, id: "invalid" }); expect(filter(overlay, { valid: [] })).toBe(true); }); it("filters overlay with empty bins", () => { expect(filter(new HeatmapOverlay("test", EMPTY), {})).toBe(true); }); it("handles multiple overlays in same bin", () => { const bins = { test: [new HeatmapOverlay("test", EMPTY)] }; expect(filter(new HeatmapOverlay("test", EMPTY), bins)).toBe(true); }); });
7-10: Consider making EMPTY constant more type-safeThe
EMPTYconstant could benefit from explicit typing to ensure type safety.const EMPTY: OverlayConfig = { id: "", tags: [], };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (19)
app/package.jsonis excluded by!**/*.jsonapp/packages/aggregations/package.jsonis excluded by!**/*.jsonapp/packages/app/package.jsonis excluded by!**/*.jsonapp/packages/components/package.jsonis excluded by!**/*.jsonapp/packages/core/package.jsonis excluded by!**/*.jsonapp/packages/embeddings/package.jsonis excluded by!**/*.jsonapp/packages/flashlight/package.jsonis excluded by!**/*.jsonapp/packages/looker-3d/package.jsonis excluded by!**/*.jsonapp/packages/looker/package.jsonis excluded by!**/*.jsonapp/packages/map/package.jsonis excluded by!**/*.jsonapp/packages/operators/package.jsonis excluded by!**/*.jsonapp/packages/playback/package.jsonis excluded by!**/*.jsonapp/packages/plugins/package.jsonis excluded by!**/*.jsonapp/packages/relay/package.jsonis excluded by!**/*.jsonapp/packages/spaces/package.jsonis excluded by!**/*.jsonapp/packages/spotlight/package.jsonis excluded by!**/*.jsonapp/packages/state/package.jsonis excluded by!**/*.jsonapp/packages/utilities/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (4)
app/packages/looker/src/processOverlays.test.ts(1 hunks)app/packages/looker/src/processOverlays.ts(3 hunks)app/packages/looker/src/worker/canvas-decoder.ts(2 hunks)app/packages/operators/src/OperatorPalette.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/looker/src/processOverlays.test.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/looker/src/processOverlays.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/OperatorPalette.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/looker/src/worker/canvas-decoder.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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build / build
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
🔇 Additional comments (7)
app/packages/looker/src/worker/canvas-decoder.ts (1)
18-22: Great improvement in function naming and documentation!The rename to
getMaybePngcolorTypebetter reflects the function's behavior by explicitly indicating the possibility of returning undefined. The updated documentation also provides clearer expectations about the return value.app/packages/operators/src/OperatorPalette.tsx (2)
28-28: LGTM! Clean prop addition.The
isExecutingprop is well-positioned with other control-related props.
93-93: LGTM! Proper event handler assignment.The
handleClosefunction is correctly assigned to the Dialog'sonCloseprop.app/packages/looker/src/processOverlays.ts (4)
5-10: LGTM! Good use of type importsConverting
OverlayandBaseStateto type imports is a good practice as they're only used for type information. This helps with tree-shaking and bundle size optimization.
36-37: LGTM! Good extraction of filtering logicMoving the filtering logic to a separate function improves code organization and maintainability.
41-44: LGTM! More explicit array manipulationUsing
pushinstead of spread operator makes the code more explicit and potentially more performant for large arrays.
93-96: Address the TODO comment with a more robust solutionThe comment indicates a known limitation in the current implementation. Consider using a more flexible approach such as:
- Adding a
isValid()method to theOverlaybase class- Using a strategy pattern for different overlay types
Would you like me to propose a specific implementation for either approach?
Cherry Pick Open3d fix
add 16 bit png support
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/packages/looker/src/worker/custom-16-bit-png-decoder.ts (1)
4-25: Add error handling for potential decoding failuresThe
customDecode16BitPngfunction lacks error handling around thedecodeoperation. To prevent the application from crashing due to unexpected decoding errors, consider wrapping the decoding logic in a try-catch block.Apply this diff to include error handling:
export const customDecode16BitPng = async (blob: Blob) => { + let decodedPng; const arrayBuffer = await blob.arrayBuffer(); - const decodedPng = decode(arrayBuffer); + try { + decodedPng = decode(arrayBuffer); + } catch (e) { + throw new Error("Failed to decode PNG: " + e.message); + } if (decodedPng.palette?.length) { // spec doesn't allow palettes for 16-bit PNGs throw new Error("Indexed PNGs are not supported for 16-bit PNGs"); } const width = decodedPng.width; const height = decodedPng.height; const numChannels = decodedPng.channels ?? decodedPng.data.length / (width * height); return { buffer: decodedPng.data.buffer, channels: numChannels, arrayType: decodedPng.data.constructor.name as OverlayMask["arrayType"], shape: [height, width], } as OverlayMask; };app/packages/looker/src/worker/mask-decoder.test.ts (1)
74-88: Consider adding validation for slice parameters.The
createFakeBlobhelper could benefit from parameter validation to ensurestartandendare valid indices.slice(start: number, end: number) { + if (start < 0 || end > data.length || start > end) { + throw new Error("Invalid slice parameters"); + } return { async arrayBuffer() { return data.slice(start, end).buffer; }, }; },app/packages/looker/src/worker/canvas-decoder.ts (1)
Line range hint
63-83: Consider adding input validation for numOriginalChannels.The function should validate that
numOriginalChannelsis within a valid range (1-4) to prevent potential issues with buffer processing.export const decodeWithCanvas = async ( blob: Blob, cls: string, numOriginalChannels: number ) => { + if (numOriginalChannels < 1 || numOriginalChannels > 4) { + throw new Error("Number of channels must be between 1 and 4"); + } let channels: number = numOriginalChannels;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/packages/looker/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (6)
app/packages/looker/src/worker/canvas-decoder.ts(1 hunks)app/packages/looker/src/worker/custom-16-bit-png-decoder.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.test.ts(8 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)app/packages/looker/src/worker/mask-decoder.test.ts(1 hunks)app/packages/looker/src/worker/mask-decoder.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
app/packages/looker/src/worker/custom-16-bit-png-decoder.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/looker/src/worker/disk-overlay-decoder.test.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/looker/src/worker/disk-overlay-decoder.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/looker/src/worker/mask-decoder.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/looker/src/worker/mask-decoder.test.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/looker/src/worker/canvas-decoder.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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: build / build
🔇 Additional comments (7)
app/packages/looker/src/worker/mask-decoder.ts (1)
10-34: PNG header parsing is correctly implementedThe
getMaybePngHeaderfunction properly reads the PNG file signature and extracts the bit depth and color type from the image header. This implementation aligns with the PNG specification and correctly handles identification of PNG files.app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
117-122: Robust error handling for overlay mask decodingThe added check for
overlayMaskbeing falsy and the corresponding error message improve the robustness of thedecodeOverlayOnDiskfunction. This ensures that any issues during the mask decoding process are properly logged and handled.app/packages/looker/src/worker/mask-decoder.test.ts (2)
41-67: Well-documented test helper with real PNG data!The
createReal16BitPngBlobhelper function is well-documented with the Python code used to generate the test data. This makes it easy to regenerate or modify the test data if needed.
97-106: Comprehensive test coverage for different PNG types!The test suite effectively covers:
- Non-PNG files (JPEG)
- 16-bit PNG files
- 8-bit PNG files
Each test case validates the appropriate decoding path and results.
Also applies to: 108-120, 122-139
app/packages/looker/src/worker/disk-overlay-decoder.test.ts (2)
20-22: Clean mock setup for the new decoder.The mock setup is concise and follows the single responsibility principle by mocking only the necessary function.
71-71: Comprehensive test coverage for all overlay types!The test suite effectively covers:
- Segmentation overlays
- HEATMAP overlays
- DETECTIONS overlays (with recursive processing)
- Error handling for failed fetches
Each test case validates the appropriate decoding path and results.
Also applies to: 89-89, 107-107, 125-125, 151-151, 203-203
app/packages/looker/src/worker/canvas-decoder.ts (1)
41-46: Improved function signature with explicit channel information.The addition of
numOriginalChannelsparameter makes the function more explicit and removes the need for PNG-specific header parsing. This change:
- Simplifies the function's responsibility
- Makes it more reusable for different image formats
- Improves type safety
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/looker/src/worker/mask-decoder.ts (1)
11-35: Add error handling for malformed PNG headers.The PNG header parsing implementation is well-documented and follows the specification correctly. However, it could benefit from additional error handling for malformed PNGs.
Consider adding checks for:
const getMaybePngHeader = async ( blob: Blob ): Promise<{ bitDepth: number; colorType: number } | undefined> => { const header = new Uint8Array(await blob.slice(0, 29).arrayBuffer()); + if (header.length < 29) { + return undefined; + } // check PNG signature for (let i = 0; i < PNG_SIGNATURE.length; i++) { if (header[i] !== PNG_SIGNATURE[i]) { return undefined; } } + // Validate IHDR chunk type (bytes 12-15 should spell "IHDR") + if ( + header[12] !== 0x49 || // 'I' + header[13] !== 0x48 || // 'H' + header[14] !== 0x44 || // 'D' + header[15] !== 0x52 // 'R' + ) { + return undefined; + } + const bitDepth = header[24]; const colorType = header[25]; + + // Validate according to PNG spec + if (![1, 2, 4, 8, 16].includes(bitDepth) || + ![0, 2, 3, 4, 6].includes(colorType)) { + return undefined; + } + return { bitDepth, colorType }; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/packages/looker/src/worker/canvas-decoder.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.test.ts(8 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)app/packages/looker/src/worker/mask-decoder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker/src/worker/disk-overlay-decoder.ts
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/looker/src/worker/disk-overlay-decoder.test.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/looker/src/worker/canvas-decoder.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/looker/src/worker/mask-decoder.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.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (8)
app/packages/looker/src/worker/mask-decoder.ts (3)
37-44: LGTM: Function signature is well-defined.The updated function signature properly includes all necessary parameters for mask decoding with proper TypeScript types.
45-73: Consider not relying solely on blob.type for initial format check.Similar to the previous review comment, the function still relies on
blob.typefor the initial JPEG check.
75-76: LGTM: Proper parameter passing to decodeWithCanvas.The final decoding step correctly passes all required parameters to the canvas decoder.
app/packages/looker/src/worker/canvas-decoder.ts (2)
46-49: LGTM: Function signature updated correctly.The function signature has been properly updated to include the new parameters with appropriate TypeScript types.
50-50: LGTM: Simplified channel handling.The channel handling has been simplified by directly using the passed parameter, reducing complexity.
app/packages/looker/src/worker/disk-overlay-decoder.test.ts (3)
7-9: LGTM: Imports and mocks updated correctly.The file correctly updates the imports and adds proper mocking for the new decodeMaskOnDisk function.
Also applies to: 20-22
Line range hint
89-93: LGTM: Test assertions properly updated.The test assertions have been correctly updated to verify the new decodeMaskOnDisk function calls with proper parameters.
Also applies to: 130-134
213-213: LGTM: Error handling test updated correctly.The error handling test has been properly updated to verify that decodeMaskOnDisk is not called on fetch failure.
Merge
release/v1.3.0todevelopSummary by CodeRabbit
Release Notes
Operator Execution
Device Management
Dataset Handling
Image Processing
Testing