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

Use high progress_bar_refresh_rate on Google Colab #4654

Merged
merged 8 commits into from
Nov 23, 2020
Merged

Use high progress_bar_refresh_rate on Google Colab #4654

merged 8 commits into from
Nov 23, 2020

Conversation

Samyak2
Copy link
Contributor

@Samyak2 Samyak2 commented Nov 13, 2020 •
edited
Loading

What does this PR do?

Automatically override progress_bar_refresh_rate when on Google Colab.
For this, I added a IS_COLAB constant under utilities.

The changes in import were by isort.

Fixes #3786

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

Did you have fun?

Yes 😄

@pep8speaks
Copy link

pep8speaks commented Nov 13, 2020 •
edited
Loading

Hello @Samyak2! Thanks for updating this PR.

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

Comment last updated at 2020-11-23 20:02:02 UTC

@codecov
Copy link

codecov bot commented Nov 13, 2020 •
edited
Loading

Codecov Report

Merging #4654 (c5ad5e8) into master (c586e5d) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4654   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         118     118           
  Lines        9020    9021    +1     
======================================
+ Hits         8393    8394    +1     
  Misses        627     627

@ydcjeff ydcjeff added the feature Is an improvement or enhancement label Nov 13, 2020
@ydcjeff ydcjeff added this to the 1.1 milestone Nov 13, 2020
Borda
Borda requested changes Nov 13, 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.

this is just for default values adjustment? or shall we cast anything smaller than 20 in COlab to 20?

pytorch_lightning/trainer/connectors/callback_connector.py Outdated Show resolved Hide resolved
tests/trainer/test_trainer.py Outdated Show resolved Hide resolved
@Samyak2
Copy link
Contributor Author

Samyak2 commented Nov 13, 2020

this is just for default values adjustment? or shall we cast anything smaller than 20 in COlab to 20?

I think if the user sets it manually to a value less than 20, we should not change that. I personally haven't experienced crashes with the default 1 (although the CPU usage is significantly higher for 1 than 20).

Is there a way to find out if the user set it manually?

@rohitgr7
Copy link
Contributor

rohitgr7 commented Nov 13, 2020 •
edited
Loading

what if the user sets it to 1? I don't think it should change it at all. IMO a better way to handle this is generating a simple warning on trainer init, if it's less than some threshold and is on colab.

@Samyak2
Copy link
Contributor Author

Samyak2 commented Nov 14, 2020

That seems better.

Samyak2 and others added 3 commits November 15, 2020 00:15
@Samyak2
Use high refresh rate on Google Colab (#3786)
a15320e 
Automatically override progress_bar_refresh_rate when on Google
Colab. Also added a constant IS_COLAB in utilities to check
whether it is being run in colab or not.
(#3786)
@Samyak2
Show a warning instead of overriding when rate is low on colab
5adc968
@tchaton
Merge branch 'master' into master
052619a
tchaton
tchaton approved these changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Great catch there !

@Samyak2
Change warning to suggestion and move it
89b962d 
Moved warning to configure_progress_bar instead of on_trainer_init
@rohitgr7 rohitgr7 added the ready PRs ready to be merged label Nov 18, 2020
@rohitgr7 rohitgr7 requested a review from Borda November 18, 2020 19:14
@edenlightning
Copy link
Contributor

I actually disagree. I think its super useful as an override, less as warning. Do we have a way to tell if the user overrides?

@rohitgr7
Copy link
Contributor

rohitgr7 commented Nov 18, 2020 •
edited
Loading

I actually disagree. I think its super useful as an override, less as warning. Do we have a way to tell if the user overrides?

I don't think a force override is a good idea since it doesn't crash always with low refresh rate and what if the user wants refresh_rate < 20??

@rohitgr7 rohitgr7 merged commit ccf38ce into Lightning-AI:master Nov 23, 2020
@Borda Borda modified the milestones: 1.1, 1.0.x Nov 24, 2020
Borda pushed a commit that referenced this pull request Nov 24, 2020
@Samyak2 @Borda
Use high progress_bar_refresh_rate on Google Colab (#4654)
c8e83a1 
* Use high refresh rate on Google Colab (#3786)

Automatically override progress_bar_refresh_rate when on Google
Colab. Also added a constant IS_COLAB in utilities to check
whether it is being run in colab or not.
(#3786)

* Show a warning instead of overriding when rate is low on colab

* Change warning to suggestion and move it

Moved warning to configure_progress_bar instead of on_trainer_init

* Apply suggestions from code review

Co-authored-by: Rohit Gupta <[email protected]>

* add a mock test

Co-authored-by: chaton <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>

(cherry picked from commit ccf38ce)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@tchaton tchaton tchaton approved these changes

@Borda Borda Borda approved these changes

@awaelchli awaelchli awaelchli approved these changes

@williamFalcon williamFalcon williamFalcon approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

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

@nateraw nateraw Awaiting requested review from nateraw

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@teddykoker teddykoker Awaiting requested review from teddykoker

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

Successfully merging this pull request may close these issues.

automatically override progress_bar_refresh_rate in google colab
9 participants
@Samyak2 @pep8speaks @rohitgr7 @edenlightning @williamFalcon @awaelchli @Borda @tchaton @ydcjeff