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

feat(trainer): add enable_benchmarking option #803

Merged
merged 17 commits into from
Feb 25, 2020
Merged

feat(trainer): add enable_benchmarking option #803

merged 17 commits into from
Feb 25, 2020

Conversation

Ir1d
Copy link
Contributor

@Ir1d Ir1d commented Feb 8, 2020

closes #370
I don't think we need tests for this option for now. Tests can be added when this flag includes more options.

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
  • Did you read the contributor guideline?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes #370 (issue).

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 🙃

@Borda Borda added the feature Is an improvement or enhancement label Feb 8, 2020
@Borda Borda added this to the 0.6.1 milestone Feb 8, 2020
@Borda
Copy link
Member

Borda commented Feb 14, 2020

@Ir1d I wee that Will made some changes, could you check it and push so it triggers the new CI

@Ir1d
Copy link
Contributor Author

Ir1d commented Feb 14, 2020

@Borda you can re-trigger the build on travis page since you are a member of the org. I don't have other things to push in this PR

@Borda
Copy link
Member

Borda commented Feb 14, 2020

well the point is that we moved to GitHub CI and that need to add a commit to update all these things lol

Borda
Borda requested changes Feb 14, 2020
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

almost there, keep rolling ;]

@pep8speaks
Copy link

pep8speaks commented Feb 16, 2020 •
edited
Loading

Hello @Ir1d! Thanks for updating this PR.

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

Comment last updated at 2020-02-25 13:52:46 UTC

@Ir1d
Copy link
Contributor Author

Ir1d commented Feb 16, 2020

The checks are passed and now ready to land.

@williamFalcon
Copy link
Contributor

@Ir1d this needs a test. Just run a simple model for a few steps with this flag enabled.

Tests are not just for functionality but also for test coverage.

Borda
Borda approved these changes Feb 17, 2020
View reviewed changes
Copy link
Member

@Borda Borda 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 🚀 but the test shall be added lol

Borda
Borda approved these changes Feb 18, 2020
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@Borda Borda added the ready PRs ready to be merged label Feb 18, 2020
@williamFalcon
Copy link
Contributor

@Ir1d mind doing a rebase?

@Borda
Copy link
Member

Borda commented Feb 24, 2020

@williamFalcon ^^

@awaelchli
Copy link
Contributor

awaelchli commented Feb 24, 2020 •
edited
Loading

@Ir1d What's the purpose of the test you wrote? It has nothing to do with the benchmarking option and just tests if the trainer completes. Or am i wrong?

Maybe assert the value of torch.backends.cudnn.benchmark before and after fit?

@Ir1d
Copy link
Contributor Author

Ir1d commented Feb 24, 2020

@awaelchli Feel free to enlighten me on how to test this benchmarking option. The tests are added for test coverage, as suggested above by william.

@awaelchli
Copy link
Contributor

awaelchli commented Feb 24, 2020 •
edited
Loading

@Ir1d Ok, I see. Maybe just assert the value of torch.backends.cudnn.benchmark before and after fit? If trainer option is true, we would expect it to stay true during fit.

@Ir1d
Copy link
Contributor Author

Ir1d commented Feb 24, 2020

Thank you for your suggestions @awaelchli , I've added the assert statements.

@Borda
Copy link
Member

Borda commented Feb 24, 2020

@Ir1d @awaelchli I love this spirit! ⚡

@awaelchli
Copy link
Contributor

nice, great addition! GPU tests pass on my machine.

@williamFalcon williamFalcon merged commit be83e75 into Lightning-AI:master Feb 25, 2020
@awaelchli
Copy link
Contributor

I think @Borda made changes in #936 and this pr was not rebased after that, so it broke tests.

@awaelchli
Copy link
Contributor

nvm, I think merge of #889 fixed it.

tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Apr 3, 2020
@Ir1d @williamFalcon @tullie
feat(trainer): add enable_benchmarking option (Lightning-AI#803)
f456744 
* feat(trainer): add enable_benchmarking option

closes Lightning-AI#370

* Update trainer.py

* Update trainer.py

* Update trainer.py

* Update trainer.py

* Update trainer.py

* Update trainer.py

* Update trainer.py

* Update trainer.py

* add test

* try to make the lint work

* fix typo

* add test, verify torch.backends.cudnn.benchmark

* make lint happy

* make lint happy

Co-authored-by: William Falcon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda approved these changes

Assignees
No one assigned
Labels
feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Milestone
pl:0.7
Development

Successfully merging this pull request may close these issues.

Support to turn on cudnn benchmark mode
5 participants
@Ir1d @Borda @pep8speaks @williamFalcon @awaelchli