Set progressbar refresh rate in Google Colab #5516

awaelchli · 2021-01-14T17:11:11Z

What does this PR do?

Colab example based on this branch:
https://colab.research.google.com/drive/1CX9WOkZPB5HtqOQD0_rHPeaOa9Cca-1g?usp=sharing

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!

rohitgr7 · 2021-01-14T18:07:59Z

so refresh_rate is 1 now just because of the colab thing? The warning is raised during Trainer init itself, so isn't it better than setting it to 1 in other cases?

awaelchli · 2021-01-14T18:21:29Z

so refresh_rate is 1 now just because of the colab thing?

no, the other way around. It is always one (as it was before) except when in colab and except the user sets it to the value they want.
I was requested to remove the warning

rohitgr7 · 2021-01-14T18:24:51Z

so refresh_rate is 1 now just because of the colab thing?

no, the other way around. It is always one (as it was before) except when in colab and except the user sets it to the value they want.
I was requested to remove the warning

ok my bad... got confused it with log_every_n_steps.

codecov · 2021-01-14T18:34:38Z

Codecov Report

Merging #5516 (f776243) into release/1.2-dev (8566f69) will increase coverage by 4%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##           release/1.2-dev   #5516    +/-   ##
================================================
+ Coverage               89%     93%    +4%     
================================================
  Files                  153     153            
  Lines                10813   11088   +275     
================================================
+ Hits                  9615   10261   +646     
+ Misses                1198     827   -371

pytorch_lightning/trainer/trainer.py

Co-authored-by: Nicki Skafte <[email protected]>

Borda · 2021-01-15T08:16:23Z

pytorch_lightning/trainer/trainer.py

@@ -219,7 +219,7 @@ def __init__(
            process_position: orders the progress bar when running multiple models on same machine.

            progress_bar_refresh_rate: How often to refresh progress bar (in steps). Value ``0`` disables progress bar.
-                Ignored when a custom callback is passed to :paramref:`~Trainer.callbacks`.
+                Ignored when a custom progress bar is passed to :paramref:`~Trainer.callbacks`. Default: 1


this is confusing as you say default but in code is none, so maybe tell that it is set internally?

what should I do?
we have other trainer args set as None and they are internally set to the defaults

yes, but usually this is only done for one of the following reasons:

1.) We need to check for some type before setting the default (for example to raise deprecation warnings)
2.) They are mutable otherwise.
3.) They depend on other parameters
4.) They are really hard to parse/set correctly

In general the 2) is most common

Ok, here we are in case 3, because the default depends on an environment variable.
Then I will just update the docstring, or do you request other changes?

updated docstring, do you like it better?

justusschock · 2021-01-18T07:49:05Z

pytorch_lightning/trainer/connectors/callback_connector.py

-                UserWarning
-            )
+    def configure_progress_bar(self, refresh_rate=None, process_position=0):
+        if os.getenv('COLAB_GPU') and refresh_rate is None:


is this env always set for colab? also on TPU?

it's '0' otherwise

@rohitgr7 not according to my quick testing.
https://colab.research.google.com/drive/1_W9Dk-cxbBZmg6R2tNrqu3gzpjcEPjdb?usp=sharing
I get True back on CPU, GPU, and TPU even after restarting the runtime.

Maybe it's changed now. Last time I checked it was '1' for GPU and '0' for others.

justusschock · 2021-01-18T07:51:02Z

pytorch_lightning/trainer/trainer.py

@@ -219,7 +219,7 @@ def __init__(
            process_position: orders the progress bar when running multiple models on same machine.

            progress_bar_refresh_rate: How often to refresh progress bar (in steps). Value ``0`` disables progress bar.
-                Ignored when a custom callback is passed to :paramref:`~Trainer.callbacks`.
+                Ignored when a custom progress bar is passed to :paramref:`~Trainer.callbacks`. Default: 1


yes, but usually this is only done for one of the following reasons:

1.) We need to check for some type before setting the default (for example to raise deprecation warnings)
2.) They are mutable otherwise.
3.) They depend on other parameters
4.) They are really hard to parse/set correctly

tchaton

LGTM !

awaelchli added 4 commits January 14, 2021 17:58

refresh

3dbf7ae

add tests

521ac0d

docs

3ae1304

chlog

9995c8d

awaelchli added design Includes a design discussion callback feature Is an improvement or enhancement labels Jan 14, 2021

awaelchli added this to the 1.2 milestone Jan 14, 2021

chlog

e0c9171

awaelchli added the docs Documentation related label Jan 14, 2021

awaelchli marked this pull request as ready for review January 14, 2021 17:56

awaelchli requested review from Borda, edenlightning, justusschock, SeanNaren, tchaton and williamFalcon as code owners January 14, 2021 17:56

rohitgr7 approved these changes Jan 14, 2021

View reviewed changes

awaelchli added the ready PRs ready to be merged label Jan 14, 2021

SkafteNicki approved these changes Jan 14, 2021

View reviewed changes

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

Update pytorch_lightning/trainer/trainer.py

534352b

Co-authored-by: Nicki Skafte <[email protected]>

awaelchli linked an issue Jan 15, 2021 that may be closed by this pull request

Improve handling progressbar refresh rate in Google Colab #5515

Closed

ydcjeff approved these changes Jan 15, 2021

View reviewed changes

Borda enabled auto-merge (squash) January 15, 2021 08:14

Borda approved these changes Jan 15, 2021

View reviewed changes

Merge branch 'release/1.2-dev' into feature/colab-refresh

1496c75

justusschock approved these changes Jan 18, 2021

View reviewed changes

update docstring

4918fb3

tchaton approved these changes Jan 19, 2021

View reviewed changes

Merge branch 'release/1.2-dev' into feature/colab-refresh

f776243

tchaton disabled auto-merge January 19, 2021 11:51

tchaton enabled auto-merge (squash) January 19, 2021 11:51

Merge branch 'release/1.2-dev' into feature/colab-refresh

e4f8b43

tchaton disabled auto-merge January 19, 2021 15:45

tchaton enabled auto-merge (squash) January 19, 2021 15:45

tchaton merged commit 24462dc into release/1.2-dev Jan 19, 2021

tchaton deleted the feature/colab-refresh branch January 19, 2021 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set progressbar refresh rate in Google Colab #5516

Set progressbar refresh rate in Google Colab #5516

awaelchli commented Jan 14, 2021 •

edited

Loading

rohitgr7 commented Jan 14, 2021

awaelchli commented Jan 14, 2021

rohitgr7 commented Jan 14, 2021

codecov bot commented Jan 14, 2021 •

edited

Loading

Borda Jan 15, 2021

awaelchli Jan 18, 2021

justusschock Jan 18, 2021

Borda Jan 18, 2021

awaelchli Jan 18, 2021

awaelchli Jan 18, 2021

justusschock Jan 18, 2021

rohitgr7 Jan 18, 2021

awaelchli Jan 18, 2021

rohitgr7 Jan 18, 2021

justusschock Jan 18, 2021

tchaton left a comment

Set progressbar refresh rate in Google Colab #5516

Set progressbar refresh rate in Google Colab #5516

Conversation

awaelchli commented Jan 14, 2021 • edited Loading

What does this PR do?

Before submitting

PR review

rohitgr7 commented Jan 14, 2021

awaelchli commented Jan 14, 2021

rohitgr7 commented Jan 14, 2021

codecov bot commented Jan 14, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaton left a comment

Choose a reason for hiding this comment

awaelchli commented Jan 14, 2021 •

edited

Loading

codecov bot commented Jan 14, 2021 •

edited

Loading