Add LLaVa-1.6, bis #29586

NielsRogge · 2024-03-11T10:39:38Z

What does this PR do?

This PR adds LLaVa-1.6 (also called LLaVa-NeXT) as a standalone model rather than modifying the existing modeling_llava.py. As discussed in #29012 this might be better.

To do:

convert all checkpoints once PR is approved, along with corresponding integration test, which will also fix the failing documentation test.

HuggingFaceDocBuilderDev · 2024-03-11T11:09:39Z

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-03-12T10:33:23Z

Thanks for opening @NielsRogge! I can see some model related tests are failing - ping me for review once they're all passing

NielsRogge · 2024-03-12T15:05:11Z

@amyeroberts I've fixed all tests, only the documentation test is failing because the official checkpoint hasn't been uploaded yet.

amyeroberts

Thanks for adding this as a new architecture - it's a lot cleaner this way!

docs/source/en/model_doc/llava_next.md

src/transformers/models/llava_next/configuration_llava_next.py

src/transformers/models/llava_next/processing_llava_next.py

src/transformers/models/llava_next/image_processing_llava_next.py

tests/models/llava_next/test_modeling_llava_next.py

amyeroberts · 2024-03-13T16:37:26Z

tests/models/llava_next/test_modeling_llava_next.py

+    @unittest.skip(reason="CPU offload is not yet supported")
+    def test_cpu_offload(self):
+        pass


This is funny - if we have _no_split_modules set then it should be supported. Otherwise the model will automatically be skipped for this test, so we shouldn't need it

Pinging @younesbelkada here regarding how _no_split_modules should be defined for llava/llava-next. This test fails, but it's not clear to me which class would need to be added to _no_split_modules to make it pass, this test is also skipped for llava.

To be resolved

OK, as _no_split_modules are set in a copied model, let's just leave as-is for the moment

tests/models/llava_next/test_modeling_llava_next.py

NielsRogge · 2024-03-17T21:41:53Z

Thanks for your review, addressed all comments and uploaded all checkpoints to a collection.

The documentation test fails, seems like the machine crashed due to the size of the model (cc @ydshieh).

ydshieh · 2024-03-18T09:37:25Z

Thanks for your review, addressed all comments and uploaded all checkpoints to a collection.

The documentation test fails, seems like the machine crashed due to the size of the model (cc @ydshieh).

If you can make use lower resource (anyway like fp16, 4-bit etc) that would be great.
If nothing could avoid the crash, just put the file to utils/not_doctested.txt

amyeroberts · 2024-03-18T12:06:39Z

@NielsRogge Please review the PR before asking for review. There's files which should not be included as part of the PR here

NielsRogge · 2024-03-18T13:56:37Z

Apologies, scripts are removed

amyeroberts

Thanks for the continued work on this. It's almost ready, but some more coverage is needed for tests. In particular, the generation tests need to be added for the model

tests/models/llava_next/test_modeling_llava_next.py

src/transformers/models/llava_next/configuration_llava_next.py

src/transformers/models/llava/configuration_llava.py

src/transformers/models/llava/modeling_llava.py

src/transformers/models/llava_next/image_processing_llava_next.py

src/transformers/models/llava_next/modeling_llava_next.py

tests/models/llava_next/test_image_processor_llava_next.py

src/transformers/models/llava_next/image_processing_llava_next.py

NielsRogge · 2024-03-19T07:35:17Z

Regarding your comment regarding the aspect_ratio_setting of LlavaNextImageProcessor -> actually only aspect_ratio_setting = "anyres" is used for llava 1.6 (LlavaNextForConditionalGeneration).

The other 2 settings ("clip" and "pad") are there to be used for LlavaForConditionalGeneration (the previous generation). This one currently uses CLIPImageProcessor, which is technically incorrect, it should use the "pad" setting of LlavaNextImageProcessor. But I assume we can't simply replace the image processor of LlavaForConditionalGeneration by a new one (cc @ArthurZucker).

Should I keep the aspect_ratio_setting, or remove it and only support "anyres" by default?

amyeroberts · 2024-03-19T09:40:46Z

This one currently uses CLIPImageProcessor, which is technically incorrect, it should use the "pad" setting of LlavaNextImageProcessor. But I assume we can't simply replace the image processor of LlavaForConditionalGeneration by a new one (cc @ArthurZucker).

I don't quite understand this statement. "This one" is referring to LlavaForConditionalGeneration or LlavaNextForConditionalGeneration?

