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 parallelization support for T5EncoderModel #9082

Merged
merged 6 commits into from
Dec 14, 2020

Conversation

agemagician
Copy link
Contributor

What does this PR do?

Extend T5EncoderModel to support model parallization across different GPUs.

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?

T5: @patrickvonplaten

@LysandreJik
Copy link
Member

Very cool! Could you also enable the parallelization tests for these models? You can check how it was done in the initial model parallel PR, here's the commit related to the tests. You can just add the T5EncoderModel to the all_parallelizable_model_classes attribute of the T5ModelTester class.

@agemagician
Copy link
Contributor Author

Very cool! Could you also enable the parallelization tests for these models? You can check how it was done in the initial model parallel PR, here's the commit related to the tests. You can just add the T5EncoderModel to the all_parallelizable_model_classes attribute of the T5ModelTester class.

Thanks for the tip.
Done, please let me know if anything else is needed from my side.

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.

This LGTM. Looking into it it seems we have an error in T5Stask as it is creating the device map with torch.cuda.device_count(), rather than the range of that value like you're doing it here. Since we're always passing the device map to T5Stack (it's never used as a standalone model) we don't see it, but it doesn't seem correct.

What do you think? If you think this is true, do you mind adding a range in T5Stack so that we can merge it together? Thanks!

@LysandreJik
Copy link
Member

Also it would be great if you could run make style && make quality or make fixup to solve the quality issues.

@agemagician
Copy link
Contributor Author

This LGTM. Looking into it it seems we have an error in T5Stask as it is creating the device map with torch.cuda.device_count(), rather than the range of that value like you're doing it here. Since we're always passing the device map to T5Stack (it's never used as a standalone model) we don't see it, but it doesn't seem correct.

What do you think? If you think this is true, do you mind adding a range in T5Stack so that we can merge it together? Thanks!

Yes, you are correct, T5Stack should also use range. Since "get_device_map" function apply len to it .
I have updated T5Stack using range.

@agemagician
Copy link
Contributor Author

Also it would be great if you could run make style && make quality or make fixup to solve the quality issues.

Done and passed the code quality testing.

@LysandreJik
Copy link
Member

Wonderful!

@LysandreJik LysandreJik merged commit a9c8bff into huggingface:master Dec 14, 2020
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

2 participants