-
Notifications
You must be signed in to change notification settings - Fork 66
Conversation
guenthermi
commented
Sep 23, 2022
•
edited
Loading
edited
- Removes info and debug log messages from commons packages
- add spinner for waiting until finetuning job is started
- remove envelops in stream log messages
- move console related functions and configuration into new module
- This PR references an open issue
- I have added a line about this change to CHANGELOG
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 spiner! much better than print
finetuner/run.py
Outdated
print(msg) | ||
else: | ||
break | ||
console = Console(width=200) |
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.
create Console
outside from the function, also check describe_models
.
yield entry.decode('utf-8', errors='ignore') | ||
decoded_message: str = entry.decode('utf-8', errors='ignore') | ||
sep_pos = decoded_message.find(': ') | ||
if sep_pos != -1: |
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 you elaberate what is this doing?
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.
The EventSourceResponse class generates different kinds of log message (e.g., "event" and "data" messages). It adds this type at the beginning of the message, e.g., "data: Finetuning ...". This code snippet divides the type and the message by splitting it at the first occurrence of ": " and only returns the message itself as log message.
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.
what if sep_pos
is None?
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.
The find
function returns -1 if it is not present. Therefore, I check for -1
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.
minor comment, can you attach a screenshot in the PR description of the spinner?
yield entry.decode('utf-8', errors='ignore') | ||
decoded_message: str = entry.decode('utf-8', errors='ignore') | ||
sep_pos = decoded_message.find(': ') | ||
if sep_pos != -1: |
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.
what if sep_pos
is None?
finetuner/__init__.py
Outdated
@@ -347,6 +326,9 @@ def get_model( | |||
""" | |||
from commons.data.inference import ONNXRuntimeInferenceEngine | |||
|
|||
for key in logging.root.manager.loggerDict: | |||
logging.getLogger(key).setLevel('CRITICAL') |
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.
why are you silencing all loggers?
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 thought that we only want to log the messages of the executor which are set by the ONNXRuntimeInferenceEngine. However, I can also change it to only silence the commons and the ulrlib logger in the case we want to log more. However, this means that there might occur other unexpected log messages after some changes.
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.
try to silence only these loggers first, and see how it looks
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.
LGTM!
1890889
to
f79e15c
Compare
This reverts commit 2b229cd.