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

ENH: Warn when a user provided model name in the config renamed #2004

Conversation

BenjaminBossan
Copy link
Member

Resolves #2001

In PEFT, users can provide a custom base_model_name_or_path argument to the PEFT config. However, this value is overridden by the model's name_or_path attribute. This can be surprising for users. Therefore, there is now a warning about this.

To see why that can be relevant, check the original issue.

It eludes me why this argument can be passed by users only to be overridden by PEFT. This was implemented by Younes before I joined the project.

Resolves huggingface#2001

In PEFT, users can provide a custom base_model_name_or_path argument to
the PEFT config. However, this value is overridden by the model's
name_or_path attribute. This can be surprising for users. Therefore,
there is now a warning about this.

To see why that can be relevant, check the original issue.

It eludes me why this argument can be passed by users only to be
overridden by PEFT. This was implemented by Younes before I joined the
project.
@HuggingFaceDocBuilderDev

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.

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Thanks!

I slightly prefer catching warnings with a with context more, but that is just preferential.


def test_warning_name_transformers_model(self, recwarn):
# The base_model_name_or_path provided by the user is overridden.
model = AutoModelForCausalLM.from_pretrained("facebook/opt-125m")
Copy link
Member

Choose a reason for hiding this comment

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

Let's test with a smaller model? Plenty available here: https://huggingface.co/hf-internal-testing

assert any(msg in str(warning.message) for warning in recwarn.list)

def test_warning_name_custom_model(self, custom_module, recwarn):
get_peft_model(custom_module, LoraConfig(target_modules=["lin"], base_model_name_or_path="custom_name"))
Copy link
Member

Choose a reason for hiding this comment

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

"custom_name" could be assigned to a variable and reused for easier understanding.

- use smaller base model
- assign custom name to variable
@BenjaminBossan
Copy link
Member Author

Thanks @sayakpaul. The last commit should address your comments.

I slightly prefer catching warnings with a with context more

You mean something like pytest.warns? The reason I didn't go with that is that it should not be used to check for absence of warnings, so the tests would have different methods depending on whether there is a warning or not. I wanted to keep the code very similar between the tests, which is why I chose recwarn. If I only test for presence of warnings, I also prefer using a context manager.

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

LGTM!

@BenjaminBossan BenjaminBossan merged commit 22f042a into huggingface:main Aug 14, 2024
14 checks passed
@BenjaminBossan BenjaminBossan deleted the ENH-warning-when-user-provided-model-name-renamed branch August 14, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with PEFT model save_pretrained
3 participants