But I assume we can't simply replace the image processor of LlavaForConditionalGeneration by a new one (cc @ArthurZucker).

Again, not sure which object you're talking about replacing, but if there's an incorrect implementation it would be better to fix it, possibly with some kind of deprecation cycle / legacy behaviour flag. If the issue is with the pad method, I don't see why we need to completely replace the image processor, instead of updating pad

Should I keep the aspect_ratio_setting, or remove it and only support "anyres" by default?

We should remove the aspect_ratio_setting flag entirely

It doesn't make sense to have an image processor for a specific model, which is preparing inputs for a different model which already has its own image processor.

NielsRogge · 2024-03-19T13:40:26Z

Ok, I've removed the flag as it leads to confusion. Feel free to approve the PR, if possible.

amyeroberts

Thanks for iterating!

PR can't be merged as-is as it introduces breaking changes to existing models: llava and vip llava

src/transformers/models/llava/configuration_llava.py

amyeroberts · 2024-03-19T16:51:43Z

tests/models/vipllava/test_modeling_vipllava.py

+            model_tied = model_class(config_tied)
+            params_tied = list(model_tied.parameters())
+            # Check that the embedding layer and decoding layer are the same in size and in value
+            # self.assertTrue(check_same_values(embeddings, decoding))


Why are a bunch of tests commented out here?

This is copied from the common test

amyeroberts · 2024-03-19T16:52:56Z

tests/models/llava_next/test_modeling_llava_next.py

+    @unittest.skip(reason="CPU offload is not yet supported")
+    def test_cpu_offload(self):
+        pass


To be resolved

amyeroberts · 2024-03-19T16:53:27Z

tests/models/llava_next/test_modeling_llava_next.py

+            model_tied = model_class(config_tied)
+            params_tied = list(model_tied.parameters())
+            # Check that the embedding layer and decoding layer are the same in size and in value
+            # self.assertTrue(check_same_values(embeddings, decoding))


Same question here - it's better not to copy and have all this dead code

Opened #29737 to fix this

rebase and make fix-copies should fix this now :)

amyeroberts · 2024-03-19T16:54:53Z

tests/models/llava/test_modeling_llava.py

+            model_tied = model_class(config_tied)
+            params_tied = list(model_tied.parameters())
+            # Check that the embedding layer and decoding layer are the same in size and in value
+            # self.assertTrue(check_same_values(embeddings, decoding))


Same here re commented out code

amyeroberts · 2024-03-19T16:56:36Z

tests/models/llava/test_modeling_llava.py

@@ -186,6 +184,188 @@ def test_training_gradient_checkpointing_use_reentrant(self):
    def test_training_gradient_checkpointing_use_reentrant_false(self):
        pass

+    # Copied from tests.test_modeling_common.ModelTesterMixin.test_resize_tokens_embeddings with config.vocab_size->config.text_config.vocab_size


You can still set config.vocab_size as a parameter, even if it's not an input argument to the config's init. This would solve having to have all of these copied files for llava, vipllava and llava next

I've tried that, but that doesn't work. The test fails with:

# Check that resizing the token embeddings with a larger vocab size increases the model's vocab size model_embed = model.resize_token_embeddings(model_vocab_size + 10) self.assertEqual(model.config.vocab_size, model_vocab_size + 10) AssertionError: 99 != 109

This is because when calling the resize_token_embeddings method, we are changing config.text_config.vocab_size, whereas config.vocab_size remains unchanged.

That's because you removed the line that properly updates the config value in resize_token_embeddings

Ok but adding an attribute just to make a test pass sounds a bit weird, right? There's no need to add vocab_size as attribute of the model config, only the text config needs to have it, which why I've removed that line from resize_token_embeddings. Are you ok with leveraging copied from for these tests?

Yes, I agree we don't want to just add stuff to make the tests pass. I mistakenly though vocab_size was still being set in the config for BC, in which case, we would have still needed to update the parameter.

If we leave as a property to deprecate (cf notes in vip llava config file) then we can leave copied from headers here

src/transformers/models/vipllava/configuration_vipllava.py

src/transformers/models/llava_next/modeling_llava_next.py

ArthurZucker

LGTM let's just fix the last 2 nits!

src/transformers/utils/dummy_pt_objects.py

tests/models/musicgen_melody/test_modeling_musicgen_melody.py

amyeroberts · 2024-03-20T09:06:47Z

src/transformers/models/llava/configuration_llava.py

+                FutureWarning,
+            )
+            # set the vocab_size
+            text_config["vocab_size"] = kwargs["vocab_size"]


