You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Describe the bug
Any application that imports this library cannot expect their own configuration of the Python root logger to be respected, because this library adds to the root logger's list of handlers.
This is an issue because logging.basicConfig() is ignored once any handlers are configured. At a bare minimum, it is reasonable to expect all libraries make no modifications to the root handler. The quickest fix is to edit line 89 so as to only add the handler onto the subloggers that will be used throughout the library:
haystack_logger=logging.getLogger("haystack") # only add our handler within our library's hierarchy# avoid adding our handler twiceold_handlers= [
hforhinhaystack_logger.handlersif (isinstance(h, logging.StreamHandler) andh.name=="HaystackLoggingHandler")
]
forold_handlerinold_handlers:
haystack_logger.removeHandler(old_handler)
haystack_logger.addHandler(handler)
# or more succinctly, only add if not already present# if not old_handlers:# haystack_logger.addHandler(handler)
However, it is also generally expected that the application and not the library is the arbiter of all log handlers, as recommended in the python docs' Logging Cookbook. This would mean it is unusual for any library to implicitly add a log handler -- it is the application developer who knows best what log formats they need.
I agree that providing recommended overrides can be very convenient; one route would be to export a factory for the provided handler so that the consuming application can easily opt-in to this feature:
fromhaystack.loggingimportconfigure_logging_handler# function to create the HaystackLoggingHandlerlogging.getLogger().addHandler(configure_logging_handler()) # app dev can choose to add at the root, at the haystack level, or not at all
fromhaystackimportDocumentfromhaystack.document_stores.in_memoryimportInMemoryDocumentStoreimportloggingimportpandasaspdlogging.basicConfig(level=logging.CRITICAL)
document_store=InMemoryDocumentStore()
document_store.write_documents([
# still prints a warning, because of the logging.getLogger().root changes within haystackDocument(
content="My name is Jean and I live in Paris.",
dataframe=pd.DataFrame({"name": ["Jean"], "city": ["Paris"]}),
),
])
Describe the bug
Any application that imports this library cannot expect their own configuration of the Python root logger to be respected, because this library adds to the root logger's list of handlers.
This issue occurred previously in #2485 and #4202
Expected behavior
An application using this library should be able to
import haystack
and then uselogging.basicConfig()
as normal.Additional context
This issue was introduced here
This is an issue because
logging.basicConfig()
is ignored once any handlers are configured. At a bare minimum, it is reasonable to expect all libraries make no modifications to the root handler. The quickest fix is to edit line 89 so as to only add the handler onto the subloggers that will be used throughout the library:However, it is also generally expected that the application and not the library is the arbiter of all log handlers, as recommended in the python docs' Logging Cookbook. This would mean it is unusual for any library to implicitly add a log handler -- it is the application developer who knows best what log formats they need.
I agree that providing recommended overrides can be very convenient; one route would be to export a factory for the provided handler so that the consuming application can easily opt-in to this feature:
Quick blog post summary of developer expectations on this topic: http://rednafi.com/python/no_hijack_root_logger/
To Reproduce
Minimal repro:
FAQ Check
System:
The text was updated successfully, but these errors were encountered: