fix mime type error #5419
fix mime type error #5419
Conversation
WalkthroughThis pull request introduces modifications across multiple files, focusing on overlay processing, image decoding, and dependency management. The changes include refactoring the Changes
Sequence DiagramsequenceDiagram
participant Processor as Overlay Processor
participant Filter as Overlay Filter
participant Overlays as Overlay Collection
Processor->>Filter: Check overlay validity
Filter-->>Processor: Return filter result
Processor->>Overlays: Process valid overlays
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
🧹 Nitpick comments (10)
app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx (1)
39-44: Document the schema resolution strategy.Consider adding a code comment explaining the schema resolution chain (
schema → subSchema → computedSchema) to help future maintainers understand the fallback mechanism.+ // Schema resolution chain: + // 1. Explicitly passed schema + // 2. Computed subSchema for non-composite views + // 3. Parent computedSchema as final fallback onChange( path, value, schema ?? subSchema ?? computedSchema, computedAncestors );fiftyone/utils/open_clip.py (1)
147-154: Enhanced context management for automatic mixed precision (AMP).The new implementation using
ExitStackprovides better control over AMP context based on device type. This is particularly important for maintaining performance while ensuring compatibility across different devices.fiftyone/core/collections.py (1)
Line range hint
926-985: Consider adding default implementationsWhile making these methods abstract enforces proper inheritance, consider providing default implementations in the base class that subclasses can optionally override. This would reduce code duplication if multiple subclasses share similar implementations.
- def first(self): - raise NotImplementedError("Subclass must implement first()") + def first(self): + """Returns the first sample in the collection.""" + try: + return next(iter(self)) + except StopIteration: + return None - def last(self): - raise NotImplementedError("Subclass must implement last()") + def last(self): + """Returns the last sample in the collection.""" + try: + return next(reversed(self)) + except StopIteration: + return Nonefiftyone/core/view.py (2)
442-451: Consider improving error handling in thefirst()method.The error handling could be improved by preserving the original exception context.
Apply this diff to improve error handling:
try: return next(iter(self)) except StopIteration: - raise ValueError("%s is empty" % self.__class__.__name__) + raise ValueError("%s is empty" % self.__class__.__name__) from None🧰 Tools
🪛 Ruff (0.8.2)
451-451: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
489-553: Consider improving error handling in theone()method.The error handling could be improved by preserving the original exception context.
Apply this diff to improve error handling:
try: d = next(matches) except StopIteration: - raise ValueError("No samples match the given expression") + raise ValueError("No samples match the given expression") from None if exact: try: next(matches) raise ValueError( "Expected one matching sample, but found multiple" - ) + ) from None except StopIteration: pass🧰 Tools
🪛 Ruff (0.8.2)
542-542: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
fiftyone/core/dataset.py (2)
1227-1234: Improve error handling by usingraise ... from errpattern.While the implementation is correct, the error handling could be improved by preserving the exception chain:
- raise ValueError("%s is empty" % self.__class__.__name__) + raise ValueError("%s is empty" % self.__class__.__name__) from None🧰 Tools
🪛 Ruff (0.8.2)
1232-1232: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
1242-1249: Improve error handling by usingraise ... from errpattern.Similar to the
first()method, improve the error handling by preserving the exception chain:- raise ValueError("%s is empty" % self.__class__.__name__) + raise ValueError("%s is empty" % self.__class__.__name__) from None🧰 Tools
🪛 Ruff (0.8.2)
1247-1247: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
app/packages/operators/src/components/OperatorExecutionButton/index.tsx (1)
Line range hint
39-57: Consider adding@returnsto JSDoc.While the documentation is thorough for parameters, adding a return value description would make it complete.
app/packages/operators/src/usePanelEvent.ts (1)
23-23: Consider adding type annotation for destructured values.Adding explicit type annotations would improve type safety and make the code more maintainable.
- const { params, operator, prompt, currentPanelState } = options; + const { params, operator, prompt, currentPanelState }: HandlerOptions = options;app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (1)
Line range hint
119-139: Consider extracting style variants to a separate constant.The variant-specific styles could be extracted to a constant object to improve maintainability and reduce the complexity of the getButtonProps function.
const VARIANT_STYLES = { round: { borderRadius: '1rem', p: '3.5px 10.5px', }, square: { borderRadius: '3px 3px 0 0', backgroundColor: (theme) => theme.palette.background.field, borderBottom: '1px solid', paddingBottom: '5px', borderColor: (theme) => theme.palette.primary.main, }, // ... other variants };
📜 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 (28)
app/packages/core/src/components/Modal/ImaVidLooker.tsx(1 hunks)app/packages/core/src/components/Modal/ModalLooker.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/TooltipProvider.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/index.tsx(1 hunks)app/packages/looker/src/processOverlays.ts(1 hunks)app/packages/looker/src/worker/canvas-decoder.ts(2 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)app/packages/state/src/hooks/useCreateLooker.ts(2 hunks)app/packages/state/src/recoil/dynamicGroups.ts(2 hunks)fiftyone/__public__.py(1 hunks)fiftyone/core/collections.py(5 hunks)fiftyone/core/dataset.py(6 hunks)fiftyone/core/view.py(1 hunks)fiftyone/operators/executor.py(0 hunks)fiftyone/utils/clip/zoo.py(1 hunks)fiftyone/utils/eval/base.py(2 hunks)fiftyone/utils/open_clip.py(6 hunks)fiftyone/utils/super_gradients.py(1 hunks)fiftyone/utils/transformers.py(14 hunks)fiftyone/utils/ultralytics.py(3 hunks)tests/unittests/dataset_tests.py(1 hunks)
🔥 Files not summarized due to errors (1)
- fiftyone/utils/clip/zoo.py: Error: Disallowed special token found: <|endoftext|>
💤 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 (14)
app/packages/core/src/components/Modal/ImaVidLooker.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/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/core/src/plugins/SchemaIO/components/DynamicIO.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/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/state/src/recoil/dynamicGroups.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/core/src/components/Modal/ModalLooker.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.
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/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.
app/packages/state/src/hooks/useCreateLooker.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/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.
🪛 Ruff (0.8.2)
fiftyone/__public__.py
166-166: .core.sample.Sample imported but unused
Remove unused import
(F401)
166-166: .core.sample.SampleView imported but unused
Remove unused import
(F401)
fiftyone/core/view.py
451-451: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
542-542: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
fiftyone/core/dataset.py
1232-1232: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
1247-1247: 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: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (42)
app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx (1)
39-44: Verify the impact of the additional schema fallback.The addition of
computedSchemaas a final fallback in the schema resolution chain makes the code more resilient, but we should verify that this doesn't mask any underlying schema resolution issues.Run this script to analyze the usage patterns and potential impact:
✅ Verification successful
Schema resolution change is safe and consistent with codebase patterns
The addition of
computedSchemaas a final fallback aligns with existing patterns in the codebase, wherecomputedSchemais already used as a reliable schema source in various components. The change improves schema resolution safety without introducing any inconsistencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze schema resolution patterns in the codebase # Find all direct calls to onChangeWithSchema ast-grep --pattern 'onChangeWithSchema($$$)' # Find potential undefined schema scenarios rg -A 3 'onChange\(' --type tsLength of output: 13849
fiftyone/utils/super_gradients.py (1)
99-99: LGTM! Enhanced device management flexibility.The change from hardcoded
cuda()tomodel.to(self.device)improves flexibility by allowing the model to run on any device (CPU/GPU) based on configuration.fiftyone/utils/open_clip.py (2)
61-61: LGTM! Simple attribute assignment.Clean and straightforward assignment of the preprocess function.
111-111: LGTM! Consistent device management.Text tensors are now moved to the configured device, aligning with the broader device management improvements.
Also applies to: 123-123
fiftyone/utils/ultralytics.py (3)
23-23: LGTM! Required import for device management.Added torch import to support device detection and management.
382-384: LGTM! Smart device configuration.Good implementation of device configuration with automatic CUDA detection and fallback to CPU.
397-398: LGTM! Proper model device initialization.The model is correctly moved to the configured device during initialization.
fiftyone/utils/transformers.py (3)
326-328: LGTM! Consistent device configuration pattern.Good implementation of device configuration with automatic CUDA detection and fallback to CPU, establishing a pattern used throughout the codebase.
457-458: LGTM! Consistent device initialization.Device initialization is implemented consistently across different transformer classes.
Also applies to: 503-504, 756-757
589-589: LGTM! Consistent model loading with device management.All model loading methods consistently move models to the configured device, ensuring uniform behavior across different model types.
Also applies to: 701-701, 777-779, 832-832, 886-886, 938-938
fiftyone/core/collections.py (5)
926-927: Replaced concrete implementation with abstract methodThe
first()method has been converted to an abstract method that must be implemented by subclasses. This change enforces proper inheritance and ensures that each collection type provides its own implementation.
935-936: Replaced concrete implementation with abstract methodThe
last()method has been converted to an abstract method that must be implemented by subclasses. This change enforces proper inheritance and ensures that each collection type provides its own implementation.
947-948: Updated return type documentation for collection methodsThe docstrings have been updated to clarify that these methods can return either
SampleorSampleViewobjects. This provides more accurate type information for users.Also applies to: 962-963, 982-983
950-950: Replaced concrete implementations with abstract methodsThe
head(),tail(), andone()methods have been converted to abstract methods that must be implemented by subclasses. This change enforces proper inheritance and ensures that each collection type provides its own implementation.Also applies to: 965-965, 985-985
Line range hint
926-985: Verify impact of abstract method changesThe conversion of these methods to abstract ones requires all subclasses to implement them. Let's verify that all subclasses properly implement these methods.
✅ Verification successful
All subclasses properly implement the abstract methods
Both direct subclasses of SampleCollection (Dataset and DatasetView) implement all the required abstract methods:
first(),last(),head(),tail(), andone().🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for classes that inherit from SampleCollection ast-grep --pattern 'class $NAME($BASE) { $$$ }' | grep -B 1 "SampleCollection" # Search for implementations of the required methods for method in "first" "last" "head" "tail" "one"; do echo "Checking implementations of $method():" ast-grep --pattern 'def '$method'($$$) { $$$ }' doneLength of output: 995
Script:
#!/bin/bash # First locate the SampleCollection class and its subclasses echo "=== Finding SampleCollection and its subclasses ===" rg "class.*\(.*SampleCollection.*\):" -g "*.py" -B 1 -A 1 echo -e "\n=== Finding method implementations ===" for method in "first" "last" "head" "tail" "one"; do echo -e "\nSearching for $method() implementations:" rg "def $method\(.*\):" -g "*.py" -A 1 doneLength of output: 4349
app/packages/core/src/components/Modal/ModalLooker.tsx (1)
68-71: LGTM! Good use of key prop for proper re-rendering.Using
modalMediaFieldas a key prop ensures thatImaVidLookerReactre-renders when the selected media field changes, which helps handle media type changes correctly.app/packages/core/src/plugins/SchemaIO/index.tsx (1)
23-29: LGTM! Improved value consistency.Good refactoring to use
coerceValueconsistently throughout the component. This ensures that values are properly handled before state updates and callbacks.app/packages/looker/src/processOverlays.ts (1)
38-38: LGTM! Simplified overlay validation.Good simplification of the overlay checks to focus on the presence of mask/map data rather than paths. This change aligns with handling mime type issues by validating the actual data rather than paths.
Also applies to: 42-42
app/packages/looker/src/worker/canvas-decoder.ts (2)
18-22: Good function rename for better clarity.Renaming to
getMaybePngcolorTypebetter reflects that the function may not always return a valid PNG color type.
75-76: LGTM! Improved mime type handling.The change from checking for PNG type to excluding JPEG types is more resilient, allowing proper handling of images that might have incorrect mime types.
This change directly addresses the PR objective of fixing mime type errors.
fiftyone/__public__.py (1)
166-166: LGTM! Adding SampleView to the public interface.The addition of
SampleViewto the public interface is appropriate as it makes the class accessible to users of the library.🧰 Tools
🪛 Ruff (0.8.2)
166-166:
.core.sample.Sampleimported but unusedRemove unused import
(F401)
166-166:
.core.sample.SampleViewimported but unusedRemove unused import
(F401)
app/packages/state/src/recoil/dynamicGroups.ts (1)
292-294: LGTM! Enhanced store key uniqueness with mediaField.The addition of
mediaFieldto the store key ensures proper isolation of state per media field, preventing potential state conflicts.app/packages/state/src/hooks/useCreateLooker.ts (1)
227-230: LGTM! Improved frame controller isolation.The use of a composite key (
${thisSampleId}-${mediaField}) ensures proper isolation of frame controllers for different media fields of the same sample.Also applies to: 244-245
app/packages/core/src/components/Modal/ImaVidLooker.tsx (1)
198-198: Verify event listener cleanup.The removal of
{ once: true }allows handling multiple fetch events, but ensure that the event listener is properly cleaned up to prevent memory leaks.The cleanup is handled by the
window.removeEventListener()call when the range is loaded, but let's verify there are no edge cases where the listener might not be removed:✅ Verification successful
Event listener cleanup is properly implemented
The event listener is correctly removed when the buffer range is loaded. The removal of
{ once: true }is intentional and safe as the cleanup is handled manually when the condition is met. Consider adding a cleanup in useEffect's return function to handle component unmount edge cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential memory leaks in event listener cleanup rg -A 5 "addEventListener.*fetchMore" | rg -B 5 "removeEventListener.*fetchMore"Length of output: 82
Script:
#!/bin/bash # Search for fetchMoreListener usage and cleanup patterns echo "=== Searching for fetchMoreListener usage ===" rg "fetchMoreListener" -A 5 -B 5 echo -e "\n=== Searching for useEffect cleanup patterns ===" ast-grep --pattern 'useEffect(() => { $$$ return () => { $$$ } $$$ })'Length of output: 2987
fiftyone/utils/eval/base.py (2)
Line range hint
185-215: LGTM! The newinclude_missingparameter is well documented.The parameter addition and docstring updates are clear and consistent with the existing style.
213-215: LGTM! The method call is updated correctly.The new parameter is properly passed through to the internal method.
fiftyone/core/view.py (3)
453-459: LGTM! Thelast()method implementation is efficient.The method cleverly uses slicing and the first() method to get the last element.
461-473: LGTM! Thehead()method is well implemented.The method efficiently handles cases where fewer samples are available than requested.
475-487: LGTM! Thetail()method is well implemented.The method efficiently handles cases where fewer samples are available than requested.
tests/unittests/dataset_tests.py (2)
971-1018: Well-structured test for first/last/head/tail functionality on populated dataset.The test thoroughly verifies:
- first() returns correct first sample
- last() returns correct last sample
- head() returns first 3 samples by default
- tail() returns last 3 samples by default
- Behavior is consistent for both Sample and SampleView objects
1019-1048: Comprehensive test coverage for empty dataset edge cases.The test properly verifies:
- first() raises ValueError on empty dataset
- last() raises ValueError on empty dataset
- head() returns empty list on empty dataset
- tail() returns empty list on empty dataset
- Behavior is consistent for both dataset and views
fiftyone/core/dataset.py (4)
1213-1220: LGTM! Efficient implementation using MongoDB aggregation.The implementation correctly uses the
_idfield for sorting which leverages MongoDB's default index, and detaches frames/groups for optimal performance.
1263-1264: LGTM! Clean and efficient implementation.The implementation correctly uses
_get_first()with the specified limit and handles empty datasets gracefully by returning an empty list.
1278-1281: LGTM! Clean and efficient implementation.The implementation correctly uses
_get_first()with reverse sorting and properly reverses the results to maintain the expected order.
Line range hint
1312-1341: LGTM! Clear error handling and efficient implementation.The implementation efficiently detects multiple matches when
exact=Trueby usinglimit=2, and provides clear error messages for all failure cases.app/packages/operators/src/components/OperatorExecutionButton/index.tsx (1)
Line range hint
19-38: LGTM! Well-documented component interface.The JSDoc comments clearly describe the component's purpose and all its props, making it easy for other developers to understand and use.
app/packages/operators/src/usePanelEvent.ts (1)
52-52: LGTM! Proper error handling with context.The error handling includes both user notification and console logging, with proper context about the operation that failed.
app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (2)
66-71: LGTM! Proper memoization of operator handlers.The use of useMemo for operatorHandlers prevents unnecessary re-renders.
73-84: Consider deep merging executorOptions.The current shallow merge of executorOptions might not handle nested objects correctly. Consider using a deep merge utility.
app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (1)
100-101: LGTM! Simplified icon positioning logic.The icon positioning logic is now more straightforward and easier to understand.
app/packages/operators/src/state.ts (1)
659-659: LGTM! Simplified cancellation handling.The change to make
cancela direct alias ofclosealigns with the broader refactor to remove cancellation callbacks across the codebase. Theclosefunction already handles proper cleanup by resetting state.Let's verify that all usages of the
cancelfunction have been updated:✅ Verification successful
Verified: Cancel-to-Close alias change is safe ✅
The codebase handles both cancel and close actions consistently. The only difference is UI-based (button text), while the underlying cleanup behavior remains the same.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to onCancel or cancel callbacks rg -i "oncancel|cancelcallback|on_cancel" --type tsLength of output: 1498
Script:
#!/bin/bash # Get the full context of cancel/close handling in utils.ts rg -A 5 -B 5 "onCancel = operatorPrompt" app/packages/operators/src/utils.tsLength of output: 433
app/packages/operators/src/built-in-operators.ts (1)
1048-1048: LGTM! Consistent with cancellation handling removal.The removal of
on_cancelparameter fromPromptUserForOperation.execute()is consistent with the broader refactor to simplify cancellation handling. The core functionality for success and error cases is preserved.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/packages/looker/src/processOverlays.ts (2)
41-44: Consider using spread operator for better readability.While using
pushis valid and potentially more performant, the spread operator might be more idiomatic in modern TypeScript/JavaScript. However, if performance is a concern, the current implementation is fine.Alternative implementation:
let ordered = activePaths.reduce((acc, cur) => [...acc, ...bins[cur]], [] as Overlay<State>[]);
93-96: Address the TODO comment with a more robust solution.The comment indicates a limitation with detection overlays where bounding boxes might be needed even when masks aren't loaded. Consider implementing a more flexible approach:
- Split the mask/map validation into a separate method in each overlay class
- Allow overlays to specify which properties are required vs optional
Would you like me to propose a detailed solution for this architectural improvement?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/looker/src/processOverlays.test.ts(1 hunks)app/packages/looker/src/processOverlays.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
🔇 Additional comments (2)
app/packages/looker/src/processOverlays.ts (2)
36-36: LGTM! Good separation of concerns.Extracting the filter logic into a separate function improves code organization and maintainability.
85-105: Verify the impact on detection overlays.The filter function's behavior might affect detection overlays where bounding boxes should be shown even when masks aren't loaded. Let's verify this assumption:
| @@ -70,8 +72,9 @@ export const recastBufferToMonoChannel = ( | |||
| export const decodeWithCanvas = async (blob: Blob, cls: string) => { | |||
| let channels: number = 4; | |||
|
|
|||
| if (blob.type === "image/png") { | |||
| const colorType = await getPngcolorType(blob); | |||
| if (blob.type !== "image/jpg" && blob.type !== "image/jpeg") { | |||
There was a problem hiding this comment.
This implies we still rely on MIME type, specifically for jpeg?
There was a problem hiding this comment.
Not really... more like a heuristic. We don't want to unnecessarily check if it's PNG if the mime type says it's a jpeg.
Note that all of this is used just to determine number of channels.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
fiftyone/core/odm/mixins.py (1)
1872-1878: LGTM! Consider optimizing for large path lists.The implementation correctly filters out nested paths. For better performance with large lists, consider sorting paths first to reduce the number of string comparisons.
Here's an optimized version that would perform better for large lists:
def _remove_nested_paths(paths): - return [ - path - for path in paths - if not any(path.startswith(p + ".") for p in paths) - ] + sorted_paths = sorted(paths) # Sort for efficient prefix checking + result = [] + for i, path in enumerate(sorted_paths): + if i + 1 < len(sorted_paths): + next_path = sorted_paths[i + 1] + if not next_path.startswith(path + "."): + result.append(path) + else: + result.append(path) + return resultfiftyone/utils/iou.py (2)
618-620: Consider removing the redundant_get_mask_boxfunctionThe
_get_mask_boxfunction is a simple wrapper that directly calls_get_detection_box. Unless there's a future need for differentiation, it may be unnecessary.-def _get_mask_box(x): - return _get_detection_box(x)Alternatively, if semantic clarity is desired, consider keeping the function but adding a comment to explain its purpose.
698-701: Simplify the initialization ofioususing a ternary operatorThe
if-elseblock can be replaced with a ternary operator for conciseness without sacrificing readability.Apply this diff to simplify the code:
- if sparse: - ious = defaultdict(list) - else: - ious = np.zeros((len(preds), len(gts))) + ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))🧰 Tools
🪛 Ruff (0.8.2)
698-701: Use ternary operator
ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
docs/source/user_guide/evaluation.rst (1)
748-748: Enhance clarity regarding mask usageSpecify that the
maskattribute must be populated for instance segmentation tasks when usinguse_masks=True.Consider rephrasing for clarity:
- mask populated to define the extent of the object within its bounding box. + `mask` attribute populated to define the extent of the object within its bounding box.plugins/operators/__init__.py (2)
647-649: Simplify dictionary iteration by removing unnecessary.keys()You can iterate directly over dictionaries without using the
.keys()method. Replacefor key in schema.keys():withfor key in schema:to make the code more concise and Pythonic. This pattern appears in multiple places in the code.Apply this diff to all instances:
-for key in schema.keys(): +for key in schema:🧰 Tools
🪛 Ruff (0.8.2)
647-647: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
247-261: Refactor input validation logic to reduce code duplicationThe input validation logic for field names is repeated across multiple functions (
_clone_sample_field_inputs,_clone_frame_field_inputs,_rename_sample_field_inputs,_rename_frame_field_inputs). Consider refactoring this logic into a shared helper function to improve maintainability and reduce redundancy.Also applies to: 365-379, 442-456, 534-548
📜 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 (16)
app/packages/app/src/components/Setup.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(2 hunks)docs/source/user_guide/evaluation.rst(2 hunks)fiftyone/constants.py(1 hunks)fiftyone/core/dataset.py(1 hunks)fiftyone/core/odm/mixins.py(4 hunks)fiftyone/utils/eval/coco.py(1 hunks)fiftyone/utils/eval/openimages.py(1 hunks)fiftyone/utils/iou.py(9 hunks)package/graphql/setup.py(1 hunks)plugins/operators/__init__.py(20 hunks)plugins/panels/model_evaluation/__init__.py(2 hunks)requirements/common.txt(1 hunks)requirements/extras.txt(1 hunks)requirements/test.txt(2 hunks)setup.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- package/graphql/setup.py
- app/packages/app/src/components/Setup.tsx
- setup.py
- fiftyone/utils/eval/coco.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.
🪛 Ruff (0.8.2)
fiftyone/utils/iou.py
698-701: Use ternary operator ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts))) instead of if-else-block
Replace if-else-block with ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))
(SIM108)
709-709: Local variable bbox_iou_fcn is assigned to but never used
Remove assignment to unused variable bbox_iou_fcn
(F841)
plugins/operators/__init__.py
647-647: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
755-755: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
907-907: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
982-982: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (14)
fiftyone/core/odm/mixins.py (1)
673-674: LGTM! Good addition of nested path filtering.The consistent usage of
_remove_nested_pathsat the start of field operations prevents unintended side effects when dealing with nested paths. This improves the robustness of the field manipulation methods.Also applies to: 721-722, 826-827
fiftyone/utils/iou.py (3)
77-78: Clarify the default behavior in documentationThe added clarification in the docstring effectively informs users that IoUs are computed directly on dense pixel masks when
toleranceisNone. This enhances the readability and usability of the documentation.
528-585: Ensure correct handling of image dimensions and resizing in_dense_iouThe
_dense_ioufunction appears to correctly compute the IoU between dense masks. However, there are a few points to consider:
- Image Dimension Consistency: Verify that the computed
gt_img_w,gt_img_h,pred_img_w, andpred_img_hcorrectly represent the image dimensions based on mask sizes and bounding box scales.- Resizing Logic: The resizing logic adjusts the smaller mask to match the larger mask's dimensions. Ensure that this resizing maintains the integrity of the masks and does not introduce artifacts.
- Data Type Consistency: Confirm that the mask arrays are of a binary type (
booloruint8) before performing logical operations to prevent unintended behavior.Would you like assistance in implementing unit tests to validate the functionality of this method?
663-663: Review the necessity of the pylint disable commandThe comment
# pylint: disable=no-value-for-parameteris used to suppress a pylint warning. Ensure that this suppression is necessary and that the function call is correctly implemented.If the warning arises due to the use of
rtree_index.intersection(box), confirm that the correct parameters are being passed.fiftyone/utils/eval/openimages.py (1)
47-48: Clarify the default behavior in the documentationThe additional sentence in the docstring enhances clarity by specifying that IoUs are computed directly on dense pixel masks by default when
toleranceis not provided.plugins/operators/__init__.py (3)
755-757: Simplify dictionary iteration by removing unnecessary.keys()🧰 Tools
🪛 Ruff (0.8.2)
755-755: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
907-909: Simplify dictionary iteration by removing unnecessary.keys()🧰 Tools
🪛 Ruff (0.8.2)
907-907: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
982-984: Simplify dictionary iteration by removing unnecessary.keys()🧰 Tools
🪛 Ruff (0.8.2)
982-982: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
plugins/panels/model_evaluation/__init__.py (1)
328-331: Implementation ofget_correct_incorrectis correctThe
get_correct_incorrectmethod accurately calculates the number of correct and incorrect predictions by comparingresults.ypredandresults.ytrue.fiftyone/core/dataset.py (1)
1958-1959: Consider optimizing list field unwinding.The sequential unwinding of list fields could impact performance for deeply nested data structures. Consider:
- Batching the unwind operations
- Adding an index on frequently accessed list fields
Run this script to analyze the depth and frequency of list field access:
fiftyone/constants.py (1)
45-45: Verify the necessity of version compatibility change.The extension of compatible versions from "<1.4" to "<1.5" seems broader than what's needed for fixing mime type errors. This change could have significant implications for version compatibility.
Please confirm:
- Is this version bump necessary for the mime type fix?
- Have all breaking changes between 1.4 and 1.5 been thoroughly tested?
requirements/extras.txt (1)
4-4: Consider maintaining an upper version bound for ipywidgets.Removing the upper version bound (
<8) for ipywidgets could expose the project to compatibility issues with future major versions. This change also appears unrelated to the PR's objective of fixing mime type errors.Consider:
- Maintaining an upper bound to prevent compatibility issues
- Testing with the latest ipywidgets versions before removing the constraint
requirements/test.txt (1)
1-1: Verify AWS CLI version compatibility with boto3.The added AWS CLI version 1.37.2 should be verified for compatibility with boto3 1.36.2 (specified in common.txt).
✅ Verification successful
AWS CLI version 1.37.2 is compatible with boto3 1.36.2
The specified version is recent, stable, and maintains compatibility within the 1.x series.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest AWS CLI versions and compatibility pip index versions awscli | head -n 5 curl -s https://raw.githubusercontent.com/boto/boto3/develop/setup.py | grep -A 5 "install_requires"Length of output: 19613
requirements/common.txt (1)
3-3: Verify boto3 version upgrade impact.The significant version jump from 1.17.36 to 1.36.2 requires careful verification:
- Check for breaking changes
- Ensure compatibility with awscli 1.37.2
- Verify any security advisories
✅ Verification successful
✓ boto3 upgrade to 1.36.2 appears safe
The version upgrade is safe based on:
- Standard S3 client usage in codebase
- No security vulnerabilities reported
- Version is recent (just one minor behind latest 1.36.3)
- Basic AWS credential configuration patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check boto3 release notes and security advisories curl -s https://pypi.org/pypi/boto3/json | jq -r '.releases | keys[]' | sort -V | tail -n 5 gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "boto3") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 468
Script:
#!/bin/bash # Check for AWS-related dependencies rg -l "boto|aws" requirements/ # Check boto3 usage patterns rg "boto3\." -A 2 -B 2Length of output: 1853
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx
Outdated
Show resolved
Hide resolved
benjaminpkane
force-pushed
the
fix/mime-type-error
branch
from
8a738ab to
671db6d
Compare
sashankaryal
force-pushed
the
fix/mime-type-error
branch
from
671db6d to
6cd09e6
Compare
What changes are proposed in this pull request?
Sometimes mime type is missing and we get just
binary/octet-streamfor masks. The app should be resilient against that.Note: unit tests for masks decoding are added in #5413
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Fix masks loading bug
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Dependency Updates
Chores