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

TF: Unpack model inputs through a decorator #15907

Merged
merged 10 commits into from
Mar 10, 2022

Conversation

gante
Copy link
Member

@gante gante commented Mar 3, 2022

What does this PR do?

Unpacks TF model inputs through a decorator, improving code clarity. To be read with issue #15908, which holds the description of the problem, the proposed solution, and future plan.

Closes #15908

How to review: start with modeling_tf_utils.py, and then check the changes in the models. I've run RUN_SLOW=1 py.test -vv tests/model_name/test_modeling_tf_model_name.py for all involved models, and the changes were applied to BERT (plus its copy-linked architectures) and Speech2Text, showing that it works for multiple modalities.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 3, 2022

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

@gante gante changed the title TF: Unpack inputs through decorator TF: Unpack inputs through a decorator Mar 3, 2022
@gante gante changed the title TF: Unpack inputs through a decorator TF: Unpack model inputs through a decorator Mar 3, 2022
@gante gante marked this pull request as ready for review March 8, 2022 16:22
@gante
Copy link
Member Author

gante commented Mar 8, 2022

As suggested by @sgugger in #15908, I've opened the PR :)

Comment on lines -837 to -838
if "input_ids" in inputs:
inputs["input_features"] = inputs.pop("input_ids")
Copy link
Member Author

@gante gante Mar 8, 2022

Choose a reason for hiding this comment

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

No more need for this ugly work-around for non-NLP models 😎

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.

So nice and elegant! Love how it makes the modeling code so much more readable!

@LysandreJik
Copy link
Member

This looks great! Also pinging @patrickvonplaten for review as it's quite central

@LysandreJik
Copy link
Member

Thanks for running the tests on it. It is way more readable like that indeed.

@patrickvonplaten
Copy link
Contributor

Looks very nice! Thanks for cleaning this up

1 similar comment
@patrickvonplaten
Copy link
Contributor

Looks very nice! Thanks for cleaning this up

@gante
Copy link
Member Author

gante commented Mar 10, 2022

It seems like everyone is in agreement :) @Rocketknight1 can you have a final look, and approve if you agree with the change?

Copy link
Member

@Rocketknight1 Rocketknight1 left a comment

Choose a reason for hiding this comment

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

This looks great to me! Is the plan to wait and see how things go with BERT, and assuming no problems to repeat those edits across all model classes?

@gante
Copy link
Member Author

gante commented Mar 10, 2022

Is the plan to wait and see how things go with BERT, and assuming no problems to repeat those edits across all model classes?

Yeah! I'm also going to open an issue with the good first issue tag and try to get contributors in to replicate the change over other models. After a month or so, I will update any missing model.

@gante gante merged commit b7018ab into huggingface:master Mar 10, 2022
@gante gante deleted the tf_unpacked_inputs branch March 10, 2022 13:31
@gante gante mentioned this pull request Mar 10, 2022
47 tasks
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.

RFC -- TF: unpack model inputs through a decorator
6 participants