search

cisocrgroup / ocrd_cis

OCR-D python tools
MIT License
33 stars 12 forks source link

getLogger per method in common.py #70

Closed kba closed 3 years ago

kba commented 3 years ago

That looks rather extreme. Couldn't we just:

  1. LOG = None on the module level
  2. common.LOG = getLogger('ocrolib') in the processors' constructors or process routines?

Messing with globals is not something I'd recommend, but it's your code, I just fixed module-level getLogger like everywhere else. Up to you.

bertsky commented 3 years ago

Messing with globals is not something I'd recommend, but it's your code, I just fixed module-level getLogger like everywhere else. Up to you.

Yes, I know – but what's the right pattern for a "library" module here?

kba commented 3 years ago

Messing with globals is not something I'd recommend, but it's your code, I just fixed module-level getLogger like everywhere else. Up to you.

Yes, I know – but what's the right pattern for a "library" module here?

getLogger should be a cheap operation, so I would say handle it the same as processor code: either request a logger per method or initiate a self.log instance var in the constructor where applicable.

What do you consider extreme about the PR?

bertsky commented 3 years ago

getLogger should be a cheap operation, so I would say handle it the same as processor code: either request a logger per method or initiate a self.log instance var in the constructor where applicable.

Not applicable unfortunately, since this module only provides functions.

What do you consider extreme about the PR?

Ok, so even if it is cheap to query the logger for each little function in a library, I'd actually like to remove the OCR-D dependency from ocrolib (it was a mistake to put it in there in the first place).

The logging cookbook says logging.getLogger will always yield the same instances for the same name. Since ocrd_utils.logging is based on this, can we assume it is enough just do LOG = loggging.getLogger('ocrolib') once and be done (rely on the CLI for the formatter and handler stuff)?

kba commented 3 years ago

can we assume it is enough just do LOG = loggging.getLogger('ocrolib') once and be done (rely on the CLI for the formatter and handler stuff)?

Yes, the setOverrideLoglevel mechanism won't work but otherwise, logging.getLogger should be equivalent.

bertsky commented 3 years ago

Yes, the setOverrideLoglevel mechanism won't work but otherwise

Are you sure? I thought it would catch all loggers via logging.Logger.manager.loggerDict anyway (setting their levels to NOTSET so the root logger's level applies).

bertsky commented 3 years ago

logging.getLogger should be equivalent.

I'd much prefer it that way (without ocrd_utils) in ocrolib then. Can I update your PR accordingly?

kba commented 3 years ago

Are you sure? I thought it would catch all loggers via logging.Logger.manager.loggerDict anyway (setting their levels to NOTSET so the root logger's level applies).

It's a matter of sequence. If the logger has been requested once before setOverrideLoglevel, it will be set to NOTSET. If the logger is requested afterwards from logging directly, its level isn't overridden.

I'd much prefer it that way (without ocrd_utils) in ocrolib then. Can I update your PR accordingly?

As I said: It's your code, I just wanted to wrap up the getlogger change.

kba commented 3 years ago

Are you sure? I thought it would catch all loggers via logging.Logger.manager.loggerDict anyway (setting their levels to NOTSET so the root logger's level applies).

It's a matter of sequence. If the logger has been requested once before setOverrideLoglevel, it will be set to NOTSET. If the logger is requested afterwards from logging directly, its level isn't overridden.

In fact, I think if you want to use logging directly, it's actually best to revert the commit completely and just change getLogger to logging.getLogger. That way, the logger is requested at module load time and is in the loggerDict once setOverrideLoglevel is called.

bertsky commented 3 years ago

In fact, I think if you want to use logging directly, it's actually best to revert the commit completely and just change getLogger to logging.getLogger. That way, the logger is requested at module load time and is in the loggerDict once setOverrideLoglevel is called.

That's what I was going to try out.

Perhaps we should also document this usage (and correct CLI usage) in ocrd_utils/README.md or ocrd_utils docstring?

bertsky commented 3 years ago

superseded by #71, but thanks anyway!