Change the way load_path is handled
#132
Conversation
| @@ -75,17 +75,19 @@ class LoadStrategy(StrEnum): | |||
|
|
|||
| if_available = "if_available" | |||
| """ | |||
| Only load from the load path if a checkpoint exists there. | |||
| The trainer will attempt to load a checkpoint from the save folder or load path (in that order) | |||
There was a problem hiding this comment.
Thanks for updating these! I started reviewing before you updated them and got confused.
src/olmo_core/train/trainer.py
Outdated
| elif self.load_strategy == LoadStrategy.if_available: | ||
| self.maybe_load_checkpoint(load_path) | ||
| if not self.checkpoint_loaded and self.load_strategy != LoadStrategy.never: | ||
| if not self.maybe_load_checkpoint(self.save_folder) and self.load_path is not None: |
There was a problem hiding this comment.
nit: I'm not a fan of self.maybe_load_checkpoint(self.save_folder) being in the if statement due to its side-effect nature of setting self._checkpoint_loaded.
Moreover, I'm not a fan of self.maybe_load_checkpoint returning a bool that isn't self._checkpoint_loaded. Imo we should either
- remove
self._checkpoint_loaded - (my favorite) make
self.maybe_load_checkpointnot return anything and rely explicitly onself.checkpoint_loaded - make
self.maybe_load_checkpointreturnself.checkpoint_loaded
There was a problem hiding this comment.
Fair point about the if statement, changed in d01aafc.
But I do think it's a useful part of the API for maybe_load_checkpoint() to return a boolean, and this shouldn't necessarily be the value of self._checkpoint_loaded, since self._checkpoint_loaded tells you whether any checkpoint has been loaded at any point in time, whereas the return value of maybe_load_checkpoint tells you if a checkpoint was loaded this time from the given load path.
There was a problem hiding this comment.
I like the change, but didn't read the code closely, since @2015aroras is already on it.
|
This PR has been released in v1.8.0. |
No description provided.