protect progress bar callback #1855
Conversation
|
Hello @awaelchli! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-05-24 23:38:03 UTC |
| @@ -109,20 +107,22 @@ def configure_early_stopping(self, early_stop_callback): | |||
| self.early_stop_callback = early_stop_callback | |||
| self.enable_early_stop = True | |||
|
|
|||
| def configure_progress_bar(self): | |||
| def configure_progress_bar(self, refresh_rate=1, process_position=0): | |||
| progress_bars = [c for c in self.callbacks if isinstance(c, ProgressBarBase)] | |||
| if len(progress_bars) > 1: | |||
| raise MisconfigurationException( | |||
There was a problem hiding this comment.
I ques we can allow multiple in future, just check that each is different, meaning another monitor event, frequency, etc.
There was a problem hiding this comment.
yes agree that would be good. the only reason why we currently have a limit of 1 is because Trainer needs to be able to disable the progress bar temporarily, for example in LRFinder.
| @@ -754,6 +749,10 @@ def num_gpus(self) -> int: | |||
| def data_parallel(self) -> bool: | |||
| return self.use_dp or self.use_ddp or self.use_ddp2 | |||
|
|
|||
| @property | |||
| def progress_bar_callback(self): | |||
| return self._progress_bar_callback | |||
There was a problem hiding this comment.
not sure how much it protects as it handles pointer to the same object so edit in the return will appear in the original one...
There was a problem hiding this comment.
yes I think that's fine. the goal was to make the reference read-only so that you can't change the reference to anything other than a progress bar.
Codecov Report
@@ Coverage Diff @@
## master #1855 +/- ##
======================================
- Coverage 88% 88% -0%
======================================
Files 74 74
Lines 4621 4617 -4
======================================
- Hits 4050 4046 -4
Misses 571 571 |
Before submitting
What does this PR do?
Follow up to #1450. The progress bar callback is supposed to be passed in via the
callbacksTrainer arg if overriding the default is desired and it should not be mutable after initialization of the Trainer.Someone on slack already commented that the
progress_bar_callbackTrainer arg has no effect. It was a left-over and I did not remove it in the original PR.Since it was not documented at all, this should not break anybody's code.
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 🙃