-
Notifications
You must be signed in to change notification settings - Fork 920
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
replace print with logger #1104
Conversation
* Add get_my_logger() * Use logger instead of print * Fix log level * Removed line-breaks for readability * Use setup_logging() * Add rich to requirements.txt * Make simple * Use logger instead of print --------- Co-authored-by: Kohya S <52813779+kohya-ss@users.noreply.github.com>
@shirayu Thank you again for your great contribution. I have made the following modifications:
Please take a look at |
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.
Great!
Thank you for your updates!
Almost everything is fine, but I did make a few comments.
I am sure you already understand this, but just to be clear, I note that the call of add_logging_arguments
is also needed in other files like sdxl_train.py
.
library/utils.py
Outdated
handler = RichHandler() | ||
except ImportError: | ||
print("rich is not installed, using basic logging") |
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 think that it is better to use logger.info(....)
after logging.root.addHandler(handler)
like this.
msg_init = None
# set message on ImportError
msg_init = "rich is not installed, using basic logging"
....
logging.root.addHandler(handler)
if msg_init is not None:
logger = logging.getLogger(__name__)
logger.info(msg_init)
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.
Thanks for the great suggestion! I will adopt this one.
fine_tune.py
Outdated
|
||
|
||
def setup_parser() -> argparse.ArgumentParser: | ||
parser = argparse.ArgumentParser() | ||
|
||
add_logging_arguments(parser) # モジュール指定がないのがちょっと気持ち悪い / bit weird that this does not have module prefix |
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.
Moving add_logging_arguments()
and setup_logging()
from library/utils.py
to library/logger_utils/__init__.py
is an option.
The call will be like this.
logger_utils.add_logging_arguments(parser)
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.
Thank you! I think this is certainly one way to do it. I just don't want to add too many files, so I guess I'll just leave it as it was.
library/utils.py
Outdated
from rich.logging import RichHandler | ||
|
||
handler = RichHandler() |
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 would like to add one more thing.
By default, rich
logs to standard output (stdout), but I think it is better to change it as follows.
Proposal
from rich.console import Console
from rich.logging import RichHandler
handler = RichHandler(console=Console(stderr=True))
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.
Thank you! That certainly seems to be consistent.
If someone wants the output to be on stdout as before, there is an option for it.
No description provided.