Grid LRU caching with Looker size estimates #5214
Conversation
|
Warning Rate limit exceeded@benjaminpkane has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several modifications across multiple components and files. Key changes include enhancements to error handling in numeral formatting, updates 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
|
benjaminpkane
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
app/packages/looker/src/lookers/video.ts (2)
161-172: Consider optimizing the size estimation implementationThe size estimation logic could be improved in terms of performance and maintainability:
- Consider caching the size calculation result since first frame overlays are unlikely to change frequently
- The iteration could be replaced with a more efficient reduce operation
getSizeBytesEstimate() { let size = super.getSizeBytesEstimate(); - if (!this.firstFrame.overlays?.length) { - return size; - } - - for (let index = 0; index < this.firstFrame.overlays.length; index++) { - size += this.firstFrame.overlays[index].getSizeBytes(); - } - - return size; + return size + (this.firstFrame.overlays?.reduce( + (acc, overlay) => acc + overlay.getSizeBytes(), + 0 + ) ?? 0); }
161-172: Consider adding memory management documentationThe class uses WeakRef for frame management which is good for memory, but the relationship between size estimation and frame references should be documented.
Add a class-level JSDoc comment explaining the memory management strategy:
/** * VideoLooker handles video frame management with memory-efficient practices: * - Uses WeakRef for frame caching to allow garbage collection * - Implements size estimation for LRU cache management * - First frame is kept in memory for quick access */ export class VideoLooker extends AbstractLooker<VideoState, VideoSample> {Also applies to: 26-27
app/packages/core/src/components/Grid/useLookerCache.ts (2)
18-18: Remove unnecessary statement 'reset;'The standalone
reset;statement inside theuseMemocallback does not have any effect and can be safely removed. The dependency array[reset]already ensures that the memoized value recalculates whenresetchanges.Apply this diff to remove the unnecessary statement:
/** CLEAR CACHE WHEN reset CHANGES */ - reset; /** CLEAR CACHE WHEN reset CHANGES */
27-28: Avoid using console.log in production codeThe
console.logstatement on line 27 may lead to unnecessary logging in the production environment. Consider removing it or using a logging library with appropriate log levels.Apply this diff to remove the console log:
sizeCalculation: (looker) => { - console.log(looker.getSizeBytesEstimate()); return looker.getSizeBytesEstimate(); },app/packages/core/src/components/Grid/useSelect.ts (1)
19-37: Optimize the update logic and avoid potential null references
- Null Check Enhancement: The check
if (!instance)on line 22 prevents null reference errors, which is good. However, it's more concise to return early if the instance is null.- Simplify the Loop: Consider restructuring the loop to improve readability.
You can refactor the code block as follows:
const retained = new Set<string>(); spotlight?.updateItems((id) => { const instance = store.get(id.description); if (!instance) { return; } retained.add(id.description); instance.updateOptions({ ...options, fontSize, selected: selected.has(id.description), }); }); -for (const id of store.keys()) { +for (const id of store.keys()) { if (!retained.has(id)) { store.delete(id); } }This improves readability and ensures that all non-retained instances are deleted from the store.
app/packages/core/src/components/Grid/useLookerCache.test.ts (1)
31-34: Handle the possibility of 'looker' being undefinedThe check for
lookeron line 32 is good practice. However, throwing a generic error may not provide enough context during test failures. Consider using an assertion or providing a clearer error message.You can provide a more descriptive error message:
if (!looker) { - throw new Error("looker is missing"); + throw new Error("Failed to retrieve 'looker' from the cache"); }Alternatively, you can use an assertion library for better test clarity.
app/packages/core/src/components/Common/utils.tsx (1)
71-78: Consider extracting format string selection logicThe format string selection logic could be extracted into a separate function for better maintainability and testability.
Consider refactoring to:
+ const getFormatString = (fieldType: string, bounds: [number, number]) => { + if ([INT_FIELD, FRAME_NUMBER_FIELD, FRAME_SUPPORT_FIELD].includes(fieldType)) { + return "0a"; + } + return bounds[1] - bounds[0] < 0.1 ? "0.0000a" : "0.00a"; + }; 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" + getFormatString(fieldType, bounds) );app/packages/core/src/components/Filters/NumericFieldFilter/RangeSlider.tsx (1)
41-43: Consider using a template for i18n supportThe text concatenation should be moved to a template for better internationalization support.
Consider using a template:
- <Box text={nonfinitesText ? `${nonfinitesText} present` : "No results"} /> + <Box text={nonfinitesText ? t('nonfinites.present', { text: nonfinitesText }) : t('common.no_results')} />app/packages/core/src/components/Grid/Grid.tsx (1)
Line range hint
73-86: LGTM! Performance optimization with improved cache handling.The changes effectively optimize performance by:
- Utilizing cached looker instances
- Skipping looker creation during fast scrolling
- Simplifying control flow with early returns
Consider extracting the looker creation logic into a separate function for better maintainability:
- const looker = createLooker.current?.( - { ...result, symbol: id }, - { - fontSize: getFontSize(), - } - ); + const createNewLooker = () => { + const looker = createLooker.current?.( + { ...result, symbol: id }, + { + fontSize: getFontSize(), + } + ); + looker.addEventListener("selectthumbnail", ({ detail }) => + selectSample.current?.(detail) + ); + return looker; + }; + const looker = createNewLooker();app/packages/spotlight/src/section.ts (1)
303-305: Consider memory management for large datasetsWhile the filtering logic effectively prevents duplicate items, for very large datasets, consider implementing a cleanup mechanism to prevent unbounded growth of the #itemIds Set.
- [...end.remainder, ...data.items].filter( - (i) => !this.#itemIds.has(i.id.description) - ), + const newItems = [...end.remainder, ...data.items].filter(i => { + // Consider clearing #itemIds when it grows too large + if (this.#itemIds.size > MAX_CACHED_IDS) { + this.#itemIds.clear(); + } + return !this.#itemIds.has(i.id.description); + }),fiftyone/server/lightning.py (1)
138-141: Avoid variable shadowing ofis_framesin list comprehensionIn the list comprehension at lines 138-141, the loop variable
is_framesshadows the outer variableis_frames, which can lead to confusion and potential errors. Consider renaming the loop variable to prevent shadowing.Apply this diff to rename the loop variable:
flattened = [ (collection, item, is_frame_field) for collection, sublist, is_frame_field in zip( collections, queries, is_frames ) for item in sublist ]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
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/useLookerCache.test.ts(1 hunks)app/packages/core/src/components/Grid/useLookerCache.ts(1 hunks)app/packages/core/src/components/Grid/useRefreshers.ts(2 hunks)app/packages/core/src/components/Grid/useSelect.ts(2 hunks)app/packages/core/src/plugins/SchemaIO/components/TableView.tsx(2 hunks)app/packages/looker/src/lookers/abstract.ts(2 hunks)app/packages/looker/src/lookers/video.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)docs/source/plugins/developing_plugins.rst(2 hunks)fiftyone/server/lightning.py(11 hunks)fiftyone/server/view.py(2 hunks)tests/unittests/lightning_tests.py(27 hunks)
🧰 Additional context used
📓 Path-based instructions (14)
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/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/looker/src/lookers/video.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/useRefreshers.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/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.
app/packages/core/src/components/Grid/useLookerCache.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/core/src/components/Grid/useLookerCache.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/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.
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/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.
🔇 Additional comments (21)
app/packages/looker/src/lookers/abstract.ts (2)
194-209: LGTM! Well-implemented size estimation.
The implementation correctly:
- Accounts for RGBA image data (4 bytes per pixel)
- Handles null/undefined cases
- Includes overlay sizes in the total estimate
228-239: LGTM! Improved state transition handling.
The changes enhance readability and reliability:
- Clear parameter naming with
previousandnext - Explicit load state detection using
overlaysPreparedanddimensions - Proper sequencing of event dispatching
app/packages/looker/src/lookers/video.ts (1)
161-172: Verify memory usage patterns
The size estimation is crucial for the LRU cache implementation. Let's verify how this estimation is used across the codebase.
✅ Verification successful
Let me gather more information about the cache implementation and memory usage patterns.
Size estimation implementation is correctly used in LRU cache
The size estimation is properly integrated into the LRU cache implementation:
- Used as
sizeCalculationcallback in the LRU cache configuration - Respects the
MAX_LRU_CACHE_SIZElimit - Properly handles cleanup through the
disposecallback - The video looker correctly extends the base implementation by adding overlay sizes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to getSizeBytesEstimate to understand usage patterns
rg -A 3 "getSizeBytesEstimate"
# Look for LRU cache implementations that might use this estimation
ast-grep --pattern 'class $_ implements $_Cache {
$$$
getSizeBytesEstimate($$$) {
$$$
}
$$$
}'
Length of output: 1861
Script:
#!/bin/bash
# Look for cache size limits or memory configurations
rg -A 5 "maxSize|cacheSize|memoryLimit" app/packages/core/src/components/Grid/
# Check the base class implementation
ast-grep --pattern 'class AbstractLooker {
$$$
getSizeBytesEstimate() {
$$$
}
$$$
}'
# Find cache eviction or cleanup logic
rg -A 5 "evict|cleanup|dispose" app/packages/core/src/components/Grid/
Length of output: 1360
app/packages/core/src/components/Grid/useSelect.ts (1)
34-37:
Ensure proper iteration and deletion of stale entries
When deleting entries from the store during iteration, ensure that you're not mutating the collection while iterating over it, which can lead to inconsistent behavior.
Consider collecting the keys to delete and removing them after the iteration:
-for (const id of store.keys()) {
- if (retained.has(id)) continue;
- store.delete(id);
-}
+const keysToDelete = [];
+for (const id of store.keys()) {
+ if (!retained.has(id)) {
+ keysToDelete.push(id);
+ }
+}
+for (const id of keysToDelete) {
+ store.delete(id);
+}Likely invalid or redundant comment.
app/packages/spotlight/src/types.ts (1)
41-41: Update documentation to reflect changed 'Render' signature
The Render type's signature has changed, replacing soft: boolean and disable: boolean with zooming: boolean. Ensure that all implementations of the Render function are updated accordingly, and update any associated documentation or comments to prevent confusion.
No code changes needed here, but verify that all usages of Render are consistent with the new signature.
Run the following script to find all implementations of Render and check their parameters:
app/packages/core/src/components/Grid/useLookerCache.test.ts (1)
42-45: Verify cache reset behavior upon 'reset' prop change
The test checks that the cache sizes reset after rerendering with a new reset prop. Ensure that this behavior aligns with the intended functionality of the useLookerCache hook.
Confirm that resetting the cache on reset prop change is the desired behavior. If so, the test correctly validates this functionality.
app/packages/core/src/components/Filters/NumericFieldFilter/state.ts (2)
28-38: Improve the logic for generating 'nonfinitesText'
The new selector nonfinitesText filters out entries where the key is "none" and the value is falsy. Ensure that this logic correctly captures all relevant non-finite values and that the resulting text is accurate.
The implementation appears correct. It filters out irrelevant entries and joins the keys of valid non-finite values.
28-38:
Update key in selectors to avoid duplication
The key for the nonfinitesText selector is set to "nonfinitesText", but there is another selector with the same key "hasBounds". Ensure that all selector keys are unique to prevent conflicts in the Recoil state management.
Apply this diff to correct the key for the hasDefaultRange selector:
-export const hasDefaultRange = selectorFamily({
- key: "hasBounds",
+export const hasDefaultRange = selectorFamily({
+ key: "hasDefaultRange",This ensures that each selector has a unique key.
Likely invalid or redundant comment.
app/packages/core/src/components/Grid/useRefreshers.ts (2)
7-7: LGTM!
The import of the new useLookerCache hook is correctly placed.
79-79: LGTM!
The replacement of the manual cache implementation with the useLookerCache hook is a good improvement for maintainability.
app/packages/core/src/components/Filters/NumericFieldFilter/RangeSlider.tsx (1)
38-38: LGTM!
The addition of the nonfinitesText state variable using Recoil is correct.
app/packages/spotlight/src/row.ts (1)
Line range hint 151-166: LGTM! Improved method signature with better state management.
The changes effectively:
- Remove the redundant
hiddenparameter - Simplify the show method signature
- Properly propagate the zooming state to render
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
224-224: Same font-size issue as noted above.
app/packages/spotlight/src/section.ts (2)
49-49: LGTM: New Set for tracking processed items
The addition of #itemIds as a private Set to track processed items is a good approach for preventing duplicates.
450-453: LGTM: Proper tracking of processed item IDs
The implementation correctly adds item IDs to the tracking Set as they are processed.
docs/source/plugins/developing_plugins.rst (1)
3135-3146: LGTM: Clear and helpful getting started guide
The new section provides excellent guidance for new plugin developers by:
- Referencing the hello-world-plugin-js template repository
- Introducing the fiftyone-js-plugin-build package
- Explaining the build process setup
fiftyone/server/lightning.py (1)
519-525:
Fix variable assignment error in _parse_result function
The variable value is used before being assigned when checking "value" in value, which will result in a NameError. Assign value = data[0] before using it.
Apply this diff to fix the variable assignment:
def _parse_result(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
)Likely invalid or redundant comment.
fiftyone/server/view.py (2)
617-619: Ensure safe access to "range" in _make_scalar_expression
Adding the check if "range" in args before unpacking args["range"] prevents potential KeyError exceptions, enhancing the robustness of the function when "range" is not provided.
644-651: Enhance robustness by checking "range" in arguments
In the handling of datetime and numeric fields, verifying the presence of "range" in args before unpacking prevents exceptions and ensures correct behavior when "range" is absent.
tests/unittests/lightning_tests.py (2)
Line range hint 538-969: Update tests to reflect changes in float handling
The test cases have been updated to account for non-finite float values. Setting max and min to None when non-finite values are present ensures that the tests accurately reflect the new behavior.
Line range hint 1063-1080: Add test cases for float fields in group datasets
Including confidence values in the classifications fields enhances test coverage for float handling in group datasets, ensuring min and max calculations are properly validated.
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx
Outdated
Show resolved
Hide resolved
| for (let index = 0; index < this.sampleOverlays.length; index++) { | ||
| size += this.sampleOverlays[index].getSizeBytes(); | ||
| } |
There was a problem hiding this comment.
For overlays, we might want to add one more case in sizer for bitmaps since they have the largest memory footprint. Something along the lines of:
if (typeof object === "object" && object.constructor.name === "ImageBitmap") {
size += object.height * object.width * 4;
}
What changes are proposed in this pull request?
Adds a maximum LRU cache size in bytes for Grid Lookers. Currently hardcoded to 1GB
How is this patch tested? If it is not, please explain why.
Hook test
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Release Notes
New Features
RangeSlidercomponent when no bounds are available.useLookerCache, for optimized cache management.Bug Fixes
Gridcomponent for improved performance during fast scrolling.VideoLookerclass.Documentation
Tests