Skip to content

Conversation

@Bearnardd
Copy link
Contributor

What does this PR do?

Changes the usage of scatter from torch_scatter to PyTorch's scatter thus removing the dependency on third party library

  • remove torch_scatter dependency
  • update _segment_reduce function in order to work with PyTorch's scatter
  • update test case test_reduce_sum_vectorized

Fixes # (issue)
#20101

Who can review?

@NielsRogge

@Bearnardd Bearnardd changed the title Fix tapas scatter 2 Fix tapas scatter Nov 9, 2022
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

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.

Thanks a lot for fixing!

Could you also remove all "scatter" mentions in the code base? https://github.com/search?q=repo%3Ahuggingface%2Ftransformers%20scatter&type=code

@ydshieh ydshieh mentioned this pull request Nov 10, 2022
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.

Thanks

@ydshieh ydshieh requested a review from sgugger November 10, 2022 20:54
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 again, @Bearnardd !

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 a lot for migrating to PyTorch!
Could you just remove the file src/transformers/utils/dummy_scatter_objects.py as it won't be necessary anymore.

@Bearnardd
Copy link
Contributor Author

Hi @sgugger - Sure, I will remove it as a part of removing all "scatter" mentions requested by @NielsRogge.

@Bearnardd
Copy link
Contributor Author

Hi @NielsRogge - could you tell me what is the difference between require_scatter and require_torch_scatter in transformers/src/transformers/testing_utils.py since they are calling the same thing.

@NielsRogge
Copy link
Contributor

Good question, you can remove both ;)

@Bearnardd
Copy link
Contributor Author

@NielsRogge - I have removed "scatter" mentions from the code base. It will be good to double check the changes :). I have not changed .circleci/create_circleci_config.py since removing it is a part of the #20168 PR.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@sgugger
Copy link
Collaborator

sgugger commented Nov 14, 2022

Thanks for all the work! Will rebase my PR on yours to finish the job :-)

@sgugger sgugger merged commit 78a471f into huggingface:main Nov 14, 2022
mpierrau pushed a commit to mpierrau/transformers that referenced this pull request Dec 15, 2022
* First draft

* Remove scatter dependency

* Add require_torch

* update vectorized sum test, add clone call

* remove artifacts

* fix style

* fix style v2

* remove "scatter" mentions from the code base

* fix isort error

Co-authored-by: Niels Rogge <nielsrogge@Nielss-MacBook-Pro.local>
Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
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.

5 participants