-
Notifications
You must be signed in to change notification settings - Fork 31
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
ocrd_cli_wrap_processor: always do initLogging #1296
base: master
Are you sure you want to change the base?
Conversation
(Of course, as already commented in #1223, it may still be necessary to be careful with imports on the side of the processor implementations. For example, moving Alternatively, we could still do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Of course, as already commented in #1223, it may still be necessary to be careful with imports on the side of the processor implementations. For example, moving
import tensorflow
tosetup
.)
I am wondering how future-proof these mechanisms are. There have been different workarounds to prevent tensorflow and others from logging, like our tf_disable_interactive_logs
and various copy-and-pasted stanzas like in eynollah:
os.environ["TF_CPP_MIN_LOG_LEVEL"] = "3"
stderr = sys.stderr
sys.stderr = open(os.devnull, "w")
import tensorflow as tf
from tensorflow.python.keras import backend as K
from tensorflow.keras.models import load_model
sys.stderr = stderr
tf.get_logger().setLevel("ERROR")
warnings.filterwarnings("ignore")
And apart from being really intrusive and inconsistent, they could break at any point.
Alternatively, we could still do
initLogging
on the module level, though not inocrd_utils.logging
but ratherocrd.decorators
, which arguably will only be imported by applications that do need log handlers.
Module-level in ocrd.decorators.__init__.py
makes sense and a cursory glance over how eynollah and ocrd_kraken are structured make it seem this would work. For ocrd_calamari, there is a top-level __init__.py
which does from .recognize import CalamariRecognize
, i.e. tensorflow is imported before ocrd.decorators
but it is guarded by tf_disable_interactive_logs
.
So, as much as I dislike module-level function calls in general, in the interest of fewer surprises in the future, I'm for putting initLogging
in ocrd.decorators.__init__
.
right, but that just covers the rogue
That should not be necessary if we run our
wow, that's rather extreme, it speaks to the frustration that we had with correct logging setup in the Python ecosystem
should also not be necessary anymore
That's something we could indeed add into |
fixes #1223 and replaces #1295