-
-
Notifications
You must be signed in to change notification settings - Fork 16.7k
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
fix: disable usage of root logger #7296
fix: disable usage of root logger #7296
Conversation
`logging.basicConfig` configures Python's root logger. This prohibits fine control of logging, overwrites logging configuration done outside the package, and is not best practice. Instead, the used logger is now configured directly, and the root logger is untouched. Example: If yolov5 is used as part of another project with some sophisticated logging, the internal `logging.basicConfig` call overwrites all the external configuration.
@maxstrobel thanks for the PR! Is there a way to reduce the added lines while accomplishing the same result? The PR replaces 2 lines with 9 currently. We'd like to minimize/simplify where possible. |
@glenn-jocher - Thanks for the quick response. Maybe, we could save one or two lines, but I think it will always be a bit more. Some explanations about logging: Actually the function did two things, which I tried to replicate:
I think a better approach (but also slightly more changes) would be to split this up in two functions.
|
@maxstrobel oh I see. So the current function would not return anything, and each independent file would get the logger on it's own with Would all these instances be connected as they are now (the same LOGGER essentially?) LOGGER is imported now in quite a few places with |
@glenn-jocher would this be a middle ground? def set_logging(name=None, verbose=VERBOSE):
# Sets level and returns logger
if is_kaggle():
for h in logging.root.handlers:
logging.root.removeHandler(h) # remove all handlers associated with the root logger object
rank = int(os.getenv('RANK', -1)) # rank in world for Multi-GPU trainings
level = logging.INFO if (verbose and rank in (-1, 0)) else logging.WARNING
log = logging.getLogger(name)
log.setLevel(level)
handler = logging.StreamHandler()
handler.setFormatter(logging.Formatter("%(message)s"))
handler.setLevel(level)
log.addHandler(handler)
set_logging() # EDIT: I think we need to run this before defining LOGGER
LOGGER = logging.getLogger("yolov5") # define globally (used in train.py, val.py, detect.py, etc.) |
@maxstrobel lastly, since you seem to know your way around logging, do you know what is happening in Kaggle? Without removing the root loggers in kaggle no output is observed in kaggle notebooks, so I was forced into this special workaround in
|
So, if you call
Maybe you want to have a look at the Logging Flow, https://docs.python.org/3/howto/logging.html#logging-flow. Basically logging can be separated in those two blocks. Simply speaking, the
Probably those changes would also resolve the Kaggle issue, since the specifically mention to not use |
@maxstrobel ok got it. I'll do some testing in Kaggle to see if PR resolves issue there. That would be a nice bonus. |
@glenn-jocher, sounds like a plan. If you want to avoid too much changes, I'd go for your "middle ground" solution (+ if applicable remove the Kaggle stuff). |
@maxstrobel tested with a5f8969 and get the original problem of logging output not showing in Kaggle (only the torch download print() statements are showing). Strange. Anyway that's outside the scope of the original changes so I will merge PR as is now. |
@maxstrobel PR is merged. Thank you for your contributions to YOLOv5 🚀 and Vision AI ⭐ |
* fix: disable usage of root logger `logging.basicConfig` configures Python's root logger. This prohibits fine control of logging, overwrites logging configuration done outside the package, and is not best practice. Instead, the used logger is now configured directly, and the root logger is untouched. Example: If yolov5 is used as part of another project with some sophisticated logging, the internal `logging.basicConfig` call overwrites all the external configuration. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update general.py * Update general.py * Comment kaggle * Uncomment kaggle Co-authored-by: Maximilian Strobel <[email protected]> Co-authored-by: Glenn Jocher <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
logging.basicConfig
configures Python's root logger. This prohibitsfine control of logging, overwrites logging configuration done outside
the package, and is not best practice. Instead, the used logger is now
configured directly, and the root logger is untouched.
Example:
If yolov5 is used as part of another project with some sophisticated
logging, the internal
logging.basicConfig
call overwrites all theexternal configuration.
Closes #6060
🛠️ PR Summary
Made with ❤️ by Ultralytics Actions
🌟 Summary
Improved logging setup in the YOLOv5 codebase.
📊 Key Changes
Objects365.yaml
.general.py
by:🎯 Purpose & Impact