Clearer disable validation logic #650
Conversation
| ref_model.on_sanity_check_start() | ||
| ref_model.on_train_start() | ||
| if self.get_val_dataloaders() is not None and self.num_sanity_val_steps > 0: | ||
| if not self.disable_validation and self.num_sanity_val_steps > 0: |
There was a problem hiding this comment.
consider user _ so self._disable_validation but maybe it s not needed
There was a problem hiding this comment.
It looks like _ prefix is not commonly used across the code so I believe it would be more consistent to leave it as it is
There was a problem hiding this comment.
I know, that I left it to your consideration... you introduced this variable so you should know if it is exposed (without _ ) or internal (with _ ) one...
There was a problem hiding this comment.
Yeah, I understand. And I just mean that I would prefer to stick to the current codebase style, where there seems to be no distinction between exposed and internal variables :)
| self.num_training_batches = None | ||
| self.val_check_batch = None | ||
| self.num_val_batches = None | ||
| self.disable_validation = None |
There was a problem hiding this comment.
earlier you have it as True/False
There was a problem hiding this comment.
Yeah, but it is the definition inside TrainerTrainLoopMixin, where we have
# this is just a summary on variables used in this abstract class, # the proper values/initialisation should be done in child class There was a problem hiding this comment.
yeah, but be consistent in your addition, use bool or obejct/None everywhere...
There was a problem hiding this comment.
Hmmm... I define this only in 2 places: in TrainerTrainLoopMixin and in Trainer. Inside TrainerTrainLoopMixin all fields are set to None, and inside Trainer we assign the actual value. Maybe I don't unserstand something?
| # when testing make sure user defined a test step | ||
| can_run_test_step = False | ||
| if not (self.is_overriden('test_step') and self.is_overriden('test_end')): | ||
| m = '''You called .test() without defining a test step or test_end. |
There was a problem hiding this comment.
pls add ` around functions and variables
| @kuynzereb great job! sorry for the delay :) |
3 participants
In this PR I suggest to make clearer disable validation logic. It will be a flag which will be set in the
pretrain_routine()so during the training the trainer will know if there will be validation runs or not. Now validation will be disabled if:num_val_batches=0. It either means that there is noval_dataloaderor that the user have setval_percent_check=0(this part will work when Fix percent_checks #649 is merged).validation_stepwas not overridenOnce this merged we will be able to finish #542.