Skip to content

Conversation

@lezwon
Copy link
Contributor

@lezwon lezwon commented Jan 9, 2021

What does this PR do?

Fixes #2705

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
  • Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 🙃

@lezwon lezwon added bug Something isn't working accelerator: tpu Tensor Processing Unit labels Jan 9, 2021
@lezwon lezwon self-assigned this Jan 9, 2021
@lezwon lezwon changed the title [WIP] [WIP] Add option for weight tying on TPU's Jan 9, 2021
@lezwon lezwon force-pushed the bugfix/2705_weights_tying branch from 7dd0f7e to 0e6d43b Compare January 9, 2021 18:34
@pep8speaks
Copy link

pep8speaks commented Jan 9, 2021

Hello @lezwon! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-17 23:42:15 UTC

@codecov
Copy link

codecov bot commented Jan 10, 2021

Codecov Report

Merging #5441 (3605c0b) into master (6a409c7) will decrease coverage by 2%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #5441    +/-   ##
=======================================
- Coverage      93%     91%    -2%     
=======================================
  Files         160     160            
  Lines       11358   11720   +362     
=======================================
+ Hits        10569   10648    +79     
- Misses        789    1072   +283     

@lezwon lezwon modified the milestones: 1.1.x, 1.2.x Jan 10, 2021
@lezwon lezwon changed the base branch from master to release/1.2-dev January 10, 2021 07:11
@lezwon lezwon changed the base branch from release/1.2-dev to master January 10, 2021 07:12
@lezwon lezwon modified the milestones: 1.2.x, 1.1.x Jan 10, 2021
@lezwon lezwon force-pushed the bugfix/2705_weights_tying branch 2 times, most recently from 2275b35 to 5a00c88 Compare January 10, 2021 11:19
@lezwon lezwon marked this pull request as ready for review January 10, 2021 11:34
@lezwon lezwon changed the title [WIP] Add option for weight tying on TPU's Add option for weight tying on TPU's Jan 10, 2021
@rohitgr7
Copy link
Contributor

this should go to release/1.2-dev.

@lezwon lezwon force-pushed the bugfix/2705_weights_tying branch from 5a00c88 to eebc4de Compare January 10, 2021 12:14
@lezwon lezwon changed the base branch from master to release/1.2-dev January 10, 2021 12:14
@Borda Borda modified the milestones: 1.1.x, 1.2 Feb 8, 2021
@edenlightning edenlightning modified the milestones: 1.2, 1.2.x Feb 8, 2021
Base automatically changed from release/1.2-dev to master February 11, 2021 14:32
@lexierule lexierule requested a review from carmocca as a code owner February 11, 2021 14:32
@carmocca carmocca removed the ready PRs ready to be merged label Feb 12, 2021
@carmocca
Copy link
Contributor

Removed ready-to-go since this is now blocked by the accelerator refactor

@Borda Borda changed the title Add option for weight tying on TPU's [blocked by #5743] Add option for weight tying on TPU's Feb 12, 2021
@edenlightning edenlightning changed the title [blocked by #5743] Add option for weight tying on TPU's Add option for weight tying on TPU's Feb 16, 2021
@mergify mergify bot removed the has conflicts label Feb 17, 2021
@Borda Borda added ready PRs ready to be merged and removed _Will labels Feb 17, 2021
@Borda Borda disabled auto-merge February 17, 2021 23:32
@tchaton tchaton enabled auto-merge (squash) February 17, 2021 23:45
@tchaton tchaton requested a review from carmocca February 17, 2021 23:45
@tchaton tchaton merged commit d2cd7cb into master Feb 18, 2021
@tchaton tchaton deleted the bugfix/2705_weights_tying branch February 18, 2021 00:03
@Borda Borda modified the milestones: 1.2.x, 1.2 Feb 18, 2021
post_layer_count = len(list(self.parameters()))

if not pre_layer_count == post_layer_count:
rank_zero_warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

@lezwon Could you help me out on what this check means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaushikb11 if the layer weights are not tied while on tpu, then the layer count on the tpu will show as no_of_layers + 1, as the xla library will make a copy of the layer weights. This check makes sure the layer count matches after moving the model to the device. 😊👍

Copy link
Contributor

@kaushikb11 kaushikb11 Apr 20, 2021

Choose a reason for hiding this comment

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

@lezwon Yup, but I don't think it's necessary to do parameter_validation for every module to call? wdyt?

Just a fyi: Currently I am doing this https://github.com/PyTorchLightning/pytorch-lightning/blob/tpu_spawn_added/pytorch_lightning/plugins/training_type/tpu_spawn.py#L172, by using xla's MpModelWrapper. This will make the test fail. Maybe I could add this check specific to TPU acclerators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you could maybe add it specifically to TPU accelerators. Also do check it works well when using xla with GPU's.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accelerator: tpu Tensor Processing Unit bug Something isn't working feature Is an improvement or enhancement ready PRs ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Weight tying is broken on TPUs leading to silent errors