adding positional encoder changes and tests #32600

manuelsh · 2024-08-11T17:25:28Z

@amyeroberts as there were some conflicts with merging with main on #31900 (possibly due to the make scripts), I have reimplemented all the changes of #30783 in a new branch, which is rebased to main.

manuelsh · 2024-08-12T16:56:18Z

@amyeroberts I have included the interpolation of positional embeddings in all the following models, and their respective tests:

altclip
bridgetower
chineseclip
clip
clipseg
kosmos_2
x_clip
git

Thanks!

amyeroberts

Thanks for adding - looks great!

Just a handful of small nits. Before merge, we'll need to run the slow tests for the models affected. Could you trigger this by running git commit --allow-empty -m "[run_slow] altclip, bridgetower, chinese_clip, clip, clipseg, git, kosmos2, x_clip"

amyeroberts · 2024-08-14T11:36:05Z

tests/models/x_clip/test_modeling_x_clip.py

+                model(**inputs, interpolate_pos_encoding=False)
+        # forward pass
+        with torch.no_grad():
+            outputs = model(**inputs, interpolate_pos_encoding=True)


amyeroberts · 2024-08-14T11:36:50Z

tests/models/git/test_modeling_git.py

+    @unittest.skip(reason="GitForCausalLM does not support inputs_embeds in generate method")
+    def test_inputs_embeds_matches_input_ids_with_generate(self):
+        pass
+


Let's remove this as this logic is independent of this PR

Suggested change

@unittest.skip(reason="GitForCausalLM does not support inputs_embeds in generate method")

def test_inputs_embeds_matches_input_ids_with_generate(self):

pass

If I remove this, I will get the following error from the CI pipeline:

FAILED tests/models/git/test_modeling_git.py::GitModelTest::test_inputs_embeds_matches_input_ids_with_generate - ValueError: You passed `inputs_embeds` to `.generate()`, but the model class GitForCausalLM doesn't have its forwarding implemented. See the GPT2 implementation for an example (https://github.com/huggingface/transformers/pull/21405), and feel free to open a PR with it!

as shown here

Could you rebase on main? I believe this has been resolved upstream

This should still be removed as this tests is unrelated to this PR

amyeroberts · 2024-08-14T11:39:33Z

src/transformers/models/clip/modeling_clip.py

        return_dict (`bool`, *optional*):
            Whether or not to return a [`~utils.ModelOutput`] instead of a plain tuple.
+


Suggested change

amyeroberts · 2024-08-14T11:39:59Z

src/transformers/models/clipseg/modeling_clipseg.py

@@ -512,6 +526,8 @@ def _init_weights(self, module):
        output_hidden_states (`bool`, *optional*):
            Whether or not to return the hidden states of all layers. See `hidden_states` under returned tensors for
            more detail.
+        interpolate_pos_encoding (`bool`, *optional*):


Suggested change

interpolate_pos_encoding (`bool`, *optional*):

interpolate_pos_encoding (`bool`, *optional*, defaults to `False`):

amyeroberts · 2024-08-14T11:40:07Z

src/transformers/models/clipseg/modeling_clipseg.py

@@ -549,6 +565,8 @@ def _init_weights(self, module):
        output_hidden_states (`bool`, *optional*):
            Whether or not to return the hidden states of all layers. See `hidden_states` under returned tensors for
            more detail.
+        interpolate_pos_encoding (`bool`, *optional*):


Suggested change

interpolate_pos_encoding (`bool`, *optional*):

interpolate_pos_encoding (`bool`, *optional*, defaults to `False`):

…smos2, x_clip

manuelsh · 2024-08-15T22:47:06Z

OK, I've:

added your three suggestions (thanks!)
I haven't removed the @unittest.skip(reason="GitForCausalLM does not support inputs_embeds in generate method")... lines, as per my comment, please let me know
I have run the slow tests with the git command sent. However, I don't think it is running the right slow tests as I just detected some errors on them which I am fixing now (for example in the "bridgetower" one)

Please don't merge yet, just need some time to check and potentially fix the tests.

manuelsh · 2024-08-16T20:37:53Z

GIT model test still requires to be fixed. Getting this error:

