🚨🚨🚨 Fix sdpa in sam and refactor relative position embeddings #36422

geetu040 · 2025-02-26T12:41:49Z

What does this PR do?

This PR

fixes SamVisionSdpaAttention when output_attentions=True, fall back to eager implementation
refactors position embeddings in both attention layers to a single function
fixes tensorflow related tests: runs/13544413658
reflects these changes in TFSamVisionSdpaAttention and GotOcr2VisionAttention

Previously discussed here: #36248 (comment)

🚨 Breaking changes

add_decomposed_rel_pos public method of *Attention module was replaced with get_decomposed_rel_pos method, which will return pos embedding instead of adding it to the attention weights

Who can review?

@amyeroberts, @qubvel, @zucchini-nlp

qubvel · 2025-02-26T12:57:28Z

Hi @geetu040, thanks for opening the PR!

qubvel · 2025-02-26T12:57:36Z

run-slow: sam

github-actions · 2025-02-26T12:59:02Z

This comment contains run-slow, running the specified jobs: This comment contains run-slow, running the specified jobs:

models: ['models/sam']
quantizations: [] ...

geetu040 · 2025-03-03T08:33:35Z

run-slow: sam

geetu040 · 2025-03-03T08:37:46Z

Hi @qubvel , I have fixed the failing tests at runs/13544413658. This PR is ready for review now and can you please run slow tests again?

qubvel

Thanks for fixing!

run-slow: sam

A question:

qubvel · 2025-03-03T14:03:21Z

src/transformers/models/sam/image_processing_sam.py

+        counts += (
+            [cur_idxs[0].numpy().item()] + btw_idxs.numpy().tolist() + [height * width - cur_idxs[-1].numpy().item()]
+        )


Why do we need ot add .numpy()?

because tensorflow tensors don't have a direct item() method and to access the scalar value, we need to do numpy().item()

ok! just wondering if it was broken before

there are a bunch of other tensorflow bugs that I don't why didnot appear in the workflows before, except in #36493, and I have checked for different version of tensorflow they seem to be geniune bugs over all versions.

tf tensor in kwarg_value

tf.tensor instead of tf.constant/tf.Tensor

boolean indexing in tf.Tensor

tf.Tensor.item()

ok, not sure if there any usage of TF Sam actually, so there might be some bugs indeed!

qubvel · 2025-03-03T14:29:57Z

run-slow: sam

github-actions · 2025-03-03T14:31:15Z

This comment contains run-slow, running the specified jobs: This comment contains run-slow, running the specified jobs:

models: ['models/sam']
quantizations: [] ...

geetu040 · 2025-03-03T14:41:58Z

@qubvel the custom tests have passed now, the failing ones seem unrelated

qubvel · 2025-03-03T15:01:51Z

Nice! Thanks for fixing

qubvel · 2025-03-03T15:02:47Z

Lets wait for those tests being fixed on main, and then merge it

qubvel · 2025-03-04T09:19:30Z

cc @ArthurZucker for approval: changing add_decomposed_rel_pos to get_decomposed_rel_pos, which is a public method of a module

geetu040 · 2025-03-06T13:51:51Z

@ArthurZucker a soft ping, since this blocks the failing tests in other PRs: #36248, #36493

geetu040 · 2025-03-12T14:49:18Z

Hi @qubvel, would it be possible to ping someone else for this? It seems that @ArthurZucker is quite busy and might not have had a chance to review it.

