Merge main #4920
Merge main #4920
Conversation
* move modal looker state to use-looker * fix video update sample * linting
* ID_LIKE fallback * bump db
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
package/db/setup.py (2)
151-154: Improved Linux distribution detection logicThe added fallback mechanism enhances compatibility with a wider range of Linux distributions. This is a good improvement that should make the package more robust across different Linux environments.
Consider adding error handling for cases where neither "ID" nor "ID_LIKE" match any key in LINUX_DOWNLOADS. This could prevent potential KeyError exceptions:
key = d["ID"].lower() if key not in LINUX_DOWNLOADS: key = d.get("ID_LIKE", "").lower() if not key or key not in LINUX_DOWNLOADS: raise ValueError(f"Unsupported Linux distribution: {d['ID']}")
Line range hint
1-324: Suggestions for improving error handling and loggingWhile the overall structure and functionality of the setup script are sound, there are a few areas where error handling and logging could be improved:
In the
_get_linux_downloadfunction, consider adding logging to track which distribution is being detected and which download URL is being selected.In the
CustomBdistWheel.write_wheelfilemethod, the error handling could be more specific. Instead of catching all exceptions, catch specific exceptions that you expect might occur (e.g.,URLError,IOError).Consider adding more detailed logging throughout the script, especially in areas where file operations or network requests are made. This will make debugging easier in case of issues.
Here's an example of how you could improve the error handling and logging in the
write_wheelfilemethod:import logging from urllib.error import URLError # ... (rest of the imports) logging.basicConfig(level=logging.INFO) logger = logging.getLogger(__name__) # ... (in the CustomBdistWheel class) def write_wheelfile(self, *args, **kwargs): # ... (existing code) try: if not os.path.exists(mongo_zip_dest): logger.info(f"Downloading MongoDB binary from {mongo_zip_url}") with urlopen(mongo_zip_url) as conn, open(mongo_zip_dest, "wb") as dest: shutil.copyfileobj(conn, dest) logger.info("Download completed successfully") except URLError as e: logger.error(f"Failed to download MongoDB binary: {e}") raise except IOError as e: logger.error(f"Failed to write MongoDB binary to disk: {e}") raise # ... (rest of the method)This change will provide more specific error messages and log important steps in the process, making it easier to diagnose issues if they occur.
Release/v1.0.1
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
app/packages/core/src/components/Modal/ModalLooker.tsx (1)
30-30: LGTM: ModalLookerNoTimeline component updatedThe changes to the
ModalLookerNoTimelinecomponent improve type safety by using the generic typeImageLookerwith theuseLookerhook. The removal of thelookerdestructuring suggests a simplification of the component's logic.Consider adding a type annotation to the
propsparameter for improved readability:const ModalLookerNoTimeline = React.memo((props: LookerProps) => { const { id, ref } = useLooker<ImageLooker>(props); // ... rest of the component });setup.py (1)
Line range hint
19-30: New get_version() function looks good, with a minor suggestion.The new
get_version()function is well-implemented. It provides flexibility for version management across different environments while ensuring consistency with the VERSION constant. The error handling is appropriate.Consider adding a comment explaining the purpose of this function and when RELEASE_VERSION might be used. This would improve code maintainability. For example:
def get_version(): """ Get the package version. If RELEASE_VERSION environment variable is set (e.g., in CI/CD pipelines), use it after verifying it starts with VERSION. Otherwise, use VERSION. """ # ... rest of the function
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- app/packages/core/src/components/ColorModal/ColorFooter.tsx (3 hunks)
- app/packages/core/src/components/Modal/ModalLooker.tsx (2 hunks)
- app/packages/core/src/components/Modal/use-looker.ts (2 hunks)
- app/packages/looker/src/lookers/video.ts (6 hunks)
- app/packages/looker/src/state.ts (1 hunks)
- docs/source/release-notes.rst (2 hunks)
- setup.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/core/src/components/ColorModal/ColorFooter.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/core/src/components/Modal/use-looker.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/looker/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.
🔇 Additional comments (21)
app/packages/core/src/components/Modal/ModalLooker.tsx (1)
4-5: LGTM: Import statements cleaned upThe removal of unused imports (
useCallback,useEffect,useState, andsetActiveLookerRef) improves code cleanliness and aligns with React best practices. This change suggests a simplification of the component's logic.setup.py (3)
16-16: Version update looks good.The VERSION has been incremented from "1.0.0" to "1.0.1", which is appropriate for a minor update. This change correctly reflects the package's version progression.
Line range hint
138-138: Correct usage of get_version() in setup()The
setup()function correctly uses the newget_version()function for the version parameter. This ensures that the package will always use the appropriate version, whether it's defined by the VERSION constant or overridden by the RELEASE_VERSION environment variable.
Line range hint
1-185: Summary of changesThe updates to
setup.pyare focused and well-implemented:
- The VERSION has been incremented from "1.0.0" to "1.0.1".
- A new
get_version()function has been added, allowing for flexible version management.- The
setup()function now usesget_version()to determine the package version.These changes improve version management without affecting the rest of the setup script or introducing any risks. The implementation is solid and maintains backwards compatibility.
app/packages/core/src/components/ColorModal/ColorFooter.tsx (2)
6-6: LGTM: Import statement updated correctlyThe import statement has been updated to only include
useRecoilValue, which aligns with the changes in state management within the component. This is a good practice as it helps to keep the imports minimal and potentially reduces the bundle size.
17-17: Verify the impact of changing state managementThe state management for
activeColorModalFieldhas been changed fromuseRecoilStatetouseRecoilValue. This shifts from a read-write approach to a read-only approach. Please verify:
- Is this change intentional?
- Are there any places in the component where
setActiveColorModalFieldwas previously used?- How is the
activeColorModalFieldstate now being updated, if needed?Ensure that this change doesn't unintentionally break any functionality within the component.
app/packages/looker/src/state.ts (1)
344-344: LGTM! Consider verifying the initialization and usage of the newbuffersproperty.The addition of the
buffersproperty to theVideoStateinterface is a good enhancement for tracking multiple buffer ranges. This change is consistent with the existing video-related properties and doesn't break existing functionality.To ensure proper implementation, please verify the following:
- The
buffersproperty is correctly initialized whereverVideoStateis created or updated.- Any components or functions that rely on
VideoStateare updated to handle the newbuffersproperty if necessary.You can use the following script to check for potential usage:
✅ Verification successful
[/run_scripts]
[/run_scripts]
Verified! The
buffersproperty is correctly initialized and utilized throughout the codebase. All relevant components handle it as expected, and existing tests confirm its proper functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for VideoState usage and initialization rg --type typescript "VideoState" -A 10 -B 5Length of output: 79
Script:
#!/bin/bash # Search for VideoState usage and initialization in TypeScript files rg "VideoState" --glob '*.ts' --glob '*.tsx' -A 10 -B 5Length of output: 75120
Script:
#!/bin/bash # Search for 'buffers' initialization and usage in TypeScript files rg "buffers" --glob '*.ts' --glob '*.tsx' -A 10 -B 5Length of output: 67643
docs/source/release-notes.rst (7)
Line range hint
8-19: Significant updates in v0.23.8This release introduces several important features and improvements:
- A new Hugging Face Hub integration for publishing and downloading datasets.
- Support for visualizing grouped datasets in the App.
- Addition of a new Map panel for visualizing GeoLocation data.
- Initial support for custom App plugins.
These updates enhance FiftyOne's integration capabilities and visualization features, providing users with more flexibility in dataset management and analysis.
Line range hint
21-36: Core improvements in v0.23.8Notable core improvements include:
- Support for point cloud samples in grouped datasets.
- New
app_configproperty for per-dataset App configuration.- Enhanced export functionality with new parameters like
rel_dirandabs_paths.- Improved COCO importer to load all available label types by default.
These changes provide more flexibility in dataset handling and improve interoperability with other formats and tools.
Line range hint
38-48: App enhancements in v0.23.8Key App improvements:
- Support for visualizing clips views.
- New date and datetime field rendering and filtering.
- Improved error handling and display of stack traces.
These updates enhance the user experience and provide more robust data visualization and filtering capabilities in the App.
Line range hint
50-61: Brain and Zoo updates in v0.23.8Significant updates:
- New Brain methods for finding unique and duplicate images or objects.
- Addition of CLIP and DINOv2 models to the Model Zoo.
- Support for applying Segment Anything models to video frames.
These additions expand FiftyOne's capabilities in image analysis and provide access to state-of-the-art models for various tasks.
Line range hint
63-73: Documentation improvements in v0.23.8Notable documentation updates:
- New tutorials on clustering, small object detection, and monocular depth estimation.
- Updated popular tutorials for better clarity and relevance.
- New cheat sheets for pandas comparison, FiftyOne terminology, view stages, and filtering.
These documentation improvements make it easier for users to learn and effectively use FiftyOne's features.
Line range hint
1042-1062: Major updates in v0.23.0This release introduces several significant features and integrations:
New integrations for native text and image searches:
- Redis integration
- MongoDB integration
- Milvus integration
- LanceDB integration
V7 integration for annotating FiftyOne datasets.
App improvements:
- New Lightning mode for optimized filtering on large datasets
- Support for viewing image groups as a video
Core enhancements:
- Support for storing and visualizing cuboids and rotated bounding boxes
- New aggregations framework for computing aggregate values
These updates significantly expand FiftyOne's capabilities in search, annotation, and data visualization, making it more powerful for large-scale dataset management and analysis.
Line range hint
1321-1340: Breaking changes and significant updates in v0.22.0 and earlier
Breaking changes in v0.22.0:
- Renamed all applicable API components that previously referenced "objects" to use "labels" instead. This affects attributes, classes, and methods related to label selection and manipulation.
Major features and integrations in earlier versions:
- v0.21.0: Added support for querying by arbitrary text prompts in the App.
- v0.20.0: Introduced the Spaces framework and native support for visualizing embeddings in the App.
- v0.19.0: Added support for storing frame labels on video samples.
- v0.18.0: Introduced the Model Zoo with over 70 pretrained models for various tasks.
- v0.17.0: Added support for grouped datasets and point cloud visualization.
These updates represent significant milestones in FiftyOne's development, expanding its capabilities in areas such as video analysis, model integration, and data visualization.
app/packages/core/src/components/Modal/use-looker.ts (2)
106-107: Appropriate use of Recoil'suseSetRecoilStateThe implementation correctly utilizes
useSetRecoilStateto manage themodalLookerstate in Recoil, ensuring that thelookerinstance is properly stored.
108-109: Proper use of modal context withuseModalContextDestructuring
setActiveLookerReffromuseModalContext()allows effective interaction with the modal context.app/packages/looker/src/lookers/video.ts (5)
18-18: ImportingVideoConfigis appropriateThe addition of
VideoConfigto the imports aligns with its usage in the code and ensures type consistency.
322-322: InitializebuffersusinginitialBuffersmethod enhances maintainabilitySetting
buffersin the initial state withthis.initialBuffers(config)encapsulates buffer initialization logic, improving code readability and maintainability.
332-333: Useconstforpanandscaleto signify immutabilityChanging
lettoconstfor thepanandscalevariables is appropriate since their values are not reassigned, enhancing code clarity.
552-558: Proper cleanup when updating sampleReinitializing
this.framesand resettingthis.state.buffersinupdateSampleclears outdated data, ensuring that the new sample loads correctly without residual state from previous samples.
572-576: AddinginitialBuffersmethod improves code modularityIntroducing the
initialBuffersmethod encapsulates the buffer initialization logic, promoting reusability and simplifying future modifications.
| () => foq.subscribe((_, { set }) => set(activeColorEntry, null)), | ||
| [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the implications of the useEffect changes
The useEffect hook has been modified:
- The dependency array is now empty, meaning this effect will only run once on component mount.
- The callback now directly sets
activeColorEntryto null using Recoil'ssetfunction.
While this simplifies the state update logic, consider the following:
- Is it intentional to only reset
activeColorEntryon component mount? - Are there any scenarios where
activeColorEntryneeds to be reset based on other state changes?
If the current behavior is intended, you might want to add a comment explaining why the effect only runs once. If not, consider adding appropriate dependencies to the dependency array.
Additionally, to improve code clarity, you could extract the subscription logic into a custom hook:
const useResetActiveColorEntry = () => {
useEffect(() => foq.subscribe((_, { set }) => set(activeColorEntry, null)), []);
};Then use it in the component:
const ColorFooter: React.FC = () => {
// ... other code ...
useResetActiveColorEntry();
// ... rest of the component ...
};This would make the component's logic more modular and easier to test.
|
|
||
| useEffect(() => { | ||
| if (looker) { | ||
| setActiveLookerRef(looker as fos.Lookers); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unnecessary type assertion
Using as fos.Lookers in setActiveLookerRef(looker as fos.Lookers); may mask potential type safety issues. Since looker is of type L extending fos.Lookers, the cast might be unnecessary. Consider ensuring that looker is correctly typed to eliminate the need for casting.
Adjust the code to avoid the type assertion:
- setActiveLookerRef(looker as fos.Lookers);
+ setActiveLookerRef(looker);📝 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.
| setActiveLookerRef(looker as fos.Lookers); | |
| setActiveLookerRef(looker); |
| useEffect(() => { | ||
| setModalLooker(looker); | ||
| }, [looker, setModalLooker]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider cleaning up modalLooker state on unmount
Currently, modalLooker is set when looker changes, but there is no cleanup when the component unmounts. To prevent potential stale references or memory leaks, consider resetting modalLooker when the component unmounts.
Add a cleanup function to the useEffect:
useEffect(() => {
setModalLooker(looker);
+ return () => {
+ setModalLooker(null);
+ };
}, [looker, setModalLooker]);📝 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.
| useEffect(() => { | |
| setModalLooker(looker); | |
| }, [looker, setModalLooker]); | |
| useEffect(() => { | |
| setModalLooker(looker); | |
| return () => { | |
| setModalLooker(null); | |
| }; | |
| }, [looker, setModalLooker]); |
| useEffect(() => { | ||
| if (looker) { | ||
| setActiveLookerRef(looker as fos.Lookers); | ||
| } | ||
| }, [looker, setActiveLookerRef]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure activeLookerRef is cleaned up on unmount
In the useEffect, you set activeLookerRef when looker is available, but you should also reset it when the component unmounts to avoid holding onto stale references.
Modify the useEffect to include a cleanup function:
useEffect(() => {
if (looker) {
setActiveLookerRef(looker as fos.Lookers);
}
+ return () => {
+ setActiveLookerRef(null);
+ };
}, [looker, setActiveLookerRef]);📝 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.
| useEffect(() => { | |
| if (looker) { | |
| setActiveLookerRef(looker as fos.Lookers); | |
| } | |
| }, [looker, setActiveLookerRef]); | |
| useEffect(() => { | |
| if (looker) { | |
| setActiveLookerRef(looker as fos.Lookers); | |
| } | |
| return () => { | |
| setActiveLookerRef(null); | |
| }; | |
| }, [looker, setActiveLookerRef]); |
| lookerWithReader?.pause(); | ||
| lookerWithReader = this; | ||
| this.state.buffers = [[1, 1]]; | ||
| this.setReader(); |
There was a problem hiding this comment.
Potential race condition with shared lookerWithReader variable
Using the shared variable lookerWithReader may lead to unexpected behavior if multiple VideoLooker instances are active simultaneously. This can cause instances to pause or overwrite each other's readers.
Consider refactoring to avoid shared mutable state across instances. Encapsulating lookerWithReader within the class or managing instance-specific readers can prevent conflicts.
Summary by CodeRabbit
New Features
rerunpanel.Bug Fixes
fiftyone quickstartandfiftyone app launchcommands from launching the App.classesargument when loading specific classes in COCO format.