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

Update for SAM 2.1 #10

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Update for SAM 2.1 #10

wants to merge 3 commits into from

Conversation

drien
Copy link

@drien drien commented Dec 29, 2024

This brings in the code changes and configs for the 2.1 updates directly from the original sam2 repo. Everything seems to work as expected with both of the 2 and 2.1 models. I just updated the variants to have a version prefix so there aren't breaking changes for users who've already integrated this with the 2.0 version.

I duplicated some of the tests to run with the 2.1 models as well and everything passes with both versions, but I've only done real-world testing using the 2.1 checkpoints.

@drien
Copy link
Author

drien commented Dec 29, 2024

Also I've added in a fix for a mistaken dependency on pytest–since the download_weights() method was imported to the root util module, it was then imported when running from sam2 import load_model, so installing in an environment without pytest would crash with an ImportError. I've just removed that import and made the tests import directly from sam2.utils.download.

Reproduce:

$ docker run --rm -it python:3.11-bullseye  bash -il
# pip install samv2
...
# python
>>> from sam2 import load_model

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.11/site-packages/sam2/__init__.py", line 9, in <module>
    from .build_sam import load_model
  File "/usr/local/lib/python3.11/site-packages/sam2/build_sam.py", line 14, in <module>
    from .utils.misc import VARIANTS, variant_to_config_mapping
  File "/usr/local/lib/python3.11/site-packages/sam2/utils/__init__.py", line 7, in <module>
    from .download import download_weights
  File "/usr/local/lib/python3.11/site-packages/sam2/utils/download.py", line 5, in <module>
    import pytest
ModuleNotFoundError: No module named 'pytest'

Test Fix:

$ docker run --rm -it -v .:/opt/samv2 python:3.11-bullseye  bash -il
# pip install -e .
...
# python
>>> from sam2 import load_model
>>> 
>>> exit()
# pip3 install pytest
...
# pytest
=========================================================================================== test session starts ============================================================================================
platform linux -- Python 3.11.11, pytest-8.3.4, pluggy-1.5.0
rootdir: /opt/samv2
configfile: pyproject.toml
testpaths: tests
plugins: hydra-core-1.3.2
collected 15 items                                                                                                                                                                                         

tests/test_build_model.py ..^C

@SauravMaheshkar SauravMaheshkar added the feature 🚀 New feature or request label Dec 30, 2024
@SauravMaheshkar SauravMaheshkar self-requested a review December 30, 2024 19:03
@SauravMaheshkar SauravMaheshkar added the documentation 📚 Improvements or additions to documentation label Dec 30, 2024
Copy link
Owner

Choose a reason for hiding this comment

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

Are these new functions used somewhere? This fork was created with the aim to be a minimal version of the original codebase.

@@ -245,9 +240,7 @@ def forward(self, q: Tensor, k: Tensor, v: Tensor) -> Tensor:

dropout_p = self.dropout_p if self.training else 0.0
# Attention

with torch.nn.attention.sdpa_kernel(get_sdp_backends(dropout_p)):
Copy link
Owner

Choose a reason for hiding this comment

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

Any particular reason for removing this ? I created the get_sdp_backends function to support the newer backends if available on the system being used.

Copy link
Owner

Choose a reason for hiding this comment

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

Huh this is interesting, I used ruff initially for sorting imports, it shouldn't re-organise like this.

self.only_obj_ptrs_in_the_past_for_eval = only_obj_ptrs_in_the_past_for_eval

# Part 2: memory attention to condition current frame's visual features
# with memories (and obj ptrs) from past frames
self.memory_attention = memory_attention
self.hidden_dim = memory_attention.d_model
self.hidden_dim = image_encoder.neck.d_model
Copy link
Owner

Choose a reason for hiding this comment

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

maybe I'm being pedantic but just to confirm, this runs okay during forward inference? I don't see a .neck submodule.

Copy link
Owner

@SauravMaheshkar SauravMaheshkar left a comment

Choose a reason for hiding this comment

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

Hey @drien thanks a lot for contributing ❤️ . Left a couple of review notes ☕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📚 Improvements or additions to documentation feature 🚀 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants