[`core`/ FEAT] Add the possibility to push custom tags using `PreTrainedModel` itself #28405

younesbelkada · 2024-01-09T08:04:09Z

What does this PR do?

From an idea we discussed internally. This PR introduces a new API to inject custom tags in the model card

From a community perspective it will make easier to push custom tags as for now it is only limited to trainers.

Below I demonstrate the simplicity of the API

from transformers import AutoModelForCausalLM

model_name = "HuggingFaceM4/tiny-random-LlamaForCausalLM"
model = AutoModelForCausalLM.from_pretrained(model_name)

model.add_model_tags(["tag-test"])
model.push_to_hub("llama-tagged")

cc @osanseviero @Narsil @julien-c

Note with the current design each time a user calls push_to_hub, it will create a model card template if no model card is present on the hub

HuggingFaceDocBuilderDev · 2024-01-09T08:35:42Z

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.

younesbelkada · 2024-01-09T09:36:20Z

I made some modification after some offline discussion, an exmaple model pushed can be found here: https://huggingface.co/ybelkada/test-tags-model lmk wdyt @amyeroberts @ArthurZucker @Wauplin
I also updated the API on the PR description

osanseviero · 2024-01-09T10:14:11Z

src/transformers/modeling_utils.py

+        elif "tags" in kwargs and self._model_tags is not None:
+            logger.warning(
+                "You manually passed `tags` to `push_to_hub` method and the model has already some tags set, we will use the tags that you passed."
+            )


Should we use both instead?

+1 on merging both lists if they exist (for flexibility)

Makes sense, done !

amyeroberts

Thanks for adding this! Looking good 🤗

Two comments:

I'd have some way for the user to inspect the current tags e.g. get_model_tags or just using a non-private attribute model_tags. Otherwise I'm blindly modifying the tags without knowing the current state.
It should be possible to reset or remove tags, rather than just add them

amyeroberts · 2024-01-09T10:09:54Z

src/transformers/utils/hub.py

+        card_data = ModelCardData(language="en", tags=[])
+        model_card = ModelCard.from_template(card_data)
+
+    if model_card is not None and tags is not None:


Why would model_card be None here?

Ah nice catch, yes it should never be None here

src/transformers/utils/hub.py

amyeroberts · 2024-01-09T10:13:12Z

src/transformers/modeling_utils.py

+        for tag in tags:
+            if tag not in self._model_tags:
+                self._model_tags.append(tag)


I don't think we want an append behaviour here - more of an overwrite. Otherwise, if I accidentally add a tag there's no way to ever remove it. I'd suggest a set_model_tags and add_model_tags method

I'd also be in favor of switching to add_model_tags if we are appending tags to a list.
Don't know if adding support for removing tags if really necessary (but I don't have full context).

Yeah makes sense, I changed that method to add_tags and added a new method set_tags

Wauplin

Thanks for implementing this @younesbelkada!
I left some comments. Some might be out of scope for this PR so feel free to do as you prefer :)

Wauplin · 2024-01-09T10:24:01Z

src/transformers/modeling_utils.py

+        for tag in tags:
+            if tag not in self._model_tags:
+                self._model_tags.append(tag)


I'd also be in favor of switching to add_model_tags if we are appending tags to a list.
Don't know if adding support for removing tags if really necessary (but I don't have full context).

Wauplin · 2024-01-09T10:26:28Z

src/transformers/modeling_utils.py

+        elif "tags" in kwargs and self._model_tags is not None:
+            logger.warning(
+                "You manually passed `tags` to `push_to_hub` method and the model has already some tags set, we will use the tags that you passed."
+            )


+1 on merging both lists if they exist (for flexibility)

Wauplin · 2024-01-09T10:32:10Z

src/transformers/modeling_utils.py

+        if "tags" not in kwargs:
+            kwargs["tags"] = self._model_tags
+        elif "tags" in kwargs and self._model_tags is not None:
+            logger.warning(


(nit) I think using warnings.warn would be more appropriate here. I usually tend to follow the "rule" from https://docs.python.org/3/howto/logging.html:

warnings.warn() in library code if the issue is avoidable and the client application should be modified to eliminate the warning

logging.warning() if there is nothing the client application can do about the situation, but the event should still be noted

Here the problem is most likely avoidable by the user.

Thanks! I think that transformers fully switched to using logger.warning (I got told that in a PR review from @LysandreJik but I don't remember which one 😢 ) let me try to find that and link the comment here

No you can keep using warnings.warn! see #26527

src/transformers/utils/hub.py

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

Co-authored-by: Lucain <lucainp@gmail.com>

younesbelkada · 2024-01-09T14:07:00Z

@amyeroberts @Wauplin I think that I have addressed most of your comments now, the API is much more flexible and less "agressive" thanks to your suggestions
1- Creates a simple description
2- Moved transformers to library-name
3- Now Trainer and model_tags are compatible together, which was not the case in the first commits. e.g. when combining SFTTrainer from trl that automatically pushes sft and trl tags and this PR, all tags gets successfully pushed. E.g.:

from transformers import AutoModelForCausalLM, TrainingArguments
from trl import SFTTrainer
from datasets import load_dataset

model_name = "HuggingFaceM4/tiny-random-LlamaForCausalLM"
model = AutoModelForCausalLM.from_pretrained(model_name)

model.add_model_tags(["tag-test"])

train_data = load_dataset("imdb", split="train")

trainer = SFTTrainer(
    model=model,
    args=TrainingArguments(output_dir="test-tag-already-tagged"),
    train_dataset=train_data,
    dataset_text_field="text"
)

trainer.push_to_hub("ybelkada/test-tag-already-tagged")

The example repo is here: https://huggingface.co/ybelkada/test-tag-already-tagged
4- If tags already exists it will not overwrite all the tags but add the new tags with existing tags. e.g. if one does model.add_tags(["test-tag-2"]) after already pushing a model with a tag to Hub, as you can see from this repository: https://huggingface.co/ybelkada/test-model-already-tagged the new tag gets appended

cc @osanseviero as well

Wauplin

Looks good! Thanks for making the changes :)

Regarding set_model_tags, reset_model_tags and remove_model_tags, I feel that their behavior might be misleading/non-explicit. For example remove_model_tags will remove tags from the model object (AutoModelForCausalLM.model_tags in your example) but not from the model card on the Hub. I think some users can expect model.remove_model_tags("sft") to remove the "sft" tag from the existing model card. Maybe it's safer not to implement those three methods to avoid questions. (Also I'm not sure they would be really used -only my 2c-)

amyeroberts

Thanks for iterating - LGTM!

Agree with @Wauplin re other methods. I realise I asked for set_model_tags but realise wow model_tags are public, we probably don't need it as the user can just do it themselves directly

younesbelkada

Thanks @amyeroberts @Wauplin , this is ready for a final review, I tested it on different scenarios and added some docs with a simple code snippet, let me know wdyt !

younesbelkada · 2024-01-10T13:41:07Z

src/transformers/utils/hub.py

+    repo_id: str,
+    tags: Optional[List[str]] = None,
+    token: Optional[str] = None,
+    ignore_metadata_errors: bool = False,


@Wauplin @amyeroberts fine to assume that repo_type will always be set to "model" in ModelCard.load() (inspecting at that methods signature)

Yes definitely, repo_type="model" is implicit when using ModelCard.
(the parameter exists because ModelCard inherits from the generic RepoCard. Not a perfect design-choice but it's fine :) )

ok perfect! let's keep it like that then

younesbelkada · 2024-01-10T14:08:59Z

src/transformers/utils/hub.py

+        token (`str`, *optional*):
+            Authentication token, obtained with `huggingface_hub.HfApi.login` method. Will default to the stored token.
+        ignore_metadata_errors (`str`):
+            If True, errors while parsing the metadata section will be ignored. Some information might be lost during
+            the process. Use it at your own risk.


I copied this from ModelCard.load_card docstring

osanseviero

Looks cool!

src/transformers/modeling_utils.py

Co-authored-by: Omar Sanseviero <osanseviero@gmail.com>

Wauplin

Looks good to me! I left some minor comments but feel free to ignore.

Wauplin · 2024-01-10T14:10:47Z

src/transformers/modeling_utils.py

@@ -1253,6 +1255,38 @@ def _backward_compatibility_gradient_checkpointing(self):
            # Remove the attribute now that is has been consumed, so it's no saved in the config.
            delattr(self.config, "gradient_checkpointing")

+    def add_model_tags(self, tags: Union[List[str], str]) -> None:
+        r"""
+        Add custom tags into the model that gets pushed in 🤗 Hub. Will


(nit)

Suggested change

Add custom tags into the model that gets pushed in 🤗 Hub. Will

Add custom tags into the model that gets pushed to the Hugging Face Hub. Will

Ah thanks! I think it has been partially fixed by the suggestion of Omar above :D

let me push your fix as well

Yep sorry I was already reviewing when Omar posted his comment :)

src/transformers/utils/hub.py

Wauplin · 2024-01-10T14:15:42Z

src/transformers/utils/hub.py

+    repo_id: str,
+    tags: Optional[List[str]] = None,
+    token: Optional[str] = None,
+    ignore_metadata_errors: bool = False,


Yes definitely, repo_type="model" is implicit when using ModelCard.
(the parameter exists because ModelCard inherits from the generic RepoCard. Not a perfect design-choice but it's fine :) )

Wauplin · 2024-01-10T14:19:58Z

tests/test_modeling_utils.py

+
+        self.assertTrue(model.model_tags == new_tags)
+
+        model.push_to_hub("test-dynamic-model-with-tags", token=self._token)


Isn't it problematic to have a fixed name for the repo_id? This could lead to conflicts if several CIs are ran in parallel. Using a unique id (uuid) would avoid that.
(not an expert of transformers's CI though. It's just that I've experienced similar problem in huggingface_hub's CI in the past)

Hmm good point, we do the same things for all tests above so perhaps it is fine, I will let @ydshieh comment on this if he has more insights than me

Co-authored-by: Lucain <lucainp@gmail.com>

src/transformers/modeling_utils.py

amyeroberts

Thanks for iterating!

Just a final comment / question on the push_to_hub logic

amyeroberts · 2024-01-12T18:51:01Z

src/transformers/modeling_utils.py

+        if "tags" not in kwargs:
+            kwargs["tags"] = self.model_tags
+        elif "tags" in kwargs and self.model_tags is not None:
+            for model_tag in self.model_tags:
+                # merge the tags together
+                if model_tag not in kwargs["tags"]:
+                    kwargs["tags"].append(model_tag)
+        return super().push_to_hub(*args, **kwargs)


The current if/lse logic means:

if self.model_tags is None and tags aren't passed in kwargs['tags'] is set to None. Do we want this or no tags kwarg at all?
* tags are only used if self.model_tags is not None. - just realised I'm wrong as kwargs are passed otherwise 🙃

Another Q

Is it possible kwargs["tags"] is a string here too?

Suggested change

if "tags" not in kwargs:

kwargs["tags"] = self.model_tags

elif "tags" in kwargs and self.model_tags is not None:

for model_tag in self.model_tags:

# merge the tags together

if model_tag not in kwargs["tags"]:

kwargs["tags"].append(model_tag)

return super().push_to_hub(*args, **kwargs)

tags = self.model_tags if self.model_tags is not None else []

for tag in kwargs.get("tags", []):

if tag not in tags:

tags.append(tag)

if tags:

kwargs["tags"] = tags

return super().push_to_hub(*args, **kwargs)

I think your suggestion sounds great, also

Is it possible kwargs["tags"] is a string here too?

It can be strings, let me adapt a bit the logic after accepting your suggestion

I just took care of the str case in 1e3fc1e

src/transformers/trainer.py

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

LysandreJik · 2024-01-15T09:58:50Z

Thanks for handling this @younesbelkada

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

younesbelkada · 2024-01-15T10:45:48Z

src/transformers/trainer.py

@@ -3581,6 +3581,15 @@ def create_model_card(
            library_name = ModelCard.load(model_card_filepath).data.get("library_name")
            is_peft_library = library_name == "peft"

+            # Append existing tags in `tags`
+            existing_tags = ModelCard.load(model_card_filepath).data.tags


This handles a corner case I just found out for Trainers, if you push into a repo that has already some tags, it will overwrite the existing tags with new tags. This block simply cirumvents this by instead of overwriting new tags it will append new tags from existing tags

Let me know if this is ok

ModelCard.load(model_card_filepath).data.tags

can give None if the model card is empty

from huggingface_hub import ModelCard ModelCard.load("ybelkada/test-empty-model-case").data.tags >>> None

Look OK to me - only comment is that existing_tags might be None, so you'll need to convert it to a list before adding tags if ModelCard.load(model_card_filepath).data.tags doesn't have anything set

I pushed in 59738c6 a simple workaround, I think that way we don't need to convert it to a list as we care about that corner case only if tags are explicilty passed + there are some existing tags already

amyeroberts

Thanks for adding this and iterating - looks great!

…nedModel` itself (huggingface#28405) * v1 tags * remove unneeded conversion * v2 * rm unneeded warning * add more utility methods * Update src/transformers/utils/hub.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> * Update src/transformers/utils/hub.py Co-authored-by: Lucain <lucainp@gmail.com> * Update src/transformers/utils/hub.py Co-authored-by: Lucain <lucainp@gmail.com> * more enhancements * oops * merge tags * clean up * revert unneeded change * add extensive docs * more docs * more kwargs * add test * oops * fix test * Update src/transformers/modeling_utils.py Co-authored-by: Omar Sanseviero <osanseviero@gmail.com> * Update src/transformers/utils/hub.py Co-authored-by: Lucain <lucainp@gmail.com> * Update src/transformers/modeling_utils.py * Update src/transformers/trainer.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> * Update src/transformers/modeling_utils.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> * add more conditions * more logic --------- Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> Co-authored-by: Lucain <lucainp@gmail.com> Co-authored-by: Omar Sanseviero <osanseviero@gmail.com>

v1 tags

dd5cbe3

younesbelkada changed the title ~~[core/ FETA] Add the possibility to push custom tags using PreTrainedModel itself~~ [core/ FEAT] Add the possibility to push custom tags using PreTrainedModel itself Jan 9, 2024

remove unneeded conversion

f78ed31

v2

da00274

younesbelkada requested review from ArthurZucker, amyeroberts and Wauplin January 9, 2024 09:36

rm unneeded warning

1180585

osanseviero reviewed Jan 9, 2024

View reviewed changes

amyeroberts reviewed Jan 9, 2024

View reviewed changes

Wauplin reviewed Jan 9, 2024

View reviewed changes

younesbelkada and others added 10 commits January 9, 2024 12:27

Merge remote-tracking branch 'upstream/main' into set-custom-tag

3485b10

add more utility methods

4b82255

Update src/transformers/utils/hub.py

4c7806e

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

Update src/transformers/utils/hub.py

8b89796

Co-authored-by: Lucain <lucainp@gmail.com>

Update src/transformers/utils/hub.py

e73dc7b

Co-authored-by: Lucain <lucainp@gmail.com>

more enhancements

fbef2de

oops

0e4daad

merge tags

c19e751

clean up

eb93371

revert unneeded change

1fe93b3

younesbelkada requested review from amyeroberts and Wauplin January 9, 2024 14:07

Wauplin reviewed Jan 9, 2024

View reviewed changes

amyeroberts approved these changes Jan 9, 2024

View reviewed changes

younesbelkada added 2 commits January 10, 2024 13:14

Merge remote-tracking branch 'upstream/main' into set-custom-tag

a24ad9b

add extensive docs

6cfd6f5

younesbelkada commented Jan 10, 2024

View reviewed changes

younesbelkada marked this pull request as ready for review January 10, 2024 13:41

younesbelkada requested review from Wauplin and amyeroberts January 10, 2024 13:41

younesbelkada added 3 commits January 10, 2024 13:46

add test

acd676b

oops

db3197d

fix test

f14cf93

younesbelkada commented Jan 10, 2024

View reviewed changes

osanseviero approved these changes Jan 10, 2024

View reviewed changes

src/transformers/modeling_utils.py Outdated Show resolved Hide resolved

Update src/transformers/modeling_utils.py

31117f4

Co-authored-by: Omar Sanseviero <osanseviero@gmail.com>

Wauplin approved these changes Jan 10, 2024

View reviewed changes

Update src/transformers/utils/hub.py

36f2cb7

Co-authored-by: Lucain <lucainp@gmail.com>

younesbelkada commented Jan 10, 2024

View reviewed changes

src/transformers/modeling_utils.py Outdated Show resolved Hide resolved

Update src/transformers/modeling_utils.py

514f13b

ArthurZucker removed their request for review January 11, 2024 10:22

amyeroberts reviewed Jan 12, 2024

View reviewed changes

Update src/transformers/trainer.py

b3d5900

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

younesbelkada and others added 3 commits January 15, 2024 11:29

Update src/transformers/modeling_utils.py

22d3412

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

Merge remote-tracking branch 'upstream/main' into set-custom-tag

85584ae

add more conditions

1e3fc1e

younesbelkada commented Jan 15, 2024

View reviewed changes

more logic

59738c6

younesbelkada requested a review from amyeroberts January 15, 2024 10:48

amyeroberts approved these changes Jan 15, 2024

View reviewed changes

younesbelkada merged commit 1b9a2e4 into huggingface:main Jan 15, 2024
21 checks passed

younesbelkada deleted the set-custom-tag branch January 15, 2024 13:48

	Add custom tags into the model that gets pushed in 🤗 Hub. Will
	Add custom tags into the model that gets pushed to the Hugging Face Hub. Will


		self.assertTrue(model.model_tags == new_tags)

		model.push_to_hub("test-dynamic-model-with-tags", token=self._token)

[core/ FEAT] Add the possibility to push custom tags using PreTrainedModel itself #28405

[core/ FEAT] Add the possibility to push custom tags using PreTrainedModel itself #28405

Conversation

younesbelkada commented Jan 9, 2024 • edited

What does this PR do?

HuggingFaceDocBuilderDev commented Jan 9, 2024

younesbelkada commented Jan 9, 2024 • edited

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

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

Wauplin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wauplin Jan 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

younesbelkada commented Jan 9, 2024 • edited

Wauplin left a comment

Choose a reason for hiding this comment

amyeroberts left a comment

Choose a reason for hiding this comment

younesbelkada 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

osanseviero left a comment

Choose a reason for hiding this comment

Wauplin 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

amyeroberts Jan 12, 2024 • edited

Choose a reason for hiding this comment

younesbelkada Jan 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LysandreJik commented Jan 15, 2024

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

[`core`/ FEAT] Add the possibility to push custom tags using `PreTrainedModel` itself #28405

[`core`/ FEAT] Add the possibility to push custom tags using `PreTrainedModel` itself #28405

younesbelkada commented Jan 9, 2024 •

edited

younesbelkada commented Jan 9, 2024 •

edited

Wauplin Jan 9, 2024 •

edited

younesbelkada commented Jan 9, 2024 •

edited

amyeroberts Jan 12, 2024 •

edited

younesbelkada Jan 15, 2024 •

edited