-
Notifications
You must be signed in to change notification settings - Fork 1k
Add training date and time to policy metadata when persisting Fixes #1104 #1108
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left two comments / suggestions. looks good otherwise 👍
@@ -123,7 +126,8 @@ def _persist_metadata(self, path, dump_flattened_stories=False): | |||
"python": ".".join([str(s) for s in sys.version_info[:3]]), | |||
"max_histories": self._max_histories(), | |||
"ensemble_name": self.__module__ + "." + self.__class__.__name__, | |||
"policy_names": policy_names | |||
"policy_names": policy_names, | |||
"trained_at": self.date_trained |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just use the timestamp at persistence time as this happens right after actual training? in some sense the persist()
method completes the training. in that case we wouldn't need the self.date_trained
attribute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I agree that persist completes the training but having it inside the training function ensures that the time is logged as it is described (in case the script in question doesn't follow .train()
with .persist()
immediately after).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, but that's very much a special case. either way works, really
Proposed changes:
Status (please check what you already did):