Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add segment-anything models (SAM) to model zoo #3019

Merged
merged 6 commits into from
Jul 18, 2023

Conversation

Rusteam
Copy link
Contributor

@Rusteam Rusteam commented May 12, 2023

What changes are proposed in this pull request?

Add three segment-anything models to the model zoo.

How is this patch tested? If it is not, please explain why.

Added an extra intensive model test for sam backbone.

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release
    notes for FiftyOne users.

Segment-anything model is now available in the model zoo. It can perform automatic mask generation and guided mask generation with prompts.

What areas of FiftyOne does this PR affect?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Patch coverage: 73.29% and project coverage change: -13.63 ⚠️

Comparison is base (94b3988) 62.24% compared to head (cd51f71) 48.61%.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #3019       +/-   ##
============================================
- Coverage    62.24%   48.61%   -13.63%     
============================================
  Files          250      227       -23     
  Lines        45578    33359    -12219     
  Branches       319      319               
============================================
- Hits         28368    16217    -12151     
+ Misses       17210    17142       -68     
Flag Coverage Δ
app 48.61% <73.29%> (-0.01%) ⬇️
python ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/packages/state/src/hooks/useSetView.ts 18.32% <0.00%> (ø)
app/packages/state/src/routing/RouterContext.ts 41.53% <50.00%> (ø)
app/packages/state/src/recoil/groups.ts 48.59% <58.33%> (ø)
app/packages/state/src/hooks/useSchemaSettings.ts 83.01% <72.95%> (ø)
..._generated__/searchSelectFieldsMutation.graphql.ts 100.00% <100.00%> (ø)
...packages/relay/src/mutations/searchSelectFields.ts 100.00% <100.00%> (ø)

... and 24 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Rusteam
Copy link
Contributor Author

Rusteam commented May 19, 2023

@brimoor Need your input on the following issue. I'm following this tutorial to add SAM to the model zoo, so far AMG mode worked fine. In order to implement prompting SAM with points and boxes I want to let users specify keypoint/detection label field while calling dataset.apply_model(..., points_from=<keypoint_label_field>, boxes_from=None) and then pull up points and bounding boxes from a sample collection. However in order to achieve that, I have to parse points and boxes before the model.predict_all() method here and then pass them as parameters to that call. This should be backward-compatible but looks like a major change. What do you think is the best scenario to achieve that?

@timmermansjoy
Copy link
Contributor

also a idea and idk if it would bloat this PR. but if you would integrate groundingDINO first you would also add a parameter of "objects" or so and then just prompt the model for objects.

@Rusteam
Copy link
Contributor Author

Rusteam commented May 23, 2023

Thanks for the suggestion @timmermansjoy. I am myself not familiar with grounding Dino and that's why I want to limit the scope to segment-anything for the moment. After it is integrated, we can think about adding grounding Dino.

@brimoor
Copy link
Contributor

brimoor commented May 24, 2023

@Rusteam hmm, I see what you mean. Perhaps there should be a way for a Model instance to declare that it needs the sample objects to be supplied along with the images during inference?

prediction = model.predict(img, sample)
predictions = model.predict_all(imgs, samples)

One stylistic way to achieve this would be to implement a SampleMixin class (named similarly to PromptMixin and EmbeddingsMixin).

Note that this line:

samples = samples.select_fields()

will need to be excluded for models that inherit from SampleMixin, as they'll need access to all fields in principle (the select_fields() was added to minimize database read bandwidth, since, until now, inference never required anything beyond the image pixels).

Alternatively there could be a SampleMixin.needs_fields property that returns a list of sample fields that should be included:

if isinstance(model, SampleMixin):
    samples = samples.select_fields(model.needs_fields)
else:
    samples = samples.select_fields()

...

if isinstance(model, SampleMixin):
    predictions_batch = model.predict_all(imgs_batch, samples_batch)
else:
    predictions_batch = model.predict_all(imgs_batch)

@Rusteam
Copy link
Contributor Author

Rusteam commented May 25, 2023

I've slightly changed the behavior of Keypoints from the @jacobmarks blogpost. In this implementation, each Keypoint object will be processed individually with all points passed as positive labels (and no background labels). Also a user can select which mask (0,1, or 2) to use. I guess this requires a short tutorial, which I can work on after we complete this integration.

@Rusteam Rusteam changed the title [WIP] Add segment-anything models (SAM) to model zoo Add segment-anything models (SAM) to model zoo May 25, 2023
@Rusteam
Copy link
Contributor Author

Rusteam commented Jun 1, 2023

@brimoor hi, this is now ready for testing and integration.

@Rusteam
Copy link
Contributor Author

Rusteam commented Jul 13, 2023

@brimoor hey any updates?

@brimoor brimoor self-requested a review July 18, 2023 04:07
@brimoor brimoor self-assigned this Jul 18, 2023
@brimoor brimoor added the feature Work on a feature request label Jul 18, 2023
@brimoor brimoor changed the base branch from develop to feature/sam July 18, 2023 15:02
Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rusteam LGTM (sorry for the delay! 😅)

I'm going to merge this into a feature branch and add a couple additional enhancements. Then we'll merge into develop and it will be included in the next release 📈

@brimoor brimoor merged commit 39eefc3 into voxel51:feature/sam Jul 18, 2023
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Work on a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants