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.

Sign up for GitHub

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

Jump to bottom

IPU Integration 5/5 #7867

Merged
merged 69 commits into from
Jun 11, 2021
Merged

IPU Integration 5/5 #7867

merged 69 commits into from
Jun 11, 2021

Conversation

SeanNaren
Copy link
Contributor

@SeanNaren SeanNaren commented Jun 7, 2021 •
edited
Loading

What does this PR do?

Adds the IPU Accelerator to PyTorch Lightning!

I have created a new folder called ipu_examples to store a simple mnist.py example, however it might be best if this moves elsewhere where we can add more examples.

Overall we only support 1 optimizer, and most of the other features.

We're working with the GraphCore engineers to support logging/metrics, as they require results calculated from the device which is current unsupported. They already are working on this, and I'll make an issue for us to track

Still gives people who want to test this out something to work with if they have access to IPUs :)

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

SeanNaren added 30 commits February 22, 2021 17:10
Initial changes
f75f445
Merge branch 'master' into wip/acc
a4a60c2 
# Conflicts:
#	pytorch_lightning/trainer/connectors/accelerator_connector.py
Add broken example for now
dc9744b
Fix reference
931bb74
Merge branch 'master' into wip/acc
9b18baf
Fix format
c617f02
Code runs
522a81f
Fixes
0c00360
Merge branch 'master' into wip/acc
30c1370
Clear up files
adbdb2a
Add tests, helpers, fixes
3e733af
Small cleanups
a51f23e
Refactors based on review
be7de87
Swap to special tests
83c8a79
Add special tests
a6018e5
Add source
0e71bbe
Cleanups
6e38bd1
Add logic to attach/detach model from devices
526383f
Fixes for tests
e18039c
Fixes for tests
2e43fee
Move earlier
53d31a0
Cleanups
6241432
Add check for nvcc
d249a13
Add tests, cleanups
d08cf39
Fix errors
7469744
fix
f474c5b
Try condition
e178d5f
Add missing annotation
c704920
Clearer
c54a216
Clearer message
2ea1766
@SeanNaren SeanNaren enabled auto-merge (squash) June 11, 2021 13:03
tchaton
tchaton approved these changes Jun 11, 2021
View reviewed changes
Copy link
Contributor

@tchaton tchaton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM ! Incredible work !

pytorch_lightning/plugins/training_type/ipu.py Outdated Show resolved Hide resolved
pytorch_lightning/plugins/training_type/ipu.py Show resolved Hide resolved
awaelchli
awaelchli approved these changes Jun 11, 2021
View reviewed changes
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

will it impact coverage? maybe better to exclude the files for now until we run it also on the IPU CI?

pl_examples/ipu_examples/mnist.py Outdated Show resolved Hide resolved
pytorch_lightning/plugins/training_type/ipu.py Show resolved Hide resolved
pytorch_lightning/plugins/training_type/ipu.py Show resolved Hide resolved
pytorch_lightning/plugins/training_type/ipu.py Outdated Show resolved Hide resolved
pytorch_lightning/plugins/training_type/ipu.py Show resolved Hide resolved
pytorch_lightning/trainer/connectors/accelerator_connector.py Outdated Show resolved Hide resolved
tests/accelerators/test_ipu.py Outdated Show resolved Hide resolved
@awaelchli
Copy link
Contributor

@SeanNaren and Graphcore team, really nice work

@SeanNaren SeanNaren disabled auto-merge June 11, 2021 14:13
kaushikb11
kaushikb11 approved these changes Jun 11, 2021
View reviewed changes
Copy link
Contributor

@kaushikb11 kaushikb11 left a comment

Choose a reason for hiding this comment

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

Awesome!

@SeanNaren SeanNaren enabled auto-merge (squash) June 11, 2021 14:35
@SeanNaren SeanNaren merged commit 96433d0 into master Jun 11, 2021
@SeanNaren SeanNaren deleted the wip/acc branch June 11, 2021 15:07
speediedan pushed a commit to speediedan/lightning that referenced this pull request Jun 11, 2021
@kaushikb11 @tchaton
IPU Integration 5/5 (Lightning-AI#7867)
4c0baa3 
* Initial changes

* Add broken example for now

* Fix reference

* Fix format

* Code runs

* Fixes

* Clear up files

* Add tests, helpers, fixes

* Small cleanups

* Refactors based on review

* Swap to special tests

* Add special tests

* Add source

* Cleanups

* Add logic to attach/detach model from devices

* Fixes for tests

* Fixes for tests

* Move earlier

* Cleanups

* Add check for nvcc

* Add tests, cleanups

* Fix errors

* fix

* Try condition

* Add missing annotation

* Clearer

* Clearer message

* Fix variable

* Cleanups

* Add comment

* CHANGELOG.md

* Add simple selection test

* Remove special=True to see what happens

* Fix test

* Update tests/accelerators/test_ipu.py

Co-authored-by: Kaushik B <[email protected]>

* Convert ipu_cores -> ipus

* Add typing, fail earlier

* simplify precision

* Add test, add helper

* fix accum

* Update pytorch_lightning/plugins/training_type/ipu.py

Co-authored-by: thomas chaton <[email protected]>

* Use stages

* Make sure warning message returned

* thorw error

* Add more tests, use fs

* add comment

* Clean

* Address feedback, add IPU tests

* Fixes

* Fix signature

* Add types

* Remove autoround

* Add docstring

* ipu_cores -> ipus

* Add test, remove unnecessary precision set

* Add optimizer test

* Add precision back with test

* Address code review

* Change to probs

* Move some of the asserts earlier

Co-authored-by: Kaushik B <[email protected]>
Co-authored-by: thomas chaton <[email protected]>
@SeanNaren SeanNaren mentioned this pull request Jun 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davors72 davors72 davors72 left review comments

@ananthsub ananthsub ananthsub left review comments

@kaushikb11 kaushikb11 kaushikb11 approved these changes

@tchaton tchaton tchaton approved these changes

@awaelchli awaelchli awaelchli approved these changes

@Borda Borda Awaiting requested review from Borda Borda is a code owner

@carmocca carmocca Awaiting requested review from carmocca

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees

@SeanNaren SeanNaren

Labels
3rd party Related to a 3rd-party
Projects
None yet
Milestone
pl:1.4
Development

Successfully merging this pull request may close these issues.
7 participants
@SeanNaren @Borda @awaelchli @ananthsub @davors72 @tchaton @kaushikb11