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

Add TF DeiT implementation #17806

Merged
merged 39 commits into from
Jul 13, 2022
Merged

Conversation

amyeroberts
Copy link
Collaborator

What does this PR do?

Adds the TF implementations of DeiT

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 21, 2022

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

@NielsRogge
Copy link
Contributor

Please also incorporate the updates of #17731

@amyeroberts
Copy link
Collaborator Author

Please also incorporate the updates of #17731

@NielsRogge Will do! I originally added, but it resulted in lots of changes because of all the Copied From statements, so will wait until your PR is merged and those updates finalised.

@amyeroberts amyeroberts marked this pull request as ready for review June 24, 2022 21:53
@amyeroberts amyeroberts changed the title [WIP] Add tf deit Add TF DeiT implementation Jun 24, 2022
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.

I couldn't see any issues with the ported TF code anywhere! (Except my usual nitpicks that people are using tf.multiply and tf.matmul instead of just * and @, but that's purely cosmetic and copied from another file anyway)

Also, tests seem comprehensive, so this PR looks good to me!

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.

Deferring to @NielsRogge and @gante here as it's Vision and TF. Happy to do a final review though if you want :-)

@patrickvonplaten
Copy link
Contributor

@sgugger could you maybe give it a quick review as it's vision? (otherwise happy to do it if you're busy)

)


@dataclass
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually I add model output classes at the top of the modeling file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed - it's much nicer. I've moved it to the top :)

Copy link
Contributor

@NielsRogge NielsRogge left a comment

Choose a reason for hiding this comment

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

LGTM! Very clean!

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.

Very clean, thanks a lot for porting this model to TensorFlow!

tests/models/deit/test_modeling_tf_deit.py Outdated Show resolved Hide resolved
@amyeroberts amyeroberts merged commit 8581a79 into huggingface:main Jul 13, 2022
@amyeroberts amyeroberts deleted the add-tf-deit branch July 13, 2022 17:04
viclzhu pushed a commit to viclzhu/transformers that referenced this pull request Jul 18, 2022
* Initial TF DeiT implementation

* Fix copies naming issues

* Fix up + docs

* Properly same main layer

* Name layers properly

* Initial TF DeiT implementation

* Fix copies naming issues

* Fix up + docs

* Properly same main layer

* Name layers properly

* Fixup

* Fix import

* Fix import

* Fix import

* Fix weight loading for tests whilst not on hub

* Add doc tests and remove to_2tuple

* Add back to_2tuple
Removing to_2tuple results in many downstream changes needed because of the copies checks

* Incorporate updates in Improve vision models huggingface#17731 PR

* Don't hard code num_channels

* Copy PyTorch DeiT embeddings and remove pytorch operations with mask

* Fix patch embeddings & tidy up

* Update PixelShuffle to move logic into class layer

* Update doc strings - remove PT references

* Use NHWC format in internal layers

* Fix up

* Use linear activation layer

* Remove unused import

* Apply suggestions from code review

Co-authored-by: Sylvain Gugger <[email protected]>
Co-authored-by: NielsRogge <[email protected]>

Co-authored-by: NielsRogge <[email protected]>
Co-authored-by: Sylvain Gugger <[email protected]>

* Move dataclass to top of file

* Remove from_pt now weights on hub

* Fixup

Co-authored-by: NielsRogge <[email protected]>
Co-authored-by: Sylvain Gugger <[email protected]>
Co-authored-by: Amy Roberts <[email protected]>
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