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

Generate: move generation_*.py src files into generation/*.py #20096

Merged
merged 11 commits into from
Nov 9, 2022

Conversation

gante
Copy link
Member

@gante gante commented Nov 7, 2022

What does this PR do?

Moves generation_*.py source files into generation/*.py.

I tried a few slow tests locally, no problems were raised.

⚠️ the link to the docs seems broken, can't validate their correctness 🤔

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 7, 2022

The documentation is not available anymore as the PR was closed or merged.

@gante gante marked this pull request as ready for review November 7, 2022 15:22
@@ -56,7 +56,7 @@ Wenn Sie mehr als eine Eingabe haben, übergeben Sie die Eingabe als Liste:
... ) # doctest: +SKIP
```

Alle zusätzlichen Parameter für Ihre Aufgabe können auch in die [`pipeline`] aufgenommen werden. Die Aufgabe `Text-Generierung` hat eine [`~generation_utils.GenerationMixin.generate`]-Methode mit mehreren Parametern zur Steuerung der Ausgabe. Wenn Sie zum Beispiel mehr als eine Ausgabe erzeugen wollen, setzen Sie den Parameter `num_return_sequences`:
Alle zusätzlichen Parameter für Ihre Aufgabe können auch in die [`pipeline`] aufgenommen werden. Die Aufgabe `Text-Generierung` hat eine [`~generation.utils.GenerationMixin.generate`]-Methode mit mehreren Parametern zur Steuerung der Ausgabe. Wenn Sie zum Beispiel mehr als eine Ausgabe erzeugen wollen, setzen Sie den Parameter `num_return_sequences`:
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: these changes were mass made with generation_X -> generation.X, X being the name of the moved files (with light manual checking before committing the changes)

@@ -0,0 +1,947 @@
# coding=utf-8
Copy link
Member Author

Choose a reason for hiding this comment

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

(No changes in this file, wasn't tagged as moved because of the new Mixin alias left in the original file. The same comment applies to the PT and TF changes)

Comment on lines 22 to 28
class FlaxGenerationMixin(FlaxGenerationMixin):
# warning at import time
warnings.warn(
"Importing `FlaxGenerationMixin` from `src/transformers/generation_flax_utils.py` is deprecated and will "
"be removed in Transformers v5. Import from `src/transformers/generation/flax_utils.py` instead.",
FutureWarning,
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Our deprecation warnings typically go in __init__() (example). However, the generation mixins have no __init__() -- as it stands, the warning is printed at import time.

@@ -11,7 +11,8 @@ docs/source/en/model_doc/byt5.mdx
docs/source/en/model_doc/tapex.mdx
docs/source/en/model_doc/donut.mdx
docs/source/en/model_doc/encoder-decoder.mdx
src/transformers/generation_utils.py
src/transformers/generation/utils.py
src/transformers/generation/tf_utils.py
Copy link
Member Author

Choose a reason for hiding this comment

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

Sneakily added this one 👼

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Looking at the changes, would be great to see all imports move to a simple from .generation import Xxx. What do you think?

@@ -0,0 +1,13 @@
# Copyright 2022 The HuggingFace Team. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit sad to have an empty init here. Would probably will make it easier for users to have everything importable in this submodule directly? That would just quire building this init like the main init of the models init.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good 👍

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Thanks!

@gante
Copy link
Member Author

gante commented Nov 8, 2022

@sgugger now with ~/generation/__init__.py populated with lazy references (as in most __init__.py files).

I've updated all references to objects outside /generation from generation.file_name.ObjectName to generation.ObjectName, including in the docs. That does make tracking external references easier -- if an object is in ~/generation/__init__.py it means that it is likely used somewhere else, and should be treated with extra care.

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

New init is looking great! Can you jsut track all the generation.xxx to just abbreviate them into generation?

[`~generation_utils.GenerationMixin.beam_sample`],
[`~generation_utils.GenerationMixin.group_beam_search`], and
[`~generation_utils.GenerationMixin.constrained_beam_search`].
This page lists all the utility functions used by [`~generation.utils.GenerationMixin.generate`],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use generation everywhere generation.utils is used?

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Nice refactor!

@gante gante merged commit f270b96 into huggingface:main Nov 9, 2022
@gante gante deleted the separate_generate_folders branch November 9, 2022 15:39
amyeroberts pushed a commit to amyeroberts/transformers that referenced this pull request Nov 14, 2022
…gface#20096)

* move generation_*.py src files into generation/*.py

* populate generation.__init__ with lazy loading

* move imports and references from generation.xxx.object to generation.object
@evrial
Copy link

evrial commented Dec 2, 2022

Could you please update optimum to reflect these changes?

from optimum.onnxruntime import ORTModelForSeq2SeqLM
site-packages/transformers/generation_utils.py:27: FutureWarning: Importing `GenerationMixin` from `src/transformers/generation_utils.py` is deprecated and will be removed in Transformers v5. Import as `from transformers import GenerationMixin` instead.
  FutureWarning,

@regisss
Copy link
Contributor

regisss commented Dec 2, 2022

Could you please update optimum to reflect these changes?

from optimum.onnxruntime import ORTModelForSeq2SeqLM
site-packages/transformers/generation_utils.py:27: FutureWarning: Importing `GenerationMixin` from `src/transformers/generation_utils.py` is deprecated and will be removed in Transformers v5. Import as `from transformers import GenerationMixin` instead.
  FutureWarning,

The PR huggingface/optimum#536 has just been merged and solves this issue.

mpierrau pushed a commit to mpierrau/transformers that referenced this pull request Dec 15, 2022
…gface#20096)

* move generation_*.py src files into generation/*.py

* populate generation.__init__ with lazy loading

* move imports and references from generation.xxx.object to generation.object
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.

6 participants