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

Fix natten #22229

Merged
merged 2 commits into from
Mar 17, 2023
Merged

Fix natten #22229

merged 2 commits into from
Mar 17, 2023

Conversation

alihassanijr
Copy link
Contributor

What does this PR do?

The new NATTEN 0.14.5 supports PyTorch 2.0, but also adds an additional
argument to the QK operation to allow optional RPBs.

This ends up failing NATTEN tests.

This commit adds NATTEN back to circleci and adds the arguments to get
it working again.

Reverts #22218.

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?

Who can review?

@sgugger

Misc

Related issues on NATTEN:

SHI-Labs/NATTEN#23

SHI-Labs/NATTEN#19

The new NATTEN 0.14.5 supports PyTorch 2.0, but also adds an additional
argument to the QK operation to allow optional RPBs.

This ends up failing NATTEN tests.

This commit adds NATTEN back to circleci and adds the arguments to get
it working again.
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 17, 2023

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

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Thank you for the (super fast) fix ❤️

@ydshieh ydshieh requested review from sgugger and amyeroberts and removed request for sgugger March 17, 2023 14:50
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.

Thanks for the fixes!

@sgugger sgugger merged commit 3028b20 into huggingface:main Mar 17, 2023
@amyeroberts
Copy link
Collaborator

@alihassanijr Thanks for such a quick fix! Just double checking - is this version of natten compatible with later versions of PyTorch, or just >= 2.x.x?

@alihassanijr
Copy link
Contributor Author

alihassanijr commented Mar 17, 2023

My pleasure, sorry I didn't realize this earlier.
Yes, 0.14.5 still supports torch >= 1.8 and comes with wheels for those:
https://shi-labs.com/natten

So the problem was that we had a pull request a couple of months ago that added an additional argument to one of the C functions. We didn't immediately rebuild and push out a new release at the time.
We did however push out a new build to support PyTorch 2.0, and it included this change, which is why we had to open a PR here as well.

On a different note, we only had to change this here in the first place because we explicitly use the C function calls in the models using NA:

https://github.com/alihassanijr/transformers/blob/6125d62e05aba0bd1f6a53bd3bf44b4d86b58f25/src/transformers/models/dinat/modeling_dinat.py#L350

https://github.com/alihassanijr/transformers/blob/6125d62e05aba0bd1f6a53bd3bf44b4d86b58f25/src/transformers/models/nat/modeling_nat.py#L342

We could try and figure out how we would directly import the nn.Module we typically encourage everyone to use, that way any changes to the signatures would not affect transformers.

NATTEN can in theory support future PyTorch versions without any change required (unless PyTorch changes anything in their ATEN backend like they did with the dispatchers in 1.13, which would require us to work those changes into our CPP backend as well.)
The only slight hitch is that if users want to install NATTEN with wheels (and not have to wait for pip to build it locally), we have to build those on our end and upload them.

I've been wanting to set up CircleCI or Travis so that I wouldn't have to set that up manually every time there's a new PyTorch release, but just haven't found the time to do so yet. But we will to the best of our abilities try to build them upon new PyTorch releases and push them out as soon as possible.

@alihassanijr alihassanijr deleted the fix_natten branch March 21, 2023 18:25
raghavanone pushed a commit to raghavanone/transformers that referenced this pull request Apr 5, 2023
* Add kernel size to NATTEN's QK arguments.

The new NATTEN 0.14.5 supports PyTorch 2.0, but also adds an additional
argument to the QK operation to allow optional RPBs.

This ends up failing NATTEN tests.

This commit adds NATTEN back to circleci and adds the arguments to get
it working again.

* Force NATTEN >= 0.14.5
alihassanijr added a commit to alihassanijr/StyleNAT that referenced this pull request Apr 5, 2023
NATTEN functions take in kernel size as well as dilation since 0.14.6.
The change in signature breaks Hydra-NA, which calls those functions
directly instead of using the nn.Module.

This fixes SHI-Labs#8 .

Refs:

* https://github.com/SHI-Labs/NATTEN/releases/tag/v0.14.6
* https://github.com/SHI-Labs/NATTEN/blob/main/CHANGELOG.md#0146---2023-03-21

Same change in other repositories using NATTEN:
* huggingface/transformers#22229
* huggingface/transformers#22298
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
* Add kernel size to NATTEN's QK arguments.

The new NATTEN 0.14.5 supports PyTorch 2.0, but also adds an additional
argument to the QK operation to allow optional RPBs.

This ends up failing NATTEN tests.

This commit adds NATTEN back to circleci and adds the arguments to get
it working again.

* Force NATTEN >= 0.14.5
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.

None yet

5 participants