No - this is still wrong and will break backwards compatibility

vocab size from the kwargs was only ever set when text_config was None

Just pushed an update which resolves this. If not, please let me know how we can make it backwards compatible.

amyeroberts

Thanks for the continued iterations!

Just a few final notes on BC for the config files

amyeroberts · 2024-03-20T14:12:41Z

tests/models/llava_next/test_modeling_llava_next.py

+            model_tied = model_class(config_tied)
+            params_tied = list(model_tied.parameters())
+            # Check that the embedding layer and decoding layer are the same in size and in value
+            # self.assertTrue(check_same_values(embeddings, decoding))


rebase and make fix-copies should fix this now :)

amyeroberts · 2024-03-20T14:17:37Z

tests/models/llava/test_modeling_llava.py

@@ -186,6 +184,188 @@ def test_training_gradient_checkpointing_use_reentrant(self):
    def test_training_gradient_checkpointing_use_reentrant_false(self):
        pass

+    # Copied from tests.test_modeling_common.ModelTesterMixin.test_resize_tokens_embeddings with config.vocab_size->config.text_config.vocab_size


Yes, I agree we don't want to just add stuff to make the tests pass. I mistakenly though vocab_size was still being set in the config for BC, in which case, we would have still needed to update the parameter.

If we leave as a property to deprecate (cf notes in vip llava config file) then we can leave copied from headers here

amyeroberts · 2024-03-20T14:21:24Z

tests/models/llava_next/test_modeling_llava_next.py

+    @unittest.skip(reason="CPU offload is not yet supported")
+    def test_cpu_offload(self):
+        pass


OK, as _no_split_modules are set in a copied model, let's just leave as-is for the moment

amyeroberts · 2024-03-20T14:25:17Z

src/transformers/models/vipllava/configuration_vipllava.py

+                # set the vocab_size
+                text_config.vocab_size = kwargs["vocab_size"]
+
+        self.text_config = text_config

        super().__init__(**kwargs)


Because it's a param which used to be part of the config, we'll need to deprecate vocab_size too

def __init__(...): ... self._vocab_size = self.text_config.vocab_size @property def vocab_size(self): warnings.warn( "The `vocab_size` property is deprecated and will be removed in v4.42, since it can be inferred from the `text_config`.", FutureWarning, ) return self._vocab_size def to_dict(self): output = super().to_dict() output.pop("_vocab_size", None) return output

This should be added to the llava config too

amyeroberts · 2024-03-20T14:25:56Z

src/transformers/models/vipllava/configuration_vipllava.py

+        if "vocab_size" in kwargs and not isinstance(text_config, dict):
+            warnings.warn(
+                "The `vocab_size` argument is deprecated and will be removed in v4.42, since it can be inferred from the `text_config`.",
+                FutureWarning,
+            )
+            # set the vocab_size
+            text_config["vocab_size"] = kwargs["vocab_size"]


Accidentally left? This has the same BC issues as before and conflicts with the code below

Suggested change

if "vocab_size" in kwargs and not isinstance(text_config, dict):

warnings.warn(

"The `vocab_size` argument is deprecated and will be removed in v4.42, since it can be inferred from the `text_config`.",

FutureWarning,

)

# set the vocab_size

text_config["vocab_size"] = kwargs["vocab_size"]

amyeroberts · 2024-03-20T14:34:39Z

src/transformers/models/vipllava/configuration_vipllava.py

+            if "vocab_size" in kwargs:
+                warnings.warn(
+                    "The `vocab_size` argument is deprecated and will be removed in v4.42, since it can be inferred from the `text_config`.",
+                    FutureWarning,
+                )
+                # set the vocab_size
+                text_config.vocab_size = kwargs["vocab_size"]


This might still result in some unexpected behaviour, as previously self.vocab_size was only set from the kwarg vocab_size if text_config was None, but in neither case was the text config overwritten by the passed in argument.

Instead, rather than forcibly overwriting here, I would just have the warning at the top of the init if "vocab_size" is passed in

def __init__(...): if "vocab_size" in kwargs: warnings.warn( "The `vocab_size` argument is deprecated and will be removed in v4.42, since it can be inferred from the `text_config`. Passing this argument will have no effect", FutureWarning, )

Suggested change

if "vocab_size" in kwargs:

warnings.warn(

"The `vocab_size` argument is deprecated and will be removed in v4.42, since it can be inferred from the `text_config`.",

FutureWarning,

)

# set the vocab_size

text_config.vocab_size = kwargs["vocab_size"]

