Skip to content

Generate: can_generate() recursive check - #33718

Merged
gante merged 3 commits into
huggingface:mainfrom
gante:recursive_check
Sep 26, 2024
Merged

Generate: can_generate() recursive check#33718
gante merged 3 commits into
huggingface:mainfrom
gante:recursive_check

Conversation

@gante

@gante gante commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

What does this PR do?

The deprecation warning added in #33203 was incomplete: classes inheriting from classes that have received the update were throwing the warning. This PR adds a recursive check to can_generate() and tests the warnings thrown by the function.

Example of script that is throwing a warning, but shouldn't (and is fixed in this PR):

from transformers.models.llama import LlamaForCausalLM

class NewLlamaForCausalLM(LlamaForCausalLM):
    pass

model = NewLlamaForCausalLM.from_pretrained("hf-internal-testing/tiny-random-LlamaForCausalLM")
model.generate()

Thank you @regisss for warning me about this one and providing a reproducer 🙏

@gante
gante requested a review from LysandreJik September 26, 2024 10:54
@gante gante changed the title Generate: can_generate() recursive Generate: can_generate() recursive check Sep 26, 2024
@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

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.

@LysandreJik LysandreJik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok! @regisss can you comment whether this fixes the issue you encountered?

@regisss

regisss commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

Ok! @regisss can you comment whether this fixes the issue you encountered?

Yep it's all good on my side, the warning doesn't appear anymore with this fix. Thanks!

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, wil have the same issue for great grand pa check no?

Comment thread src/transformers/modeling_utils.py Outdated
Comment on lines 1649 to 1650

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this only grandfather check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's recursive! (it calls can_generate of the parent, which in turn may call the granparent's, and so on :D )

@gante
gante merged commit 3557f9a into huggingface:main Sep 26, 2024
ArthurZucker pushed a commit that referenced this pull request Sep 26, 2024
* add recursive check and test warnings

* missing space

* models without can_generate
@gante
gante deleted the recursive_check branch September 26, 2024 19:43
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
* add recursive check and test warnings

* missing space

* models without can_generate
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.

5 participants