fix TPU parsing and TPU tests

[lezwon]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1246

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[pep8speaks]

Hello @lezwon! Thanks for updating this PR.

• In the file pytorch_lightning/trainer/distrib_parts.py:

Line 359:120: E501 line too long (124 > 119 characters)
Line 527:51: W504 line break after binary operator
Line 528:29: W504 line break after binary operator

Comment last updated at 2020-06-23 15:34:18 UTC

[codecov]

Codecov Report

Merging #2094 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #2094   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          70      70           
  Lines        5501    5516   +15     
======================================
+ Hits         4834    4849   +15     
  Misses        667     667           

[awaelchli]

what's the best way to test this? should we make a colab and run the tests from there manually?

[lezwon]

what's the best way to test this? should we make a colab and run the tests from there manually?

@awaelchli yea. I tested this on Kaggle by running the tests manually. Notebook: https://www.kaggle.com/lezwon/pytorch-lightning-tpu-tests

[awaelchli]

Nice use of test parameterization! ❤️
Thanks for contributing these tests.

In the tests that just run the trainer I would at least add a test that the model is on the correct device, otherwise the test would pass if the model runs on cpu.
When we do a test for a feature, it is not enough to just do this:

trainer = Trainer(feature=True)
result = trainer.fit()
assert result

Because that only tests that the feature does not throw an error, but it does not test that the feature actually leads to the desired side effects.

[lezwon]

Nice use of test parameterization! ❤️
Thanks for contributing these tests.

In the tests that just run the trainer I would at least add a test that the model is on the correct device, otherwise the test would pass if the model runs on cpu.
When we do a test for a feature, it is not enough to just do this:

trainer = Trainer(feature=True)
result = trainer.fit()
assert result

Because that only tests that the feature does not throw an error, but it does not test that the feature actually leads to the desired side effects.

Gotcha. Thanks for the review. I'll add the suggested changes. 😊

[Borda]

this will help us very much with checking any TOU edit and increase code coverage!

what's the best way to test this? should we make a colab and run the tests from there manually?

just need to add TPU machine to the test space @williamFalcon @luiscape

cc: @PyTorchLightning/core-contributors

[lezwon]

@Borda I've fixed the issue. It had to do when parsing CLI arguments. I will try to add tests for it.

[williamFalcon]

@lezwon waiting on @Borda to finish adding TPU tests. once we do, we can merge this

[mergify]

This pull request is now in conflict... :(

[Borda]

@lezwon waiting on @Borda to finish adding TPU tests. once we do, we can merge this

I would merge it as it is now as it doe not affect any actual functionality/testing and then make a follow-up PR if we find that we have a bug somewhere...

[Borda]

[williamFalcon]

done!