-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Profile hooks are called also in evaluation mode #33
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.
I like the direction you are heading @bedapisl, thank you! However, I would like to hear @blazekadam's opinion, since he was involved the most when writing the profiling functionality.
emloop/hooks/log_profile.py
Outdated
@@ -37,18 +37,21 @@ def after_epoch_profile(self, epoch_id, profile: TimeProfile, train_stream_name: | |||
:param extra_streams: enumeration of additional stream names |
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.
Remove those, please.
emloop/main_loop.py
Outdated
hook.after_epoch_profile(epoch_id=0, profile=self._epoch_profile) | ||
|
||
self._epoch_profile.clear() | ||
|
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.
emloop/main_loop.py
Outdated
hook.after_epoch(epoch_id=0, epoch_data=epoch_data) | ||
hook.after_epoch_profile(epoch_id=0, profile=self._epoch_profile) | ||
|
||
self._epoch_profile.clear() |
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.
We don't need to clean the profile here.
emloop/main_loop.py
Outdated
hook.after_epoch_profile(epoch_id=epoch_id, profile=self._epoch_profile, | ||
train_stream_name=self._train_stream_name, | ||
extra_streams=self._extra_streams) | ||
hook.after_epoch_profile(epoch_id=epoch_id, profile=self._epoch_profile) |
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 don't particularly like the stream extraction code in the LogProfile
hook. To avoid that, I suggest to pass
..., streams=[self._train_stream_name] + self._extra_streams)
Even better, I would probably like if the LogProfile
hook did not need to parse the stream names whatsoever and instead, the profile is stored in a more reasonable form. E.g., instead of batch_read_valid
, it would store a dict
with keys such as train
, valid
, ... What does @blazekadam think?
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 do not like it either, please pass the stream names instead of parsing them as @FloopCZ suggested.
We can postpone restructuring the profile
object for later.
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 guess we will need to update the tests as well.
emloop/main_loop.py
Outdated
hook.after_epoch_profile(epoch_id=epoch_id, profile=self._epoch_profile, | ||
train_stream_name=self._train_stream_name, | ||
extra_streams=self._extra_streams) | ||
hook.after_epoch_profile(epoch_id=epoch_id, profile=self._epoch_profile) |
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 do not like it either, please pass the stream names instead of parsing them as @FloopCZ suggested.
We can postpone restructuring the profile
object for later.
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.
Fixes #32.