amyeroberts · 2024-03-20T14:35:44Z

src/transformers/models/llava/configuration_llava.py

+            if "vocab_size" in kwargs:
+                warnings.warn(
+                    "The `vocab_size` argument is deprecated and will be removed in v4.42, since it can be inferred from the `text_config`.",
+                    FutureWarning,
+                )
+                # set the vocab_size
+                text_config.vocab_size = kwargs["vocab_size"]


Same comments as for vip llava:

Just have a warning at the start of the init

Add some deprecation for vocab_size

amyeroberts

Pushed the changes requested so all good to merge once tests pass!

* First draft * Fix tests, add docs * Improve docstrings * Fix test * Address comments * Address comments * Remove vocab_size attribute * Remove batch_size * Address comment * Add image processor tests * Support fx * Update docstring * Add support for 34b * Convert 34b model * Add integration tests * Update checkpoints * Convert vicuna-13b, remove doc tests * Remove script * Remove file * Address comments * Improve docstrings * Deprecate vocab_size * Remove aspect_ratio_setting * Address comments * Update READMEs * Add tips about chat templates * Fix tests * Deprecate vocab_size safely * Update tests --------- Co-authored-by: Amy Roberts <22614925+amyeroberts@users.noreply.github.com>

ArthurZucker · 2024-03-21T06:55:43Z

a2a9516 there were some unprotected imports 😈

fxmarty · 2024-03-22T05:09:14Z

src/transformers/models/llava/configuration_llava.py

        self.vision_feature_select_strategy = vision_feature_select_strategy
        self.vision_feature_layer = vision_feature_layer
-        self.vocab_size = vocab_size


self = <tests.models.llava.test_modeling_llava.LlavaForConditionalGenerationIntegrationTest testMethod=test_small_model_integration_test_llama> @slow @require_bitsandbytes def test_small_model_integration_test_llama(self): # Let' s make sure we test the preprocessing to replace what is used model_id = "llava-hf/llava-1.5-7b-hf" > model = LlavaForConditionalGeneration.from_pretrained("llava-hf/llava-1.5-7b-hf", load_in_4bit=True) tests/models/llava/test_modeling_llava.py:390: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ src/transformers/modeling_utils.py:3006: in from_pretrained config, model_kwargs = cls.config_class.from_pretrained( src/transformers/configuration_utils.py:609: in from_pretrained return cls.from_dict(config_dict, **kwargs) src/transformers/configuration_utils.py:761: in from_dict config = cls(**config_dict) src/transformers/models/llava/configuration_llava.py:140: in __init__ super().__init__(**kwargs) src/transformers/configuration_utils.py:375: in __init__ raise err src/transformers/configuration_utils.py:372: in __init__ setattr(self, key, value) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = LlavaConfig { "architectures": [ "LlavaForConditionalGeneration" ], "ignore_index": -100, "image_token_ind..._dim": 768, "vocab_size": 32000 }, "vision_feature_layer": -2, "vision_feature_select_strategy": "default" } key = 'vocab_size', value = 32064 def __setattr__(self, key, value): if key in super().__getattribute__("attribute_map"): key = super().__getattribute__("attribute_map")[key] > super().__setattr__(key, value) E AttributeError: can't set attribute

will fix in #29389, a setter is missing

sorry about this - thanks for addressing and fixing

aliencaocao · 2024-03-23T12:41:17Z

This model when loading gives The model weights are not tied. Please use the tie_weightsmethod before using theinfer_auto_device function.. I am using device_map='auto'. Anyway to fix this?

amyeroberts · 2024-04-02T12:19:19Z

Hi @aliencaocao, could you open an issue detailing this error and a minimal code snippet to reproduce? This will help us track what has and hasn't been addressed and help people find it if they're experiencing something similar.

aliencaocao · 2024-04-02T16:25:00Z

#30001

* First draft * Fix tests, add docs * Improve docstrings * Fix test * Address comments * Address comments * Remove vocab_size attribute * Remove batch_size * Address comment * Add image processor tests * Support fx * Update docstring * Add support for 34b * Convert 34b model * Add integration tests * Update checkpoints * Convert vicuna-13b, remove doc tests * Remove script * Remove file * Address comments * Improve docstrings * Deprecate vocab_size * Remove aspect_ratio_setting * Address comments * Update READMEs * Add tips about chat templates * Fix tests * Deprecate vocab_size safely * Update tests --------- Co-authored-by: Amy Roberts <22614925+amyeroberts@users.noreply.github.com>

