Skip to content

Conversation

@sgugger
Copy link
Collaborator

@sgugger sgugger commented Nov 10, 2022

What does this PR do?

This PR enables PyTorch 1.13 for Transformers so we can start adding functionality like safer loading with torch.load. Since there are no wheels for torch scatter, this comes at the price of uninstalling torch-scatter. However the PR to move away from this dep and use the PyTorch core ops seems well under way, so skipping the TAPAS tests for now until the PR is merged does not seem like a heavy price to pay (cc @NielsRogge for information).

A couple of tests are still failing, which are all torch FX tests (cc @michaelbenayoun, see failing job here). I'm skipping them and we can fix them next week in a followup PR.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

LGTM

@HuggingFaceDocBuilder
Copy link

HuggingFaceDocBuilder commented Nov 10, 2022

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

@ydshieh
Copy link
Collaborator

ydshieh commented Nov 10, 2022

cc @sgugger , there is a PR

#20149

but we forgot to ping you.

(Oh, I just saw you are aware of that PR)

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.

Finally PT 1.13, thanks @sgugger !

@Bearnardd
Copy link
Contributor

Hi @sgugger :) - One question, you have removed torch-scatter dependency but at the same time you have added decorator require_scatter for test_pt_tf_model_equivalence test. Is that correct or am I just missing something?

@Bearnardd Bearnardd mentioned this pull request Nov 11, 2022
3 tasks
@sgugger
Copy link
Collaborator Author

sgugger commented Nov 14, 2022

The scatter dependency is only removed from the CPU runners on circleCI, it's not removed from the library byt his PR, that's your job ;-) . When testing in our other setups that include the scatter dependency, the tests will be run (until your PR is merged and the dep is removed entirely).

@sgugger sgugger merged commit 9643ecf into main Nov 15, 2022
@sgugger sgugger deleted the pt1-13-no-scatter branch November 15, 2022 16:33
mpierrau pushed a commit to mpierrau/transformers that referenced this pull request Dec 15, 2022
* Try PT1.13 by removing torch scatter

* Skip failing tests

* Style

* Remvoe testing extras for repo utils

* Try with all decorators

* Try to wipe the cache

* Fix all tests?

* Try this way

* Fix comma

* Update to main

* Try with less deps

* Quality
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