This is an archive of the discontinued LLVM Phabricator instance.

Don't use root logger at import time
ClosedPublic

Authored by hajarmitage on Sep 28 2022, 7:44 AM.

Details

Summary

At import time, these calls to logging.debug() implicitly call logging.basicConfig (https://docs.python.org/3/library/logging.html#logging.basicConfig), setting logging config for the whole project which cannot then be overwritten later. For instance, consider the following test script:

import logging
import jax

logger = logging.getLogger(__name__)
logging.basicConfig(level=logging.INFO)
logger.info('info')

This should log out 'info', but because when import jax is called, this _mlir_lib/__init__.py file is run and a logging.debug is called, calling logging.basicConfig, my logging.basicConfig(level=logging.INFO) does nothing.

Fix: instead of using root logger, use a module level logger.

Found in this issue: https://github.com/google/jax/issues/12526

Diff Detail

Event Timeline

hajarmitage created this revision.Sep 28 2022, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 7:44 AM
hajarmitage requested review of this revision.Sep 28 2022, 7:44 AM
stellaraccident accepted this revision.Sep 28 2022, 7:53 AM

Good catch - thanks.

This revision is now accepted and ready to land.Sep 28 2022, 7:53 AM

Also note https://reviews.llvm.org/D133450 in flight. I think yours should go in first

I don't think I can commit this - can you commit this for me.

hajarmitage requested review of this revision.Oct 10 2022, 5:14 AM

Sorry for the delay - I attempted to rebase/land but the patch conflicts. I don't have time to actually update it right now - can you rebase/fix? I'll be back from traveling later in the week and can do the rebase/fix if you don't get to it but am not in a position to be making actual edits right now.

Have updated the diff, and replaced a new call to logging.warning with logger.warning

Gentle ping: can this be merged? The same bug got reported again.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 5 2022, 5:56 PM
This revision was automatically updated to reflect the committed changes.

Gentle ping: can this be merged? The same bug got reported again.

Landed. Very sorry for the delay getting this merged (do feel free to ask on discord if delays like this happen with individuals in the future: any committer can land).