Skip to content
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

pycromanager setting the root logger on import #766

Closed
bruno-digitbio opened this issue May 28, 2024 · 8 comments
Closed

pycromanager setting the root logger on import #766

bruno-digitbio opened this issue May 28, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@bruno-digitbio
Copy link
Contributor

Code for reproduction / Actual Outcome
The latest version of pycromanager adds an error handler that forwards all logging messages from the root logger to stdout. Resulting in a wall of DEBUG messages when loading any module (e.g. matplotlib, numba, etc.) that uses the logging module extensively.

>>> import pycromanager
>>> import logging
>>> logging.root.handlers[0].level
0

Expected outcome
Only deployed binaries should set the root logger. Libraries that are meant to be imported by others should leave it untouched.

>>> import pycromanager
>>> import logging
>>> logging.root.handlers
[]

The offending code is here.

If we really want to always print debug messages for pycromanager specifically, you can of course set this configuration for pycromanager's internal logger instead:

main_logger.setLevel(...)
@bruno-digitbio bruno-digitbio added the bug Something isn't working label May 28, 2024
@henrypinkard
Copy link
Member

Suggestions on a PR for a fix? Any thoughts on this @jacopoabramo ?

@bruno-digitbio
Copy link
Contributor Author

bruno-digitbio commented May 28, 2024

Depends on where the break came from, but it looks like

this commit to PyJavaZ accidentally adds an import pycromanager.logging.

For one, this introduces a circular import dependency, so I would be a bit careful here, but the most likely fix is to just remove this line:

https://github.com/PyJavaZ/PyJavaZ/blame/eef3202b33f9c3fd0f1f625e1fea0a065ecc7cce/pyjavaz/bridge.py#L16

@bruno-digitbio
Copy link
Contributor Author

(on the other hand, it looks like that commit also introduced a lot of usage of pycromanager.logging.main_logger. If we want to keep this usage, then the best solution is probably to set the level of the logging.getLogger("pycromanager") logger instead of the root logger...)

I have a lot of experience with Python logging, if you have a list of requirements (importing module X should trigger logging at level Y, but by default logging should be at level Z, but only for modules ABC, etc.), happy to send a PR, but it's a bit hard to infer from the current state what the original intention was!

@henrypinkard
Copy link
Member

Yeah the circular dependency is definitely not supposed to be there and should be removed. I don't think that any special logging is required in PyJavaZ, so that could all be removed. All this logging was added in #719

@jacopoabramo
Copy link
Contributor

Hey, sorry for the late reply. I'm the author of PR #719

Background on this PR is: I wanted to add a logger instance for debugging to be compatible with another project I've been working on (integrating pycromanager in ImSwitch). I have to admit I'm not an expert on logging and my initial thought was the following: creating a logger instance that would be the baseline for debugging when debug=True while also providing access to an external logger instance from another application (in this case, the ImSwitch logger).

@bruno-digitbio I would happy to provide a fixing PR if you have some suggestions on how to approach this

@dpshepherd
Copy link
Contributor

Hi all -

This is a high priority issue for us as well, as the changes are creating chaos with logging and the console. Is there anything we can do to help push forward fixing the problem?

Thanks.

@bruno-digitbio
Copy link
Contributor Author

bruno-digitbio commented Jun 4, 2024

compatible with another project I've been working on (integrating pycromanager in ImSwitch).

Are you trying to use coloredlogs? If so, then pycromanager itself does not need any custom logging code. Your application (when it wants to see colored pycromanger logs), can simply say

(in your_file.py),

coloredlogs.install(
    level='DEBUG', 
    logger=logging.getLogger("pycromanager"),
    fmt='%(asctime)s %(levelname)s %(message)s'
)

after this point, any code that logs to the "pycromanager" logger will have this configuration applied.

I have to admit I'm not an expert on logging and my initial thought was the following: creating a logger instance that would be the baseline for debugging

I think I see what you were trying to do here, definitely a reasonable thought, but (if I'm understanding correctly), simply coming from the wrong mental model for how Python's built-in logging module is meant to be used.

logging maintains a global "database" of "logger"s. In the simplest case, the idea is that:

  • each module creates its own logger, e.g. pycromanager.acquisition.acquisition_superclass would call logging.getLogger(__name__) to create a logger called "pycromanager.acquisition.acquisition_superclass"
  • the global loggers "database" organizes loggers into a "tree" based on the "module structure" implied by the dot-separated parts of the logger's name, so if in any module, you create a logger (let's call it logger1) called "pycromanager", and in any other module (before or after!) you create another logger (logger2) called "pycromanager.acquisition", then logger2.parent == logger1.
  • whenever you apply config to a logger, it is used by that logger's children
  • each module logs to its own logger, and doesn't touch the config at all
  • the user who is running the final binary/script/etc and wants to configure the logger then simply grabs the logger(s) they want to configure and sets information specifically for those loggers.
  • IMPORTANT: there is an implicit "root" logger (see logging.getLogger(anything).root), and because loggers' config applies to its children, all configuration performed on the root logger affects all modules. Libraries should never touch the root logger, only application code.

when debug=True while also providing access to an external logger instance from another application (in this case, the ImSwitch logger).

Walking through the implications of the system I (try to) outline above, if you want to apply the same logger configuration used by (e.g.) ImSwitch to pycromanager, you should not actually monkeypatch ImSwitch's logger to "trick" pycromanager into using it. Instead, you should simply apply that same configuration to the pycromanager logger (as shown in the coloredlogs example above.

If you want to "reset" the settings back to normal, you should again definitely not replace the logger object itself, but instead you should modify it's (global) properties to have the settings that you want. For example, coloredlogs just installs a so-called "handler" for the logger you give it:

In [2]: import logging
In [3]: pycro_logger = logging.getLogger("pycromanager")
In [4]: import coloredlogs
In [5]: coloredlogs.install(logger=pycro_logger, level=logging.DEBUG)
In [6]: pycro_logger.handlers
Out[6]: [<StandardErrorHandler <stderr> (INFO)>]

so to undo that, you would just remove it and reset the level

In [11]: pycro_logger.setLevel(logging.WARNING)
In [12]: pycro_logger.removeHandler(pycro_logger.handlers[0])
In [13]: pycro_logger.handlers
Out[13]: []

@bruno-digitbio I would happy to provide a fixing PR if you have some suggestions on how to approach this

There's a few things to fix here, across a couple of repositories:

  • In PyJavaZ, remove the usage of the "pycromanager" logger altogether (EDIT: e.g,. Create canonically-named logger for pyjavaz.bridge to use PyJavaZ/PyJavaZ#5). It's possible that actually using the pycromanager logger is what you want, but probably what you really want is for PyJavaZ to log to loggers like "pyjavaz.*", and then you can always coloredlogs.install(logger=logging.getLogger("pyjavaz"), level=logging.DEBUG) to set a handler on that top-level "parent" logger to get the debug messages if you want. If you really want to log to "pycromanager", do not bother importing the module as you have, instead simply call logging.getLogger("pycromanager").
  • In Do not set the root logger's level to DEBUG #768, I simply remove the usage of the root logger.
  • As a follow-up (e.g. Remove internal usage of pycromanager.logging #769), I would recommend we remove the usage of the existing pycromanager.logging altogether from everywhere, and instead use the (standard) Python conventions that each module logs to its own personal logger if needed (allowing for flexible configuration by application writers)

@bruno-digitbio
Copy link
Contributor Author

bruno-digitbio commented Jun 4, 2024

This is a high priority issue for us as well, as the changes are creating chaos with logging and the console. Is there anything we can do to help push forward fixing the problem?

@dpshepherd, In the mean time, at work we are patching this problem by making sure that (something like) the following lines are executed before any other imports:

import pycromanager.logging  # trigger the "bad" `logging.basicConfig` call
import logging
logging.getLogger().handlers = []  # delete the erroneously-created handler

after this, stuff like

import numba

no longer creates walls of output

bruno-digitbio added a commit to bruno-digitbio/pycro-manager that referenced this issue Jun 6, 2024
This removes public API (first introduced in micro-manager#719) for bypassing
Python's internal logging system by creating and caching a logger
instance in place of using `logging.getLogger("pycromanager")`.

Users of that previous API---i.e., application code authors that want to
globally control logging and users that want to customize Pycromanager's
logging outputs---are encouraged to simply directly use the logging
module to affect these changes instead. See micro-manager#766 for discussion.
bruno-digitbio added a commit to bruno-digitbio/pycro-manager that referenced this issue Jun 7, 2024
This removes public API (first introduced in micro-manager#719) for bypassing
Python's internal logging system by creating and caching a logger
instance in place of using `logging.getLogger("pycromanager")`.

Users of that previous API---i.e., application code authors that want to
globally control logging and users that want to customize Pycromanager's
logging outputs---are encouraged to simply directly use the logging
module to affect these changes instead. See micro-manager#766 for discussion.
bruno-digitbio added a commit to bruno-digitbio/pycro-manager that referenced this issue Jun 7, 2024
This removes public API (first introduced in micro-manager#719) for bypassing
Python's internal logging system by creating and caching a logger
instance in place of using `logging.getLogger("pycromanager")`.

Users of that previous API---i.e., application code authors that want to
globally control logging and users that want to customize Pycromanager's
logging outputs---are encouraged to simply directly use the logging
module to affect these changes instead. See micro-manager#766 for discussion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants