remove trainer hidden state | sanity refactor [1 / n] #7437
Conversation
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## master #7437 +/- ##
=======================================
- Coverage 92% 88% -4%
=======================================
Files 199 199
Lines 13030 13047 +17
=======================================
- Hits 11989 11421 -568
- Misses 1041 1626 +585 |
awaelchli
requested review from
Borda,
carmocca,
edenlightning,
justusschock,
kaushikb11,
SeanNaren,
SkafteNicki,
tchaton and
williamFalcon
as code owners
| def should_skip_training(self): | ||
| should_by_max_steps = self.trainer.max_steps is not None and self.trainer.global_step >= self.trainer.max_steps | ||
| should_by_epoch = self.trainer.max_epochs is not None and self.trainer.current_epoch >= self.trainer.max_epochs | ||
| should_by_max_steps = self.max_steps is not None and self.global_step >= self.max_steps | ||
| should_by_epoch = self.max_epochs is not None and self.current_epoch >= self.max_epochs | ||
| return should_by_max_steps or should_by_epoch or self.trainer.num_training_batches == 0 |
There was a problem hiding this comment.
super nit: bool signature
and you can return early after each line instead of handling all the ors at the end
There was a problem hiding this comment.
does Python evaluate the other side of the and if the first one is already False? I though not? That would be weird
There was a problem hiding this comment.
are you asking me to do this:
if condition1
return True # return early
if condition2
return True # return early
...
return FalseThere was a problem hiding this comment.
in anticipation of upcoming changes to the train loop I will hold back on extensive typing for now
| def global_step(self) -> int: | ||
| return self.train_loop.global_step | ||
|
|
||
| @property | ||
| def current_epoch(self) -> int: | ||
| return self.train_loop.current_epoch | ||
|
|
||
| @property | ||
| def max_epochs(self) -> Optional[int]: | ||
| return self.train_loop.max_epochs | ||
|
|
||
| @property | ||
| def min_epochs(self) -> Optional[int]: | ||
| return self.train_loop.min_epochs | ||
|
|
||
| @property | ||
| def max_steps(self) -> Optional[int]: | ||
| return self.train_loop.max_steps | ||
|
|
||
| @property | ||
| def min_steps(self) -> Optional[int]: | ||
| return self.train_loop.min_steps |
There was a problem hiding this comment.
will this be deprecated in future?
There was a problem hiding this comment.
I don't think so. The user is allowed to access it and it's useful, but it should be read-only :)
There was a problem hiding this comment.
is batch_idx intentionally left out of these read-only properties?
There was a problem hiding this comment.
yes, it's not meant to be on the trainer because 1) it is accessible to the user through the hooks already 2) it anyway wouldn't be clear what the meaning of this variable is since it is specific to the loop.
|
The link in the CHANGELOG is wrong. |
|
@ruotianluo Do you want to send a patch with the correct link? |
What does this PR do?
Restores sanity around trainer state.
These attributes are now owned by the train loop and are read only on the trainer:
Exception: Tuner is allowed to reach into anything.
Before submitting
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:
Did you have fun?
Make sure you had fun coding 🙃