-
Notifications
You must be signed in to change notification settings - Fork 877
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 if they are logs #905
Conversation
Thanks for the comprehensive and detailed PR. I appreciate your hard work and I am sure this will be of great help to the users. Since the changes are so extensive, it seems to me that it would be better to wait until after we have taken care of all the other PRs that can be addressed sooner. I appreciate if you could wait for a while. |
cf.
|
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.
Changing print()
s to logging calls is a great idea, but I think this PR proposes a slight misuse of logging
(please see my review comment within).
|
@kohya-ss Hi, thanks a lot for always doing great work. |
@shirayu |
I think the advantage of using a logging framework is that we can change the log output level, log output destination, and log format etc., perhaps by options. If we want to achieve this, should we initialize the logging framework in |
Logging is now initialized early in each script by from library.utils import setup_logging
setup_logging() I think a follow-up PR can change that to happen after |
That's great! However, just curious for me, what is the advantage of initializing the log at the beginning of the script instead of initializing it in main? |
No difference - unless something logs something "too early", in which case |
Thank you for clarification! |
@shirayu In addition, it seems that sd-scripts code is sometimes incorporated into other projects. Proper use of the logging framework may actually be a difficult task... |
The following is an example of ignoring errors in the log initialization section. # current
from library.utils import setup_logging
setup_logging() # an idea
try:
from library.utils import setup_logging
setup_logging()
except ImportError:
pass
# current
logger = logging.getLogger(__name__) # an idea
logger = logging.getLogger(__name__)
for f in [logger.debug, logger.info, logger.warning, logger.error, logger.critical]:
f = print |
Mm, troublesome how? Using logging.getLogger("library.hoge").setLevel(logging.ERROR) # quiesce all hoge except errors
logging.getLogger("library.puyo").setLevel(logging.DEBUG) # make puyo louder! Whereas with
Unfortunately the suggested replacements are not a good idea – Users who want log output to look like logging.getLogger("library").addHandler(...) and all |
Oh, and by the way – to avoid people needing to copy-paste these scripts into their other projects, I will gladly lend a hand to make this repository installable as a Python package (like I did yesterday with huchenlei/HandRefinerPortable#2) – but that's a matter for a different PR, of course 😄 |
Thank you for suggestions! Hmm... If I use other libraries, I don't like the detailed formatted log output from these libraries. Therefore, I personally would like the default behavior of each scripts to be the same as the current print. So that, would the following code do it? try:
from library.utils import setup_logging
setup_logging()
except ImportError:
# setup logging handler here same as print |
I think this is what you expect. import logging
try:
from library.utils import setup_logging
setup_logging()
except ImportError:
class PrintHandler(logging.Handler):
def emit(self, record):
print(self.format(record))
handler = PrintHandler()
logging.root.addHandler(handler)
logging.root.setLevel(logging.DEBUG) # print ALL logs
logger = logging.getLogger(__name__) |
Ah, @shirayu, I'm not quite following why there would ever be an import error for Also, could you remove the
If I understand you correctly here, you'd want logger = logging.getLogger(__name__)
# ...
if __name__ == "__main__":
from library.utils import configure_logger_to_print
configure_logger_to_print(logger)
# ... other initialization ... and something like def configure_logger_to_print(logger):
h = logging.StreamHandler(sys.stdout)
h.setLevel(logging.DEBUG)
h.propagate = False # prevent parent loggers from handling this
logger.addHandler(h) ? |
In some projects (and me), some scripts are copied and reused for some features, such as model conversions, etc. In this case, an import error may occur.
My concern is when the scripts are reused. However, well, I would like to have the option to make logger output from the main scripts as same as regular print (I think consideration should be given as to whether rich output or print should be the default). |
This PR replaces print with logger if they are logs.
This helps distinguish logs from other outputs.
In addition, this replacement can also determine whether logs are warnings, information, or errors, and can understand what line of the file they come from.