tests.models.git.test_modeling_git.GitModelIntegrationTest.test_inference_interpolate_pos_encoding failed with error: <class 'IndexError'> index out of range in self
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/local/lib/python3.10/unittest/case.py", line 591, in run
    self._callTestMethod(testMethod)
  File "/usr/local/lib/python3.10/unittest/case.py", line 549, in _callTestMethod
    method()
  File "/usr/src/app/transformers/tests/models/git/test_modeling_git.py", line 588, in test_inference_interpolate_pos_encoding
    outputs = model(**inputs, interpolate_pos_encoding=True)
  File "/usr/local/lib/python3.10/site-packages/torch/nn/modules/module.py", line 1553, in _wrapped_call_impl
    return self._call_impl(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/torch/nn/modules/module.py", line 1562, in _call_impl
    return forward_call(*args, **kwargs)
  File "/usr/src/app/transformers/src/transformers/models/git/modeling_git.py", line 1302, in forward
    embedding_output = self.embeddings(
  File "/usr/local/lib/python3.10/site-packages/torch/nn/modules/module.py", line 1553, in _wrapped_call_impl
    return self._call_impl(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/torch/nn/modules/module.py", line 1562, in _call_impl
    return forward_call(*args, **kwargs)
  File "/usr/src/app/transformers/src/transformers/models/git/modeling_git.py", line 115, in forward
    embeddings = self.word_embeddings(input_ids)
  File "/usr/local/lib/python3.10/site-packages/torch/nn/modules/module.py", line 1553, in _wrapped_call_impl
    return self._call_impl(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/torch/nn/modules/module.py", line 1562, in _call_impl
    return forward_call(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/torch/nn/modules/sparse.py", line 164, in forward
    return F.embedding(
  File "/usr/local/lib/python3.10/site-packages/torch/nn/functional.py", line 2267, in embedding
    return torch.embedding(weight, input, padding_idx, scale_grad_by_freq, sparse)
IndexError: index out of range in self

due to input_ids having values out of range (tensor([[49406, 768, 568, 530, 518, 2867, 49407]], dtype=torch.int32)). In concrete 49406 and 49407 are not accepted. Not sure why the processor is adding them.

Still on it.

amyeroberts · 2024-08-19T10:49:53Z

@manuelsh Have you included the most recent updates from main?

manuelsh · 2024-08-20T23:16:45Z

Did it and still getting the same error. These two tokens (49406 and 49407) are special tokens added by the processor, they are <|startoftext|> and <|endoftext|>. I can also see that the word_embeddings tensor has a dimension of Embedding(30522, 768, padding_idx=0), i.e. a vocabulary of 30522.

I will do further debugging once I find time. In the meantime any suggestion is appreciated.

manuelsh · 2024-08-21T18:54:44Z

@amyeroberts I found the source of the issue: the pre-trained model for GIT needed to be updated to the correct one. I think this should make it!

However, now I am getting the following integration error coming from feature_extraction_audio_spectrogram_transformer, even if I've synced the branch with the latest changes. I don't get anything related to this when I do make fixup or make repo-consistency.

Traceback (most recent call last):
  File "/root/transformers/utils/check_repo.py", line 1198, in <module>
    check_repo_quality()
  File "/root/transformers/utils/check_repo.py", line 1186, in check_repo_quality
    check_all_auto_object_names_being_defined()
  File "/root/transformers/utils/check_repo.py", line 742, in check_all_auto_object_names_being_defined
    if not hasattr(transformers, class_name):
  File "/root/transformers/src/transformers/utils/import_utils.py", line 1631, in __getattr__
    value = getattr(module, name)
  File "/root/transformers/src/transformers/utils/import_utils.py", line 1630, in __getattr__
    module = self._get_module(self._class_to_module[name])
  File "/root/transformers/src/transformers/utils/import_utils.py", line 1642, in _get_module
    raise RuntimeError(
RuntimeError: Failed to import transformers.models.audio_spectrogram_transformer.feature_extraction_audio_spectrogram_transformer because of the following error (look up to see its traceback):
libtorch_cuda.so: cannot open shared object file: No such file or directory

Exited with code exit status 1

manuelsh · 2024-08-31T15:04:45Z

Hi @amyeroberts I update main again with all changes and now it seems that all tests are passed, so it's ready to merge!

HuggingFaceDocBuilderDev · 2024-09-02T15:57:04Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

amyeroberts · 2024-09-03T13:56:00Z

@manuelsh Great! As there's been a few changes since the last slow run, we'll need to do another git commit --allow-empty -m "[run_slow] altclip, bridgetower, chinese_clip, clip, clipseg, git, kosmos2, x_clip". Once those are all passing we're good to merge!

…smos2, x_clip

…here is no vision_model_output

…smos2, x_clip

manuelsh · 2024-09-05T21:55:17Z

@amyeroberts I did a couple of fixes (one in another non related test, test_inference_image_segmentation in clipseg and another in GIT) and now all tests run.

manuelsh · 2024-09-11T16:40:04Z

Hi @amyeroberts , I wonder if there is something missing or we can merge it.

amyeroberts · 2024-09-12T15:53:00Z

@manuelsh Thanks for all the work so far on this! Yes, there's a final iteration we'll need to do -- otherwise the code all looks good.

Last week we merged in #33226. This fixed an issue in a lot of our vision models, which were using scale_factor in the nn.functional.interpolate call instead of size. This is in part needed to enable exporting to onnx and hence making the models compatible with transformers.js.

Could you update the interpolate functions to use this updated logic flow? The tests shouldn't be affected.

…smos2, x_clip

manuelsh · 2024-09-15T14:13:33Z

@amyeroberts done, all interpolate_pos_encoding functions updated.

amyeroberts

Beautiful - thanks for adding this capability to our models and for iterating on a solution!

amyeroberts · 2024-09-16T18:58:44Z

@manuelsh Just the failing slow tests to address!

manuelsh · 2024-09-16T22:30:48Z

@amyeroberts , I think it is not just substituting interpolate_pos_encoding function, but one needs to adapt it, as the position_embeddings tensor from #33226 is different from the position_embedding object in the code of this PR (note the s).

I believe I can make them work with

self.position_embeddings = self.position_embedding.weight.unsqueeze(0)

but now all my tests are crashing for different reasons (different tensors outputs for example) and this will take longer.

Why not getting back to the previous working commit (d44e070), merge it, and then open another PR like #33226 but for the clip family models?

I would be happy to contribute to it.

manuelsh · 2024-09-18T19:47:24Z

@amyeroberts I was able to fix all tests with the new function, so no need to do an additional PR. Please review.

Manuel Sanchez Hernandez added 5 commits August 11, 2024 19:21

adding positional encoder changes and tests

63e8a34

adding ruff suggestions

bf6ddf2

changes added by python utils/check_copies.py --fix_and_overwrite

c1e5058

removing pos_encoding added by script

19aaa92

adding interpolation to clipseg

b282796

manuelsh marked this pull request as ready for review August 11, 2024 23:53

Manuel Sanchez Hernandez added 3 commits August 12, 2024 17:50

formatting

14d6001

adding further testing to altclip and better documentation to kosmos2

48128b1

skipping test_inputs_embeds_matches_input_ids_with_generate in git model

8eb1beb

This was referenced Aug 13, 2024

Interpolate clip #31900

Closed

fixes clip interpolate #30783

Closed

amyeroberts added the run-slow label Aug 14, 2024

amyeroberts reviewed Aug 14, 2024

View reviewed changes

Manuel Sanchez Hernandez added 2 commits August 15, 2024 23:58

fixing clipseg comment suggestions

7ced086

[run_slow] altclip, bridgetower, chinese_clip, clip, clipseg, git, ko…

cac7886

…smos2, x_clip

Manuel Sanchez Hernandez added 5 commits August 16, 2024 00:47

fixing bridgetower test

a17b554

fixing altclip tensor output POS test

c4e56fb

adding ruff formatting

e303547

fixing several tests

ee8318d

formatting with ruff

20778a3

manuelsh and others added 6 commits August 21, 2024 00:48

Merge branch 'huggingface:main' into interpolate-clip-b

da9108a

adding positional encoder changes and tests

024ea6e

adding ruff suggestions

9c645e3

changes added by python utils/check_copies.py --fix_and_overwrite

19ad494

removing pos_encoding added by script

b383517

adding interpolation to clipseg

578411c

Manuel Sanchez Hernandez added 2 commits August 21, 2024 00:50

fixing several tests

ca9682d

formatting with ruff

8567408

Manuel Sanchez Hernandez and others added 3 commits August 21, 2024 20:28

adding right pretrained model

9941dbd

adding correct pretrained model to git

962989d

Merge branch 'huggingface:main' into interpolate-clip-b

09301c5

Merge branch 'huggingface:main' into interpolate-clip-b

b70ab52

Manuel Sanchez Hernandez added 7 commits September 3, 2024 15:29

[run_slow] altclip, bridgetower, chinese_clip, clip, clipseg, git, ko…

9d05572

…smos2, x_clip

fixing test_inference_image_segmentation

16363f6

[run_slow] altclip, bridgetower, chinese_clip, clip, clipseg, git, ko…

e3e2272

…smos2, x_clip

fixing test_inference_interpolate_pos_encoding for the git model as t…

e35729a

…here is no vision_model_output

[run_slow] altclip, bridgetower, chinese_clip, clip, clipseg, git, ko…

58a02f1

…smos2, x_clip

adding ruff formatting

fcbf2d2

[run_slow] altclip, bridgetower, chinese_clip, clip, clipseg, git, ko…

d44e070

…smos2, x_clip

Manuel Sanchez Hernandez added 2 commits September 15, 2024 16:09

adding new interpolate_pos_encoding function

ea54d25

[run_slow] altclip, bridgetower, chinese_clip, clip, clipseg, git, ko…

9d751a6

…smos2, x_clip

amyeroberts approved these changes Sep 16, 2024

View reviewed changes

Manuel Sanchez Hernandez added 2 commits September 18, 2024 15:36

fixing interpolate_POS funciton

f36537b

adapting output tensor in teests

4170cba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding positional encoder changes and tests #32600

adding positional encoder changes and tests #32600

manuelsh commented Aug 11, 2024

manuelsh commented Aug 12, 2024 •

edited

Loading

amyeroberts left a comment

amyeroberts Aug 14, 2024

amyeroberts Aug 14, 2024

manuelsh Aug 15, 2024

amyeroberts Aug 16, 2024

amyeroberts Aug 22, 2024

amyeroberts Aug 14, 2024

amyeroberts Aug 14, 2024

amyeroberts Aug 14, 2024

manuelsh commented Aug 15, 2024 •

edited

Loading

manuelsh commented Aug 16, 2024

amyeroberts commented Aug 19, 2024

manuelsh commented Aug 20, 2024 •

edited

Loading

manuelsh commented Aug 21, 2024

manuelsh commented Aug 31, 2024

HuggingFaceDocBuilderDev commented Sep 2, 2024

amyeroberts commented Sep 3, 2024

manuelsh commented Sep 5, 2024

manuelsh commented Sep 11, 2024

amyeroberts commented Sep 12, 2024

manuelsh commented Sep 15, 2024

amyeroberts left a comment

amyeroberts commented Sep 16, 2024

manuelsh commented Sep 16, 2024 •

edited

Loading

manuelsh commented Sep 18, 2024

	@unittest.skip(reason="GitForCausalLM does not support inputs_embeds in generate method")
	def test_inputs_embeds_matches_input_ids_with_generate(self):
	pass

		return_dict (`bool`, optional):
		Whether or not to return a [`~utils.ModelOutput`] instead of a plain tuple.

	interpolate_pos_encoding (`bool`, optional):
	interpolate_pos_encoding (`bool`, optional, defaults to `False`):

adding positional encoder changes and tests #32600

Are you sure you want to change the base?

adding positional encoder changes and tests #32600

Conversation

manuelsh commented Aug 11, 2024

manuelsh commented Aug 12, 2024 • edited Loading

amyeroberts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelsh commented Aug 15, 2024 • edited Loading

manuelsh commented Aug 16, 2024

amyeroberts commented Aug 19, 2024

manuelsh commented Aug 20, 2024 • edited Loading

manuelsh commented Aug 21, 2024

manuelsh commented Aug 31, 2024

HuggingFaceDocBuilderDev commented Sep 2, 2024

amyeroberts commented Sep 3, 2024

manuelsh commented Sep 5, 2024

manuelsh commented Sep 11, 2024

amyeroberts commented Sep 12, 2024

manuelsh commented Sep 15, 2024

amyeroberts left a comment

Choose a reason for hiding this comment

amyeroberts commented Sep 16, 2024

manuelsh commented Sep 16, 2024 • edited Loading

manuelsh commented Sep 18, 2024

manuelsh commented Aug 12, 2024 •

edited

Loading

manuelsh commented Aug 15, 2024 •

edited

Loading

manuelsh commented Aug 20, 2024 •

edited

Loading

manuelsh commented Sep 16, 2024 •

edited

Loading