NielsRogge requested a review from amyeroberts March 11, 2024 11:14

amyeroberts reviewed Mar 13, 2024

View reviewed changes

This was referenced Mar 17, 2024

[Processor classes] Update docs #29698

Merged

Add LLaVa 1.6 #29012

Closed

NielsRogge requested a review from amyeroberts March 18, 2024 11:53

amyeroberts reviewed Mar 18, 2024

View reviewed changes

xwjiang2010 mentioned this pull request Mar 18, 2024

[Feature] Add vision language model support. vllm-project/vllm#3042

Merged

LoFiApostasy mentioned this pull request Mar 19, 2024

Model Request: Addition of new Lava-1.6 models jhc13/taggui#50

Closed

amyeroberts reviewed Mar 19, 2024

View reviewed changes

NielsRogge mentioned this pull request Mar 19, 2024

[Tests] Remove unused code #29737

Merged

ArthurZucker reviewed Mar 20, 2024

View reviewed changes

src/transformers/utils/dummy_pt_objects.py Show resolved Hide resolved

tests/models/musicgen_melody/test_modeling_musicgen_melody.py Outdated Show resolved Hide resolved

amyeroberts reviewed Mar 20, 2024

View reviewed changes

NielsRogge added 7 commits March 20, 2024 15:27

First draft

063b32f

Fix tests, add docs

5526f06

Improve docstrings

adbeaa9

Fix test

22ac79c

Address comments

4885a05

Address comments

697d79c

Remove vocab_size attribute

c758ea8

NielsRogge and others added 18 commits March 20, 2024 15:27

Update docstring

eef0faa

Add support for 34b

10e6d75

Convert 34b model

b2a1110

Add integration tests

690bd04

Update checkpoints

5729354

Convert vicuna-13b, remove doc tests

1ce2d0e

Remove script

6a17c3b

Remove file

56a775c

Address comments

bcba222

Improve docstrings

44af6ed

Deprecate vocab_size

7c7a1ad

Remove aspect_ratio_setting

790dde4

Address comments

71442fa

Update READMEs

7f8c612

Add tips about chat templates

d8de28b

Fix tests

e09398b

Deprecate vocab_size safely

d8eb7e0

Update tests

0b68483

amyeroberts force-pushed the add_llava_1_6_bis branch from ddf7abd to 0b68483 Compare March 20, 2024 15:29

amyeroberts approved these changes Mar 20, 2024

View reviewed changes

amyeroberts merged commit d91fd7f into huggingface:main Mar 20, 2024
22 checks passed

ywang96 mentioned this pull request Mar 21, 2024

Fix 'vocab_size' attribute of LlavaConfig #29794

Closed

5 tasks

fxmarty reviewed Mar 22, 2024

View reviewed changes

fxmarty mentioned this pull request Mar 22, 2024

Correct llava mask & fix missing setter for vocab_size #29389

Merged

Add LLaVa-1.6, bis #29586

Add LLaVa-1.6, bis #29586

Conversation

NielsRogge commented Mar 11, 2024 • edited Loading

What does this PR do?

HuggingFaceDocBuilderDev commented Mar 11, 2024

amyeroberts commented Mar 12, 2024

NielsRogge commented Mar 12, 2024

amyeroberts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NielsRogge Mar 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NielsRogge commented Mar 17, 2024

ydshieh commented Mar 18, 2024

amyeroberts commented Mar 18, 2024

NielsRogge commented Mar 18, 2024

amyeroberts left a comment

Choose a reason for hiding this comment

NielsRogge commented Mar 19, 2024 • edited Loading

amyeroberts commented Mar 19, 2024

NielsRogge commented Mar 19, 2024

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

NielsRogge Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NielsRogge Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurZucker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NielsRogge Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

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

amyeroberts left a comment

Choose a reason for hiding this comment

ArthurZucker commented Mar 21, 2024

Choose a reason for hiding this comment

fxmarty Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliencaocao commented Mar 23, 2024

amyeroberts commented Apr 2, 2024

aliencaocao commented Apr 2, 2024

NielsRogge commented Mar 11, 2024 •

edited

Loading

NielsRogge Mar 17, 2024 •

edited

Loading

NielsRogge commented Mar 19, 2024 •

edited

Loading

NielsRogge Mar 19, 2024 •

edited

Loading

NielsRogge Mar 20, 2024 •

edited

Loading

NielsRogge Mar 20, 2024 •

edited

Loading

fxmarty Mar 22, 2024 •

edited

Loading