Adding SAM2 to the Model Zoo! #7
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
|
Clone of the PR voxel51/fiftyone#4671 |
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
1 similar comment
![[development] Korbit - AI Mentor](https://avatars.githubusercontent.com/in/334508?s=40&u=8e7ea115878aacba56242d7ac87807ea98eac709&v=4){kind=avatar}
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
WalkthroughThe changes introduce a new file, Changes
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 as PR comments)
Additionally, you can add Documentation and Community
|
| except Exception as e: | ||
| raise(e) |
There was a problem hiding this comment.
In the load_fiftyone_video_frames function, the code is raising a generic Exception without specifying the type of exception. Consider catching and raising specific exception types to provide more meaningful error messages and handle errors appropriately based on the exception type.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
![development-korbit-ai-mentor[bot]](https://avatars.githubusercontent.com/in/334508?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have reviewed your code and found 1 potential issue.
Summary of Code Review
Fix for Code Execution:
- Replace the generic
Exceptioninload_fiftyone_video_frameswith a more specific exception likeValueErrororRuntimeError.
Fix for Code Health:
- Add a clear error message or log a descriptive error message when catching exceptions in
load_fiftyone_video_frames.
| try: | ||
| images = torch.zeros( | ||
| num_frames, 3, image_size, image_size, dtype=torch.float32 | ||
| ) | ||
| except Exception as e: | ||
| raise(e) |
There was a problem hiding this comment.
In the load_fiftyone_video_frames function, a generic Exception is being raised:
try:
images = torch.zeros(
num_frames, 3, image_size, image_size, dtype=torch.float32
)
except Exception as e:
raise(e)Instead of raising a generic Exception, consider raising a more specific exception like ValueError or RuntimeError with a clear error message. Alternatively, catch the exception, log a descriptive error message, and handle it appropriately.
Chat with Korbit by mentioning @development-korbit-ai-mentor, and give a 👍 or 👎 to help Korbit improve your reviews.
|
Mapping from the Github issue to the LangSmith prompt that generated the issue: None: https://smith.langchain.com/o/c92ee716-8a66-4de0-a133-524e36845f3f/projects/p/bb4140d7-b5d4-4198-8f37-41c91ccecea9/r/6ce5facb-592a-42ea-a945-e2f68cd098cd?poll=true |
|
System configuration at time of review: |
|
@coderabbitai full review |
Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (1)
fiftyone/utils/sam2.py (1)
671-675: Clarify logic for handlingsam2_labels.The logic for handling
sam2_labelscould be more explicit.Consider using:
labels = np.array(keypoint.sam2_labels)[valid_rows] if hasattr(keypoint, "sam2_labels") else np.ones(len(scaled_points))
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
fiftyone/zoo/models/manifest-torch.jsonis excluded by!**/*.json
Files selected for processing (1)
- fiftyone/utils/sam2.py (1 hunks)
Additional comments not posted (6)
fiftyone/utils/sam2.py (6)
59-71: LGTM!The
SegmentAnything2VideoModelConfigclass is well-implemented and straightforward.
713-718: Raise a specific exception instead of a generic one.Raising a specific exception type can provide more meaningful error messages and improve error handling.
Consider using a more specific exception like
RuntimeErrororValueErrorwith a clear error message.
664-666: LGTM!The
_to_sam_inputfunction is well-implemented and straightforward.
679-688: LGTM!The
_to_sam_boxfunction is well-implemented and straightforward.
690-698: LGTM!The
_mask_to_boxfunction is well-implemented and straightforward.
740-746: LGTM!The
_load_video_frames_monkey_patchfunction is well-implemented and straightforward.
| if self.points_mask_index and not 0 <= self.points_mask_index <= 2: | ||
| raise ValueError("mask_index must be 0, 1, or 2") |
There was a problem hiding this comment.
Improve validation logic for points_mask_index.
The current validation logic for points_mask_index could be made more readable by using a clearer conditional expression.
Consider using:
if self.points_mask_index not in {0, 1, 2}:
raise ValueError("mask_index must be 0, 1, or 2")| raise ValueError( | ||
| "Unsupported prompt type %s. The supported field types are %s" | ||
| % (type(value), (fol.Detections, fol.Keypoints)) | ||
| ) |
There was a problem hiding this comment.
Enhance error message for unsupported prompt types.
The error message could be more descriptive to aid debugging.
Consider including the field name in the error message:
raise ValueError(
f"Unsupported prompt type {type(value)} in field '{field_name}'. Supported types are {fol.Detections, fol.Keypoints}."
)| raise ValueError( | ||
| "Sample %s is missing a prompt in field '%s'" | ||
| % (sample.id, field_name) | ||
| ) |
There was a problem hiding this comment.
Improve error message for missing prompts.
The error message could be more informative by including the field name.
Consider using:
raise ValueError(
f"Sample {sample.id} is missing a prompt in field '{field_name}'."
)| raise ValueError( | ||
| "'prompt_field' should be a frame field for segment anything 2 video model" | ||
| ) |
There was a problem hiding this comment.
Clarify error message for invalid prompt_field.
The error message should clearly indicate the expected format for prompt_field.
Consider using:
raise ValueError(
f"'prompt_field' must be a frame field and start with 'frames.' for the Segment Anything 2 video model."
)| raise AttributeError( | ||
| "Missing required argument 'prompt_field' for segment anything 2 video model" | ||
| ) |
There was a problem hiding this comment.
Enhance error message for missing prompt_field.
The error message should be more descriptive to aid debugging.
Consider using:
raise AttributeError(
"The 'prompt_field' argument is required for the Segment Anything 2 video model but is missing."
)
|
/review |
PR Reviewer Guide 🔍
|
|
/review |
What changes are proposed in this pull request?
Adding Segment Anything 2 to the Fiftyone model zoo.
How is this patch tested? If it is not, please explain why.
Tested manually with different configurations -
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Added SAM2 into the Fiftyone model zoo with inference support for both images and videos.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesBox prompt for Images
Keypoint prompt for Images
Automatic segmentation for Images
Prompting for Videos
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Description by Korbit AI
What change is being made?
Add support for Segment Anything Model 2 (SAM2) to the FiftyOne Model Zoo, including both image and video segmentation models.
Why are these changes being made?
These changes integrate the latest SAM2 models from Meta AI into the FiftyOne ecosystem, enabling users to leverage state-of-the-art segmentation capabilities for both images and videos. This addition enhances the model zoo's offerings and provides users with more advanced tools for their segmentation tasks.