Merge release/v1.1.0 to develop
#5205
Conversation
WalkthroughThe pull request introduces significant updates across various documentation files and core components of the FiftyOne platform. Key changes include enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 and nitpick comments (1)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
197-197: Consider extracting duplicate styles into a shared constant.The fontSize "1rem" is now duplicated across multiple TableCell components. To improve maintainability and ensure consistency, consider extracting this common style into a shared constant.
Here's a suggested refactor:
+ const tableCellBaseStyles = { + fontSize: "1rem", + }; const headingCellBaseStyles = { + ...tableCellBaseStyles, fontWeight: 360, - fontSize: "1rem", color: theme.palette.text.secondary, }; // ... later in the code ... <TableCell key={key} sx={{ background: isSelected ? selectedCellColor : "unset", - fontSize: "1rem", + ...tableCellBaseStyles, }} onClick={() => { handleCellClick(rowIndex, columnIndex); }} {...getComponentProps(props, "tableBodyCell")} > // ... and in the actions cell ... <TableCell align="right" sx={{ background: isRowSelected ? selectedCellColor : "unset", - fontSize: "1rem", + ...tableCellBaseStyles, }} >Also applies to: 224-224
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/TableView.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.
* clear stale lookers * simplify spotlight render state, filter duplicated ids for linked reversals * lint render * rm noop
* remove ne matching and extra sorting * use exists * add to embedded list bounds tests * skip nonfinites sort for lists * handle NaN formatting * debugging * nonfinites tweaks * selector fixes * always use distinct for embedded lists * fix selector
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
app/packages/core/src/components/Grid/Grid.tsx(2 hunks)app/packages/core/src/components/Grid/useRefreshers.ts(0 hunks)app/packages/core/src/components/Grid/useSelect.ts(1 hunks)app/packages/spotlight/src/row.ts(2 hunks)app/packages/spotlight/src/section.ts(4 hunks)app/packages/spotlight/src/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/packages/core/src/components/Grid/useRefreshers.ts
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/spotlight/src/types.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/components/Grid/useSelect.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/components/Grid/Grid.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/spotlight/src/section.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/spotlight/src/row.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (5)
app/packages/spotlight/src/section.ts (1)
303-305: Approved
The filtering logic correctly prevents duplicate items by checking against #itemIds before adding new items.
app/packages/core/src/components/Grid/useSelect.ts (1)
19-27: Approved
The changes effectively manage the store by retaining updated instances and removing obsolete ones, ensuring efficient resource utilization and preventing memory leaks.
Also applies to: 34-37
app/packages/spotlight/src/types.ts (1)
41-41: Approved
The Render type signature has been correctly updated to include zooming: boolean, aligning with the changes in rendering logic across the components.
app/packages/core/src/components/Grid/Grid.tsx (1)
Line range hint 73-85: Approved
The render method has been updated to use the zooming parameter, optimizing rendering performance during fast scrolling by appropriately skipping looker creation.
app/packages/spotlight/src/row.ts (1)
151-154: Approved
The show method has been correctly updated to utilize the zooming parameter, enhancing the control over rendering behavior and improving performance during zoom operations.
Also applies to: 166-166
| #end: Edge<K, V>; | ||
| #itemIds = new Set<string>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure #itemIds is cleared upon destruction to prevent memory leaks
The #itemIds set is not cleared in the destroy method. Over time, this could lead to memory leaks if old item IDs are retained. Please consider clearing the #itemIds set when the section is destroyed.
Apply this diff to clear the #itemIds set in the destroy method:
destroy(destroyItems = false) {
this.#section.remove();
for (const row of this.#rows) row.destroy(destroyItems);
this.#rows = [];
+ this.#itemIds.clear();
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/core/src/components/Common/utils.tsx (1)
71-78: Consider adding null/undefined check before formatting.While the NaN handling is good, the formatter might throw if
vis undefined/null. Consider adding a guard clause.const str = numeral(v).format( [INT_FIELD, FRAME_NUMBER_FIELD, FRAME_SUPPORT_FIELD].includes(fieldType) ? "0a" : bounds[1] - bounds[0] < 0.1 ? "0.0000a" : "0.00a" ); - return str === "NaN" ? v.toString() : str; + return str === "NaN" ? (v?.toString() ?? "") : str;fiftyone/server/lightning.py (1)
326-328: Consider the performance impact of additional sort operationsWhile adding sorting improves result consistency, inserting both match and sort operations for each filter key-value pair could impact query performance with large datasets.
Consider batching the sort operations or evaluating if sorting is necessary for all filter conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
app/packages/core/src/components/Common/utils.tsx(1 hunks)app/packages/core/src/components/Filters/NumericFieldFilter/RangeSlider.tsx(1 hunks)app/packages/core/src/components/Filters/NumericFieldFilter/state.ts(2 hunks)fiftyone/server/lightning.py(5 hunks)fiftyone/server/view.py(2 hunks)tests/unittests/lightning_tests.py(27 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/core/src/components/Common/utils.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/Filters/NumericFieldFilter/RangeSlider.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/Filters/NumericFieldFilter/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.
🔇 Additional comments (9)
app/packages/core/src/components/Filters/NumericFieldFilter/state.ts (1)
28-38: LGTM! The selector implementation is clear and follows best practices.
The new nonfinitesText selector:
- Correctly handles the filtering of nonfinite values
- Provides appropriate null fallback
- Uses type-safe parameter passing
app/packages/core/src/components/Filters/NumericFieldFilter/RangeSlider.tsx (1)
38-38: LGTM! Improved user feedback for nonfinite values.
The implementation:
- Correctly uses the Recoil selector
- Provides clear feedback when nonfinite values are present
- Maintains fallback for "No results" case
Also applies to: 41-43
fiftyone/server/lightning.py (3)
143-147: LGTM: Clear and explicit filter initialization
The code properly handles the group field and slice conditions, with an explicit empty dictionary initialization as a fallback.
429-430: LGTM: Improved pipeline construction logic
The changes enhance the handling of non-finite values and ensure proper path existence validation.
Also applies to: 432-432
509-515: LGTM: Robust result parsing
The code now handles non-finite float values more gracefully and has a clearer value extraction logic.
fiftyone/server/view.py (1)
617-619: LGTM: Consistent range validation across field types
The code now consistently checks for the presence of range values before processing them, improving robustness across different field types (support fields, datetime fields, and numeric fields).
Also applies to: 644-647, 649-651
tests/unittests/lightning_tests.py (3)
538-538: LGTM: Comprehensive float value handling tests
The test expectations have been updated to properly verify the handling of non-finite values (inf, nan, ninf) and their impact on min/max calculations.
Also applies to: 572-572, 591-591, 609-609, 635-635, 644-644, 654-654, 663-663, 672-672, 681-681, 734-734, 753-753, 771-771, 797-797, 806-806, 816-816, 825-825, 834-834, 843-843, 932-932, 951-951, 969-969
1063-1067: LGTM: Enhanced group dataset test coverage
The test now includes multiple classifications with varying confidence values, providing better coverage of real-world scenarios.
Also applies to: 1076-1080
1092-1096: LGTM: Added float result validation
The test suite now properly validates float results, including min/max values and confidence scores across different group slices.
Also applies to: 1114-1120, 1128-1132, 1146-1152, 1160-1164
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
fiftyone/server/lightning.py (2)
336-338: Consider optimizing pipeline operations orderWhile the implementation is correct, consider combining the match and sort operations into a single pipeline stage to potentially improve performance:
- query.insert(0, {"$match": {k: v}}) - query.insert(0, {"$sort": {k: 1}}) + query.insert(0, { + "$match": {k: v}, + "$sort": {k: 1} + })
519-525: Consider adding explicit error handling for malformed dataWhile the current implementation handles non-finite values well, consider adding explicit error handling for malformed data:
if data and data[0]: value = data[0] - if "value" in value: - value = value["value"] - return ( - value - if not isinstance(value, float) or math.isfinite(value) - else None - ) + try: + if "value" in value: + value = value["value"] + return ( + value + if not isinstance(value, float) or math.isfinite(value) + else None + ) + except (TypeError, AttributeError) as e: + # Log error for debugging + return Noneapp/packages/looker/src/worker/disk-overlay-decoder.ts (1)
58-61: Simplify conditional checks using optional chainingYou can make the code more concise and improve readability by using optional chaining in the conditional statement.
Apply this diff to simplify the condition:
-if ( - label[overlayField] && - label[overlayField].bitmap && - !label[overlayField].image -) { +if (label[overlayField]?.bitmap && !label[overlayField].image) {🧰 Tools
🪛 Biome (1.9.4)
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/packages/looker/src/worker/index.ts (1)
Line range hint
390-408: Handle rejected promises when processing image bitmapsCurrently,
Promise.all(imageBitmapPromises)may cause unhandled promise rejections if any promise is rejected. Consider usingPromise.allSettledto handle both fulfilled and rejected promises, ensuring robustness.Apply this diff to handle rejected promises:
-Promise.all(imageBitmapPromises).then((bitmaps) => { +Promise.allSettled(imageBitmapPromises).then((results) => { + const bitmaps = results + .filter((result) => result.status === 'fulfilled') + .map((result) => result.value); const flatBitmaps = bitmaps.flat() ?? []; const flatMaskTargetsBuffers = maskTargetsBuffers.flat() ?? []; const transferables = [...flatBitmaps, ...flatMaskTargetsBuffers]; postMessage( { method: "processSample", sample, coloring, uuid, customizeColorSetting, colorscale, labelTagColors, selectedLabelTags, }, // @ts-ignore transferables ); +}).catch((error) => { + console.error('Error processing image bitmaps:', error); + // Handle the error appropriately +});app/packages/looker/src/worker/pooled-fetch.ts (1)
13-14: Consider making MAX_CONCURRENT_REQUESTS configurableThe hardcoded limit of 100 concurrent requests might not be optimal for all environments.
Consider making this configurable through environment variables or constructor parameters if moving to a class-based implementation.
app/packages/looker/src/worker/decorated-fetch.ts (1)
45-48: Consider exponential backoff instead of linearLinear backoff might not be optimal for handling temporary service outages or rate limiting.
Consider implementing exponential backoff with jitter for better retry distribution:
const delay = Math.min( retry.delay * Math.pow(2, i) * (0.5 + Math.random() * 0.5), maxDelay );app/packages/looker/src/worker/decorated-fetch.test.ts (1)
97-101: Add test for concurrent requestsThe tests don't verify the behavior of multiple concurrent requests.
Consider adding a test case that verifies multiple requests are handled correctly:
it("should handle multiple concurrent requests", async () => { const requests = Array(5).fill(null).map(() => fetchWithLinearBackoff("http://fiftyone.ai") ); await Promise.all(requests); expect(global.fetch).toHaveBeenCalledTimes(5); });app/packages/looker/src/overlays/base.ts (1)
76-76: Consider documenting cleanup behaviorThe optional
cleanupmethod in theOverlayinterface would benefit from JSDoc documentation explaining its purpose and when it should be implemented.+ /** Cleanup resources associated with this overlay when it's being disposed */ cleanup?(): void;app/packages/looker/src/worker/pooled-fetch.test.ts (1)
59-83: Consider adding edge case testsWhile the concurrency test is well-implemented, consider adding tests for:
- Cancellation scenarios
- Race conditions when MAX_CONCURRENT_REQUESTS is reached
app/packages/looker/src/lookers/frame-reader.ts (1)
55-63: LGTM: Proper resource cleanup implementationThe dispose function correctly handles overlay cleanup:
- Safely accesses overlays array
- Uses optional chaining for cleanup calls
- Maintains original frame removal functionality
However, consider adding error handling to prevent a single overlay cleanup failure from blocking other cleanups:
dispose: (frame, key) => { const overlays = frame.overlays; for (let i = 0; i < overlays.length; i++) { - overlays[i].cleanup?.(); + try { + overlays[i].cleanup?.(); + } catch (error) { + console.error(`Failed to cleanup overlay ${i}:`, error); + } } removeFrame(key); },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
app/packages/looker/src/lookers/abstract.ts(1 hunks)app/packages/looker/src/lookers/frame-reader.ts(1 hunks)app/packages/looker/src/overlays/base.ts(3 hunks)app/packages/looker/src/overlays/detection.ts(3 hunks)app/packages/looker/src/overlays/heatmap.ts(3 hunks)app/packages/looker/src/overlays/segmentation.ts(5 hunks)app/packages/looker/src/worker/decorated-fetch.test.ts(5 hunks)app/packages/looker/src/worker/decorated-fetch.ts(3 hunks)app/packages/looker/src/worker/deserializer.ts(0 hunks)app/packages/looker/src/worker/disk-overlay-decoder.test.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)app/packages/looker/src/worker/index.ts(10 hunks)app/packages/looker/src/worker/pooled-fetch.test.ts(1 hunks)app/packages/looker/src/worker/pooled-fetch.ts(1 hunks)app/packages/looker/src/worker/shared.ts(2 hunks)fiftyone/server/lightning.py(11 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker/src/worker/deserializer.ts
🧰 Additional context used
📓 Path-based instructions (14)
app/packages/looker/src/worker/shared.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/lookers/frame-reader.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/overlays/base.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/decorated-fetch.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/decorated-fetch.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.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/pooled-fetch.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/pooled-fetch.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/overlays/segmentation.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/overlays/heatmap.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/overlays/detection.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/lookers/abstract.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/index.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.
🪛 Biome (1.9.4)
app/packages/looker/src/worker/disk-overlay-decoder.ts
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (15)
fiftyone/server/lightning.py (3)
130-141: LGTM: Clean implementation of frame field handling
The addition of is_frames to the result tuple and its propagation through the query chain is well-structured and maintains type safety.
145-149: LGTM: Improved filter initialization logic
The explicit handling of group_field and slice conditions makes the code more maintainable and easier to understand.
439-442: LGTM: Robust handling of non-finite float values
The addition of special handling for float values and the logical pipeline construction ensures proper handling of non-finite values while maintaining data integrity.
app/packages/looker/src/overlays/heatmap.ts (1)
205-213: Implementing resource cleanup correctly
The cleanup method properly closes the bitmap to prevent memory leaks, ensuring efficient resource management.
app/packages/looker/src/overlays/segmentation.ts (1)
260-268: Effective resource management in cleanup method
The addition of the cleanup method correctly handles the closure of the bitmap resource, which helps prevent potential memory leaks.
app/packages/looker/src/worker/shared.ts (1)
14-21: getOverlayFieldFromCls function is well-implemented
The getOverlayFieldFromCls function provides a clear and extensible mapping of overlay fields based on the class type, enhancing maintainability.
app/packages/looker/src/worker/decorated-fetch.ts (1)
6-9: 🛠️ Refactor suggestion
Add validation for RetryOptions parameters
The RetryOptions interface should enforce positive numbers for retries and delay.
export interface RetryOptions {
- retries: number;
- delay: number;
+ retries: number & Record<never, never>; // Ensures number type
+ delay: number & Record<never, never>; // Ensures number type
+ validate(): boolean {
+ return this.retries > 0 && this.delay > 0;
+ }
}Likely invalid or redundant comment.
app/packages/looker/src/overlays/base.ts (2)
6-6: LGTM: Import addition is appropriate
The addition of OverlayMask import aligns with the new mask handling functionality.
43-46: LGTM: Well-structured type definition
The LabelMask type is well-defined with optional properties, providing flexibility while maintaining type safety.
app/packages/looker/src/worker/pooled-fetch.test.ts (2)
7-15: LGTM: Well-implemented helper function
The createDeferredPromise helper is well-designed, providing type safety and proper promise control.
94-109: Verify retry delay implementation
The test verifies retry count but doesn't verify if the delay between retries is respected.
app/packages/looker/src/worker/disk-overlay-decoder.test.ts (1)
1-205: Well-structured test suite with comprehensive coverage!
The test suite effectively covers key scenarios:
- Early return for existing overlays
- Successful overlay fetching and decoding
- HEATMAP class handling
- DETECTIONS class with recursive processing
- Error handling for failed fetches
Good practices observed:
- Clear test isolation
- Proper mocking of dependencies
- Comprehensive assertions
app/packages/looker/src/overlays/detection.ts (2)
146-146: Improved mask rendering using ImageBitmap
The switch to using bitmap for mask rendering is a good optimization as ImageBitmap provides better performance for repeated rendering operations.
Also applies to: 154-156
264-269: Good addition of resource cleanup
The cleanup method properly releases ImageBitmap resources, which is essential to prevent memory leaks. The implementation follows best practices by:
- Checking for bitmap existence before cleanup
- Properly calling close() on the bitmap
- Nullifying the reference after cleanup
app/packages/looker/src/lookers/abstract.ts (1)
296-300: Proper resource cleanup on destruction
Good addition of cleanup handling when the state is destroyed. This ensures that all overlay resources are properly released, preventing memory leaks.
| .catch((error) => { | ||
| activeRequests--; | ||
| reject(error); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add queue processing on error
The error case doesn't trigger processFetchQueue(), which could lead to stalled requests if errors occur frequently.
.catch((error) => {
activeRequests--;
reject(error);
+ processFetchQueue();
});📝 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.
| .catch((error) => { | |
| activeRequests--; | |
| reject(error); | |
| }); | |
| }; | |
| .catch((error) => { | |
| activeRequests--; | |
| reject(error); | |
| processFetchQueue(); | |
| }); | |
| }; |
| let activeRequests = 0; | ||
| const requestQueue: QueueItem[] = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a class to encapsulate queue state
The global state variables activeRequests and requestQueue could lead to race conditions and make testing difficult. Consider encapsulating these in a class:
-let activeRequests = 0;
-const requestQueue: QueueItem[] = [];
+class FetchQueue {
+ private activeRequests = 0;
+ private requestQueue: QueueItem[] = [];
+
+ public enqueue(request: QueueItem["request"]): Promise<Response> {
+ return new Promise((resolve, reject) => {
+ this.requestQueue.push({ request, resolve, reject });
+ this.processQueue();
+ });
+ }
+ // ... rest of the implementation
+}Committable suggestion skipped: line range outside the PR's diff.
| fetchWithLinearBackoff( | ||
| "http://fiftyone.ai", | ||
| {}, | ||
| { | ||
| retries: 3, | ||
| delay: 10, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test for invalid RetryOptions
The tests should verify that invalid retry options are handled appropriately.
Add test cases for negative retries and delays:
it("should throw for invalid retry options", async () => {
await expect(
fetchWithLinearBackoff("http://fiftyone.ai", {}, { retries: -1, delay: 10 })
).rejects.toThrow("Invalid retry options");
});* don't filter slice for distinct scans * update lightning group test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
fiftyone/server/lightning.py (2)
336-338: Consider optimizing the query orderWhile the current implementation works, consider moving the
$sortoperation after the$matchfor better performance, as filtering before sorting typically reduces the number of documents that need to be sorted.- query.insert(0, {"$match": {k: v}}) - query.insert(0, {"$sort": {k: 1}}) + query.insert(0, {"$sort": {k: 1}}) + query.insert(0, {"$match": {k: v}})
518-524: Consider adding explicit error handlingWhile the current implementation handles non-finite float values well, consider adding explicit error handling for malformed results to improve robustness.
if data and data[0]: value = data[0] - if "value" in value: - value = value["value"] - return ( - value - if not isinstance(value, float) or math.isfinite(value) - else None - ) + try: + if "value" in value: + value = value["value"] + return ( + value + if not isinstance(value, float) or math.isfinite(value) + else None + ) + except (TypeError, AttributeError) as e: + logger.warning(f"Error parsing result: {e}") + return None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/server/lightning.py(11 hunks)tests/unittests/lightning_tests.py(27 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unittests/lightning_tests.py
🔇 Additional comments (4)
fiftyone/server/lightning.py (4)
12-12: LGTM: Required import for float handling
The addition of the math module import is necessary for the new finite float value checking functionality.
130-141: LGTM: Enhanced frame field handling
The addition of is_frames to the tuple unpacking improves the handling of frame-specific queries by properly propagating frame field information through the query chain.
145-149: LGTM: Improved filter initialization
The filter initialization logic has been improved to handle edge cases where either dataset.group_field is not present or input.slice is not provided.
438-441: LGTM: Improved float handling and pipeline structure
The changes properly handle non-finite float values and ensure the existence check is performed at the appropriate stage in the pipeline.
[Docs] Features + release notes
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
docs/source/teams/query_performance.rst (3)
59-60: Fix typo in the word "sidebar"There's a typo in the word "sidbar".
-top-right of the sidbar: +top-right of the sidebar:
194-195: Add missing period in section titleFor consistency with other section titles, add a period at the end.
-Deleting a scan +Deleting a scan.
61-63: Fix typo in "performed"There's a typo in the word "performend".
- performend synchronously and will timeout if it does not complete within a + performed synchronously and will timeout if it does not complete within adocs/source/teams/data_quality.rst (1)
202-204: Enhance code reference formattingFor better readability and consistency, wrap the operator name in backticks.
- delete_sample_field operator available via the + `delete_sample_field` operator available via thedocs/source/teams/overview.rst (1)
111-111: Split references for better clarityThe visual similarity and dataset uniqueness references should be separated for better readability.
- | :ref:`Visual similarity <brain-similarity>` and :ref:`dataset uniqueness <brain-image-uniqueness>` + | :ref:`Visual similarity <brain-similarity>` + | :ref:`Dataset uniqueness <brain-image-uniqueness>`docs/source/brain.rst (1)
1268-1357: Comprehensive documentation of leaky splits implementation.The section provides thorough coverage of:
- Input/output expectations
- Configuration options
- Code examples
- Practical usage scenarios
Consider adding a note about performance implications for large datasets.
Add a note about computational complexity and memory requirements for large datasets:
+.. note:: + + For large datasets, the leaky splits computation may require significant memory + and processing time. Consider using a subset of your data for initial analysis.docs/source/user_guide/app.rst (1)
2010-2047: Clear documentation of the Histograms panel.The section effectively explains:
- Available visualization modes
- Dynamic updates based on view
- Configuration options
Consider adding examples of common use cases.
Add a subsection with practical examples:
+Common Use Cases +--------------- + +- Analyzing class distribution imbalances +- Monitoring dataset statistics during curation +- Identifying outliers in numeric fields
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (21)
docs/source/images/app/app-query-performance-disabled.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/app-query-performance.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-class.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-compare.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-confusion.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-metric.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-notes.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-open.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-review.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-summary.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/teams/data_quality_brightness_analysis.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_brightness_mark_as_reviewed.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_brightness_scan.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_brightness_scan_options.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_brightness_scheduled.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_brightness_slider.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/teams/data_quality_brightness_tag.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_home.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_new_samples_home.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_new_samples_modal.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/qp_toggle.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (13)
docs/source/brain.rst(5 hunks)docs/source/dataset_zoo/index.rst(1 hunks)docs/source/index.rst(1 hunks)docs/source/model_zoo/index.rst(1 hunks)docs/source/plugins/developing_plugins.rst(3 hunks)docs/source/release-notes.rst(3 hunks)docs/source/teams/data_quality.rst(1 hunks)docs/source/teams/index.rst(2 hunks)docs/source/teams/overview.rst(1 hunks)docs/source/teams/query_performance.rst(1 hunks)docs/source/user_guide/app.rst(10 hunks)docs/source/user_guide/evaluation.rst(14 hunks)docs/source/user_guide/index.rst(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/source/model_zoo/index.rst
- docs/source/index.rst
🔇 Additional comments (15)
docs/source/dataset_zoo/index.rst (1)
25-26: LGTM: Section header formatting is consistent
The section header formatting follows RST conventions and maintains consistency with other documentation sections.
docs/source/user_guide/index.rst (2)
54-58: Verify the existence of annotation.html page
Ensure that the linked documentation page exists and contains the appropriate content.
Also applies to: 114-114
✅ Verification successful
The annotation.rst page exists and contains appropriate content
The linked documentation page exists at docs/source/user_guide/annotation.rst and contains comprehensive content about annotating datasets, including detailed sections on setup, requesting annotations, loading annotations, and managing annotation runs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the annotation documentation exists
fd -e rst -e md "annotation" docs/source/
Length of output: 79
Script:
#!/bin/bash
# Check the content of the annotation.rst file to verify it contains appropriate content
cat docs/source/user_guide/annotation.rst
Length of output: 37923
41-46: Verify the impact of removing "Grouped datasets" section
Please ensure that the content about multiview image, video, and point cloud data is covered elsewhere in the documentation.
✅ Verification successful
Content about grouped datasets is still available and properly documented
The content about multiview image, video, and point cloud data is extensively covered in the groups.rst documentation file. The file contains comprehensive information about:
- Creating and working with grouped datasets
- Handling multiview scenes with different perspectives
- Managing image, video, and 3D point cloud data
- Examples and tutorials for working with grouped data
The removal of the callout from the index page appears to be a documentation reorganization, as the content itself remains well-documented in the dedicated groups.rst file, which is still referenced and accessible via groups.html.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to grouped datasets content
rg -l "multiview|grouped.*datasets" docs/
Length of output: 434
Script:
#!/bin/bash
# Check content of groups.rst to verify if it's still present and contains the relevant information
cat docs/source/user_guide/groups.rst
# Check if there are any references to groups.html in the documentation
rg "groups\.html" docs/
Length of output: 35912
docs/source/teams/index.rst (2)
110-111: Verify the cross-reference to Model Evaluation panel
The link to "../user_guide/app.html#app-model-evaluation-panel" crosses documentation sections. Please verify that this cross-reference is intentional and the target section exists.
✅ Verification successful
Cross-reference is valid and correctly linked
The link to "../user_guide/app.html#app-model-evaluation-panel" is valid as the target section exists in docs/source/user_guide/app.rst with the correct anchor .. _app-model-evaluation-panel: and corresponding content for the Model Evaluation panel.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced section exists in the user guide
rg -l "app-model-evaluation-panel" docs/source/user_guide/
Length of output: 127
Script:
#!/bin/bash
# Check the content of app.rst to verify the section exists
rg -A 5 "app-model-evaluation-panel" docs/source/user_guide/app.rst
Length of output: 595
170-171: Verify new documentation files exist
Ensure that the documentation files for new sections (data_quality.rst and query_performance.rst) exist and contain appropriate content.
✅ Verification successful
Let me check the content of these files to ensure they contain appropriate content.
Documentation files exist and contain appropriate content
Both data_quality.rst and query_performance.rst files are present in the correct location and contain comprehensive documentation. The files include:
- Detailed explanations of features
- Usage instructions
- Configuration options
- Visual references to images
- Proper RST formatting with sections and subsections
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if new documentation files exist
fd -e rst "data_quality|query_performance" docs/source/teams/
Length of output: 138
Script:
#!/bin/bash
# Check content of the documentation files
cat docs/source/teams/data_quality.rst docs/source/teams/query_performance.rst
Length of output: 13972
docs/source/brain.rst (3)
41-60: Well-structured introduction to the leaky splits feature.
The introduction effectively explains the purpose and importance of detecting leaky splits between dataset splits. The text is clear and concise.
Line range hint 1415-1618: Well-documented near duplicates feature with clear examples.
The documentation effectively covers:
- Use cases and motivation
- API usage
- Visualization capabilities
- Integration with the App
The code examples are clear and demonstrate practical usage.
1620-1655: Concise documentation of exact duplicates functionality.
The section clearly explains the purpose and basic usage of exact duplicates detection. The API description is accurate and includes expected input/output formats.
docs/source/user_guide/evaluation.rst (2)
16-17: Enhanced user guidance for model evaluation.
The text now properly references the new Model Evaluation panel, improving discoverability of this feature.
1268-1271: Clear integration with Model Evaluation panel.
The note effectively directs users to the preferred evaluation workflow using the Model Evaluation panel.
docs/source/user_guide/app.rst (2)
403-409: Clear documentation of query performance optimization.
The text effectively explains the database index indicators and their significance.
1743-1875: Comprehensive documentation of the Model Evaluation panel.
Excellent coverage of:
- Panel functionality
- Review status management
- Evaluation notes
- Performance metrics visualization
- Model comparison capabilities
The documentation is well-structured with clear examples and screenshots.
docs/source/plugins/developing_plugins.rst (2)
144-146: LGTM! Clear and accurate reference update.
The change improves guidance by directing users to the more appropriate hello-world-plugin-js repository for getting started with JS plugins.
3135-3146: LGTM! Well-structured new Getting Started section.
The new section provides clear guidance for JS plugin development by:
- Introducing the hello-world-plugin-js repository as a starting point
- Mentioning the fiftyone-js-plugin-build package for build configuration
docs/source/release-notes.rst (1)
Line range hint 1-3000: LGTM! Well-structured and comprehensive release notes.
The release notes are well-organized chronologically, with clear sections documenting changes across different components. The formatting follows RST conventions consistently and version numbers follow semantic versioning practices.
| .. customcalloutitem:: | ||
| :header: Evaluating models __SUB_NEW__ | ||
| :description: Use FiftyOne's builtin methods to evaluate your models and analyze their strengths and weaknesses. | ||
| :button_text: Learn more about evaluating models | ||
| :button_link: evaluation.html |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove "SUB_NEW" suffix from section headers
The "SUB_NEW" suffix should be removed as it's not a standard documentation practice and may confuse readers.
- :header: Evaluating models __SUB_NEW__
+ :header: Evaluating models
- Evaluating models __SUB_NEW__ <evaluation>
+ Evaluating models <evaluation>Also applies to: 115-115
| @@ -93,11 +93,29 @@ pages on this site apply to Teams deployments as well. | |||
| :button_link: teams_app.html | |||
|
|
|||
| .. customcalloutitem:: | |||
| :header: Data Lens | |||
| :header: Data Lens __SUB_NEW__ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove "SUB_NEW" suffix from all callout headers
The "SUB_NEW" suffix should be removed from all callout headers for consistency with documentation standards.
- :header: Data Lens __SUB_NEW__
+ :header: Data Lens
- :header: Data Quality __SUB_NEW__
+ :header: Data Quality
- :header: Model Evaluation __SUB_NEW__
+ :header: Model Evaluation
- :header: Query Performance __SUB_NEW__
+ :header: Query PerformanceAlso applies to: 101-106, 107-112, 113-118
benjaminpkane
force-pushed
the
merge/release/v1.1.0
branch
from
62d8f4e
to
eff1e1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (13)
fiftyone/server/lightning.py (1)
518-524: Consider adding error handling for value accessWhile the float handling is improved, consider adding explicit error handling for the dictionary access to prevent potential KeyError exceptions.
- if "value" in value: - value = value["value"] + try: + if "value" in value: + value = value["value"] + except (TypeError, AttributeError) as e: + logger.debug(f"Error parsing result: {e}") + return Nonefiftyone/server/view.py (1)
617-619: Add validation for support field range valuesThe range handling for support fields could benefit from additional validation to ensure robustness.
Consider adding validation:
if "range" in args: mn, mx = args["range"] + if not isinstance(mn, (int, float)) or not isinstance(mx, (int, float)): + raise ValueError("Range values must be numeric") + if mn > mx: + raise ValueError("Range minimum must not exceed maximum") expr = (f[0] >= mn) & (f[1] <= mx)app/packages/core/src/components/Filters/NumericFieldFilter/state.ts (1)
28-38: Consider optimizing the nonfinitesText selector implementationThe implementation follows good functional programming practices and has clear return types. However, there are opportunities for improvement:
Consider this more explicit and efficient implementation:
export const nonfinitesText = selectorFamily({ key: "nonfinitesText", get: (params: { path: string; modal: boolean }) => ({ get }) => { const data = get(nonfiniteData({ ...params, extended: false })); - const result = Object.entries(data).filter( - ([k, v]) => k !== "none" && Boolean(v) - ); - - return result.length ? result.map(([key]) => key).join(", ") : null; + const validKeys = Object.entries(data) + .filter(([key, value]) => key !== "none" && Boolean(value)) + .map(([key]) => key); + + return validKeys.length > 0 ? validKeys.join(", ") : null; }, });This refactoring:
- Makes the filtering intention more explicit with better variable naming
- Reduces unnecessary array transformations
- Maintains the same functionality with improved readability
docs/source/teams/query_performance.rst (1)
35-37: Consider adding example toast notification screenshotWhile the toast notification behavior is well described, consider adding a screenshot to help users identify it more easily.
docs/source/brain.rst (2)
1420-1422: Consider adding visual example for near duplicatesWhile the near duplicates explanation is clear, consider adding a visual example showing what constitutes a "near duplicate" to help users better understand the concept.
1652-1654: Minor typo in explanationThere's a small typo in the explanation.
-What to expect: Exact duplicates analysis uses filehases to identify +What to expect: Exact duplicates analysis uses filehashes to identifydocs/source/user_guide/evaluation.rst (1)
Line range hint
403-490: Enhanced query performance documentationClear explanation of database indexing features and best practices:
- Lightning bolt indicators for indexed fields
- Recommendations for index creation
- Examples for different dataset types
Consider adding a note about index maintenance overhead and storage implications for very large datasets.
docs/source/user_guide/app.rst (1)
435-439: Added important performance optimization tipValuable guidance on using indexed fields as the first filter for optimal query performance.
Consider adding an example showing the performance difference between indexed and non-indexed field filtering.
docs/source/release-notes.rst (3)
4429-4429: Remove stray line number.The line
4429~appears to be a stray line number that should be removed.
Line range hint
1-4429: Consider standardizing version number formatting in headings.For better consistency, consider using a standardized format for version numbers in headings. For example:
- Current:
FiftyOne 0.7.4,FiftyOne Teams 1.2- Suggested:
FiftyOne v0.7.4,FiftyOne Teams v1.2.0This would make the version numbers more prominent and consistent throughout the document.
Line range hint
1-4429: Consider using relative links for better maintainability.Some documentation links use absolute URLs (e.g.,
https://voxel51.com/docs/fiftyone/...). Consider using relative links instead for better maintainability when docs are moved or restructured.app/packages/spotlight/src/row.ts (1)
166-166: LGTM! Consider adding JSDoc commentsThe render call has been simplified and properly typed. However, documentation explaining the purpose and behavior of the
zoomingparameter would improve maintainability.Add JSDoc comments to explain the zooming behavior:
show( element: HTMLDivElement, attr: typeof BOTTOM | typeof TOP, zooming: boolean, config: SpotlightConfig<K, V> + /** + * @param zooming Indicates whether the row is currently in a zoom transition + * state, affecting how items are rendered + */ ): void {app/packages/spotlight/src/section.ts (1)
303-305: Optimize array operations for better performanceThe current implementation creates an unnecessary intermediate array with the spread operator before filtering. Consider iterating directly over the items.
- [...end.remainder, ...data.items].filter( - (i) => !this.#itemIds.has(i.id.description) - ), + end.remainder.concat(data.items).filter( + (i) => !this.#itemIds.has(i.id.description) + ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (21)
docs/source/images/app/app-query-performance-disabled.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/app-query-performance.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-class.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-compare.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-confusion.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-metric.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-notes.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-open.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-review.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/model-evaluation-summary.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/teams/data_quality_brightness_analysis.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_brightness_mark_as_reviewed.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_brightness_scan.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_brightness_scan_options.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_brightness_scheduled.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_brightness_slider.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/teams/data_quality_brightness_tag.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_home.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_new_samples_home.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/data_quality_new_samples_modal.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/qp_toggle.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (26)
app/packages/core/src/components/Common/utils.tsx(1 hunks)app/packages/core/src/components/Filters/NumericFieldFilter/RangeSlider.tsx(1 hunks)app/packages/core/src/components/Filters/NumericFieldFilter/state.ts(2 hunks)app/packages/core/src/components/Grid/Grid.tsx(2 hunks)app/packages/core/src/components/Grid/useRefreshers.ts(0 hunks)app/packages/core/src/components/Grid/useSelect.ts(1 hunks)app/packages/core/src/plugins/SchemaIO/components/TableView.tsx(2 hunks)app/packages/spotlight/src/row.ts(2 hunks)app/packages/spotlight/src/section.ts(4 hunks)app/packages/spotlight/src/types.ts(1 hunks)docs/source/brain.rst(5 hunks)docs/source/dataset_zoo/index.rst(1 hunks)docs/source/index.rst(1 hunks)docs/source/model_zoo/index.rst(1 hunks)docs/source/plugins/developing_plugins.rst(3 hunks)docs/source/release-notes.rst(3 hunks)docs/source/teams/data_quality.rst(1 hunks)docs/source/teams/index.rst(2 hunks)docs/source/teams/overview.rst(1 hunks)docs/source/teams/query_performance.rst(1 hunks)docs/source/user_guide/app.rst(10 hunks)docs/source/user_guide/evaluation.rst(14 hunks)docs/source/user_guide/index.rst(2 hunks)fiftyone/server/lightning.py(11 hunks)fiftyone/server/view.py(2 hunks)tests/unittests/lightning_tests.py(27 hunks)
💤 Files with no reviewable changes (1)
- app/packages/core/src/components/Grid/useRefreshers.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/source/index.rst
- docs/source/model_zoo/index.rst
- app/packages/core/src/components/Grid/Grid.tsx
- docs/source/dataset_zoo/index.rst
- docs/source/teams/overview.rst
- app/packages/core/src/plugins/SchemaIO/components/TableView.tsx
- app/packages/core/src/components/Common/utils.tsx
- app/packages/core/src/components/Grid/useSelect.ts
- app/packages/core/src/components/Filters/NumericFieldFilter/RangeSlider.tsx
- docs/source/teams/data_quality.rst
- tests/unittests/lightning_tests.py
- docs/source/user_guide/index.rst
- app/packages/spotlight/src/types.ts
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/spotlight/src/section.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/spotlight/src/row.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/components/Filters/NumericFieldFilter/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.
🔇 Additional comments (24)
fiftyone/server/lightning.py (6)
12-12: LGTM: Enhanced frame field handling
The addition of is_frames flag and its propagation through the resolver chain improves the handling of frame-specific queries while maintaining type safety.
Also applies to: 130-141
145-149: LGTM: Improved filter initialization logic
The filter initialization is now more explicit and properly handles all cases, including when group_field or slice is not present.
216-216: LGTM: Consistent frame field handling
The addition of is_frame_field to all resolver return statements ensures consistent frame field handling across different data types.
Also applies to: 234-234, 263-263, 279-279, 296-296
305-309: LGTM: Enhanced type safety and filter handling
The updated type hints and conditional filter handling improve type safety and correctly handle frame-specific queries.
Also applies to: 315-318
330-331: LGTM: Improved query construction
The explicit has_list check and the addition of sort operations ensure correct query behavior and maintain proper ordering.
Also applies to: 336-338
438-439: LGTM: Enhanced float value handling
The addition of non-finite float handling and proper placement of existence checks improves the robustness of float value queries.
Also applies to: 441-441
fiftyone/server/view.py (1)
Line range hint 617-651: LGTM! The range handling improvements are well-implemented
The changes consistently check for the existence of the "range" key before accessing it, which prevents potential KeyError exceptions. This defensive programming approach improves the robustness of the code.
🧰 Tools
🪛 Ruff (0.8.0)
642-642: Avoid inequality comparisons to True; use if not f: for false checks
Replace with not f
(E712)
642-642: Avoid inequality comparisons to False; use if f: for truth checks
Replace with f
(E712)
app/packages/core/src/components/Filters/NumericFieldFilter/state.ts (1)
1-2: Verify the migration from nonfiniteAtom to nonfiniteData
The change from nonfiniteAtom to nonfiniteData suggests a shift in state management approach. Let's verify this change is consistent across the codebase.
✅ Verification successful
Migration from nonfiniteAtom to nonfiniteData appears incomplete
The codebase shows inconsistent usage where both nonfiniteAtom and nonfiniteData coexist:
nonfiniteAtomis still used in:app/packages/state/src/recoil/pathFilters/numeric.tsapp/packages/core/src/components/Filters/NumericFieldFilter/Nonfinites.tsx
nonfiniteDatais defined inapp/packages/state/src/recoil/pathData/numeric.tsand used in multiple locations
Let's verify if this is an intentional design where both are meant to coexist:
The nonfiniteAtom and nonfiniteData serve different purposes
The code inspection reveals this is not a migration issue but rather two distinct selectors with different responsibilities:
nonfiniteAtom(in pathFilters): A boolean selector for filter state managementnonfiniteData(in pathData): A data selector for retrieving nonfinite values with counts
The import change in NumericFieldFilter/state.ts is correct as it needs the data values rather than filter states. Both selectors are meant to coexist as part of the application's state management architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of nonfiniteAtom and verify nonfiniteData usage
# Check for any remaining references to nonfiniteAtom
echo "Checking for remaining nonfiniteAtom usage:"
rg "nonfiniteAtom" --type ts
# Check the usage pattern of nonfiniteData
echo -e "\nVerifying nonfiniteData usage pattern:"
rg "nonfiniteData" --type ts -A 3
Length of output: 2948
Script:
#!/bin/bash
# Check the implementation details of both atoms to understand their relationship
echo "Checking nonfiniteAtom implementation:"
rg -A 10 "nonfiniteAtom = selectorFamily" --type ts
echo -e "\nChecking nonfiniteData implementation:"
rg -A 10 "nonfiniteData = selectorFamily" --type ts
Length of output: 1996
docs/source/teams/index.rst (2)
96-96: Remove "SUB_NEW" suffix from callout headers
This issue was previously identified and should be addressed for consistency with documentation standards.
Apply this diff to remove the suffix:
- :header: Data Lens __SUB_NEW__
+ :header: Data Lens
- :header: Data Quality __SUB_NEW__
+ :header: Data Quality
- :header: Model Evaluation __SUB_NEW__
+ :header: Model Evaluation
- :header: Query Performance __SUB_NEW__
+ :header: Query PerformanceAlso applies to: 101-106, 107-112, 113-118
170-171: LGTM! Well-structured toctree updates
The toctree entries are properly formatted and maintain documentation hierarchy.
docs/source/teams/query_performance.rst (1)
3-4: LGTM! Clear and comprehensive documentation
The Query Performance documentation is well-structured with:
- Clear feature introduction
- Proper version availability notice
- Concise explanation of functionality
Also applies to: 6-12
docs/source/brain.rst (1)
1281-1284: LGTM! Clear explanation of leaky splits concept
The introduction effectively explains the importance of identifying leaky splits and their impact on model evaluation.
docs/source/user_guide/evaluation.rst (5)
16-17: Improved documentation clarity and user guidance
The text now better guides users to the interactive exploration capabilities in the Model Evaluation panel.
58-62: Updated code example to use standardized terminology
The example now uses the standardized eval key instead of eval_predictions, maintaining consistency across the documentation.
Line range hint 63-205: Well-structured addition of Model Evaluation panel documentation
The new section comprehensively covers the Model Evaluation panel's features including:
- Review status management
- Evaluation notes
- Summary metrics
- Class performance analysis
- Confusion matrices
- Model comparison capabilities
Line range hint 545-568: Clear documentation of query performance controls
Well-explained section on disabling query performance optimization when needed, with clear instructions on both UI and configuration approaches.
1039-1043: Consistent cross-referencing to Model Evaluation panel
Added consistent note blocks throughout the document to guide users to the Model Evaluation panel for interactive analysis.
Also applies to: 1270-1272, 1489-1493, 1741-1746
docs/source/user_guide/app.rst (2)
403-409: Clear documentation of performance optimization features
Added clear explanation of database index usage in the sidebar, with visual indicators for indexed fields.
485-490: Important clarification about frame filtering limitations
Clear explanation of frame filtering limitations and alternative approaches using summary fields.
docs/source/plugins/developing_plugins.rst (2)
144-146: Updated plugin development guidance
Improved recommendation to use the FiftyOne Hello World JS Example repository as a starting point.
3135-3146: Added clear getting started section
New section provides better guidance for JS plugin development:
- References to hello-world-plugin-js example
- Introduction to fiftyone-js-plugin-build package
- Clear build process setup instructions
app/packages/spotlight/src/row.ts (1)
151-154: Verify the hidden state management
The method no longer accepts a hidden parameter, but still uses this.#hidden internally. This could lead to inconsistent state management since there's no clear way to set this private field.
Let's verify the usage of the hidden field across the codebase:
app/packages/spotlight/src/section.ts (2)
49-49: Clear #itemIds in destroy() method to prevent memory leaks
The newly added #itemIds set should be cleared when the section is destroyed to prevent memory leaks.
49-49: Verify unique item tracking implementation and add documentation
The new item tracking mechanism using #itemIds is a significant architectural change. Consider:
- Adding JSDoc comments explaining the purpose and lifecycle of
#itemIds - Verifying that items are correctly tracked across section splits and merges
Also applies to: 303-305, 450-453
| if "range" in args: | ||
| mn, mx = args["range"] | ||
| expr = (f >= mn) & (f <= mx) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type validation for numeric range values
The numeric range handling should validate the input types to prevent potential type-related errors.
Add type validation:
if "range" in args:
mn, mx = args["range"]
+ if isinstance(field, fof.IntField):
+ if not (isinstance(mn, int) and isinstance(mx, int)):
+ raise ValueError("Range values must be integers for IntField")
+ elif not (isinstance(mn, (int, float)) and isinstance(mx, (int, float))):
+ raise ValueError("Range values must be numeric for FloatField")
expr = (f >= mn) & (f <= mx)📝 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.
| if "range" in args: | |
| mn, mx = args["range"] | |
| expr = (f >= mn) & (f <= mx) | |
| if "range" in args: | |
| mn, mx = args["range"] | |
| if isinstance(field, fof.IntField): | |
| if not (isinstance(mn, int) and isinstance(mx, int)): | |
| raise ValueError("Range values must be integers for IntField") | |
| elif not (isinstance(mn, (int, float)) and isinstance(mx, (int, float))): | |
| raise ValueError("Range values must be numeric for FloatField") | |
| expr = (f >= mn) & (f <= mx) |
| if "range" in args: | ||
| mn, mx = args["range"] | ||
| p = fou.timestamp_to_datetime | ||
| expr = (f >= p(mn)) & (f <= p(mx)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for timestamp conversion
The timestamp to datetime conversion could fail with invalid input values.
Add error handling:
if "range" in args:
mn, mx = args["range"]
- p = fou.timestamp_to_datetime
- expr = (f >= p(mn)) & (f <= p(mx))
+ try:
+ p = fou.timestamp_to_datetime
+ expr = (f >= p(mn)) & (f <= p(mx))
+ except ValueError as e:
+ raise ValueError(f"Invalid timestamp values in range: {e}")📝 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.
| if "range" in args: | |
| mn, mx = args["range"] | |
| p = fou.timestamp_to_datetime | |
| expr = (f >= p(mn)) & (f <= p(mx)) | |
| if "range" in args: | |
| mn, mx = args["range"] | |
| try: | |
| p = fou.timestamp_to_datetime | |
| expr = (f >= p(mn)) & (f <= p(mx)) | |
| except ValueError as e: | |
| raise ValueError(f"Invalid timestamp values in range: {e}") |
| for (const i of rowItems) { | ||
| this.#itemIds.add(i.id.description); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null/undefined check for item ID description
The code assumes i.id.description is always defined. Consider adding a null check to prevent potential runtime errors.
for (const i of rowItems) {
- this.#itemIds.add(i.id.description);
+ if (i.id?.description) {
+ this.#itemIds.add(i.id.description);
+ }
}📝 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.
| for (const i of rowItems) { | |
| this.#itemIds.add(i.id.description); | |
| } | |
| for (const i of rowItems) { | |
| if (i.id?.description) { | |
| this.#itemIds.add(i.id.description); | |
| } | |
| } |
Merge
release/v1.1.0todevelopSummary by CodeRabbit
Release Notes
New Features
Documentation Improvements
Bug Fixes
These updates enhance the functionality and user experience across the FiftyOne platform.