Since this is blocking other PRs (#36248 and #36493), it would be great if we could get it merged. Otherwise, if this will take some time, I can merge this branch into the other PRs and continue working from there.

qubvel · 2025-03-13T17:16:17Z

cc @molbap or @zucchini-nlp if you have bandwidth

zucchini-nlp

This is breaking, no? We had add_decomposed_rel_pos as public method and now we're removing it completely

I am oke to break, don't think anyone actually was calling it separately. But we can add a 🔴 in PR title

zucchini-nlp · 2025-03-13T22:00:33Z

src/transformers/processing_utils.py

                else:
                    kwarg_value = "__empty__"
-                if kwarg_value != "__empty__":
+                if not isinstance(kwarg_value, str) or kwarg_value != "__empty__":


wonder, why this was needed? Even if kwargs_value is float, checking kwarg_value != "__empty__" is enough isn't it

This change was necessary because kwarg_value cannot be directly compared to "__empty__" if it is a TensorFlow tensor. Attempting such a comparison results in a TypeError:

tf.Variable([1, 2, 3, 4]) == "hello" # TypeError: Cannot convert '__empty__' to EagerTensor of dtype int32

This issue occurred in the test suite test_modeling_tf_sam.py::TFSamModelIntegrationTest (slow tests for sam) within this workflow

interesting, thanks for explanation

zucchini-nlp

Oh yeah, sorry, meant to hit approve! LGTM as long as qubvel is okey with the changes

qubvel · 2025-03-17T09:38:52Z

Added a section in PR initial message with breaking change description, merging

…gface#36422) * fall back to eager if output_attentions * improve relative position embeddings * run modular on got_ocr2 * run-slow: sam * fix run-length encoding * fix tf processor errors * update tf_sam * fix compile error * re-run tests Signed-off-by: Mehant Kammakomati <[email protected]>

geetu040 added 3 commits February 26, 2025 15:54

fall back to eager if output_attentions

dea7503

improve relative position embeddings

400e948

run modular on got_ocr2

c8d5f1c

qubvel added the Vision label Feb 26, 2025

geetu040 added 2 commits March 1, 2025 17:03

Merge branch 'main' into fix-sam-sdpa

7db2903

run-slow: sam

82e1b9f

geetu040 force-pushed the fix-sam-sdpa branch from eaa89cd to 82e1b9f Compare March 1, 2025 12:14

geetu040 added 6 commits March 2, 2025 06:37

fix run-length encoding

f5f2ac5

fix tf processor errors

41c191f

update tf_sam

1c99b6c

fix compile error

99fc746

Merge branch 'main' into fix-sam-sdpa

bf57971

re-run tests

14b98a7

qubvel reviewed Mar 3, 2025

View reviewed changes

qubvel approved these changes Mar 3, 2025

View reviewed changes

Merge branch 'main' into fix-sam-sdpa

622c992

geetu040 mentioned this pull request Mar 4, 2025

Create and Expose SamVisionModel as public for better accessibility #36493

Merged

4 tasks

qubvel requested a review from ArthurZucker March 4, 2025 09:15

zucchini-nlp reviewed Mar 13, 2025

View reviewed changes

geetu040 changed the title ~~Fix sdpa in sam and refactor relative position embeddings~~ 🔴 Fix sdpa in sam and refactor relative position embeddings Mar 14, 2025

Merge branch 'main' into fix-sam-sdpa

df2e4a6

geetu040 requested a review from zucchini-nlp March 17, 2025 00:33

zucchini-nlp approved these changes Mar 17, 2025

View reviewed changes

qubvel changed the title ~~🔴 Fix sdpa in sam and refactor relative position embeddings~~ 🚨🚨🚨 Fix sdpa in sam and refactor relative position embeddings Mar 17, 2025

qubvel merged commit c53d53d into huggingface:main Mar 17, 2025
21 checks passed

🚨🚨🚨 Fix sdpa in sam and refactor relative position embeddings #36422

🚨🚨🚨 Fix sdpa in sam and refactor relative position embeddings #36422

Uh oh!

Conversation

geetu040 commented Feb 26, 2025 • edited by qubvel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

🚨 Breaking changes

Who can review?

Uh oh!

qubvel commented Feb 26, 2025

Uh oh!

qubvel commented Feb 26, 2025

Uh oh!

github-actions bot commented Feb 26, 2025

Uh oh!

geetu040 commented Mar 3, 2025

Uh oh!

geetu040 commented Mar 3, 2025

Uh oh!

qubvel left a comment

Choose a reason for hiding this comment

Uh oh!

qubvel Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

geetu040 Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

qubvel Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

geetu040 Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

qubvel Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qubvel commented Mar 3, 2025

Uh oh!

github-actions bot commented Mar 3, 2025

Uh oh!

geetu040 commented Mar 3, 2025

Uh oh!

qubvel commented Mar 3, 2025

Uh oh!

qubvel commented Mar 3, 2025

Uh oh!

qubvel commented Mar 4, 2025

Uh oh!

geetu040 commented Mar 6, 2025

Uh oh!

geetu040 commented Mar 12, 2025

Uh oh!

qubvel commented Mar 13, 2025

Uh oh!

zucchini-nlp left a comment

Choose a reason for hiding this comment

Uh oh!

zucchini-nlp Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

geetu040 Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

zucchini-nlp Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

zucchini-nlp left a comment

Choose a reason for hiding this comment

Uh oh!

qubvel commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

geetu040 commented Feb 26, 2025 •

edited by qubvel

Loading

qubvel Mar 3, 2025 •

edited

Loading

qubvel commented Mar 17, 2025 •

edited

Loading