Fix: StorageManager no longer interferes with root logger (#1405) #1419
Conversation
|
@josephvincent24 how would that work? This means that if the user has any logger with a handler already configured, ClearML will simply not add it's default log settings (which by default are setting the ClearML log level, and sets a proper log level to multiple dependency packages). |
|
@jkhenning You're right, this solution could lead to some unpredictable behavior. I'd be happy to set this up as an environment variable for now to make it more explicit. In the future, we could provide a public function so that users can initialize logging and "force" this behavior. What do you think? |
|
Perhaps a better approach would be to store the old root loggers, apply the dictConfig and than add back any root logger that does not already exist? |
|
@jkhenning I also considered this, but I'm not sure it's safe. When dictConfig is called, it closes all existing handlers attached to the root logger, so simply re-adding them will not resume logging as expected because those handler objects are now closed and unusable. Re-initializing them as new handlers will lose some user customization. Also, manipulating handlers in this way might introduce thread safety issues. |
|
If that's the case, we should at least support a configuration option / env var to control this behavior and keep the default as the old behavior - WDYT? |
Summary
This PR fixes an issue where using StorageManager.get_local_copy() (or any ClearML utility that triggers logging initialization) would close user-defined logging handlers, such as [logging.FileHandler], if the user already configured logging. This caused log messages to stop appearing in user log files after the first ClearML call.
Details
Root cause:
ClearML’s logging initialization called [logging.config.dictConfig], which resets and closes all existing logging handlers, including those set up by the user.
Fix:
The patch adds a check in clearml/backend_config/log.py so that [dictConfig] is only called if the root logger has no handlers. This prevents ClearML from interfering with user logging setups.
I also added a fix that creates a shallow copy in clearml/debugging/log.py in the clear_logger_handlers() function. This function closes the handler before it is removed and modifies the loop to iterate over a shallow copy of the handlers list.
Test:
Added a test (test_storage_manager.py) to ensure that user-defined handlers remain open and receive log messages after calling StorageManager.get_local_copy().
Impact:
Backwards compatible:
If the user does not configure logging, ClearML will still set up logging as before.
Best practice:
This approach follows standard open-source library practices for interacting with Python’s logging system.
Closes: StorageManager affects root logger of logging #1405 (#1405)