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

Do not initialize logging on import #8634

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented May 6, 2024

Initializing logging on import has many unwanted side-effects.

Most importantly, it is very difficult to configure / overwrite anything unless the config file is overwritten directly. This should delay log configuration until it is needed. This allows code like this to work

import logging

import dask
import dask.bag as db

from dask.distributed import Client

with dask.config.set({
    "logging": {
        "custom": "info",
        "distributed": "warning",  # Default is INFO which is a little verbose
    }
}):
    client = Client(silence_logs=False)

logger = logging.getLogger("custom")
logger.setLevel(logging.INFO)


def task(n: int):
    logger.info(f"Hello {n}")
    
bag = db.from_sequence([1,2,3])
bag.map(task).compute()

which just prints

2024-05-06 12:55:07,779 - matt - INFO - Hello 1
2024-05-06 12:55:07,779 - matt - INFO - Hello 3
2024-05-06 12:55:07,779 - matt - INFO - Hello 2

@fjetter
Copy link
Member Author

fjetter commented May 6, 2024

I likely have to go through a couple of edge cases and adjust some tests for this to work. I haven't tried how the silence_logs kwarg factors in but overall I think this change is a strictly positive improvement

Copy link
Contributor

github-actions bot commented May 6, 2024

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

    25 files  ±0      25 suites  ±0   10h 22m 16s ⏱️ + 5m 23s
 4 129 tests ±0   4 013 ✅  - 5    110 💤 ±0  6 ❌ +5 
47 698 runs  ±0  45 596 ✅  - 6  2 096 💤 +1  6 ❌ +5 

For more details on these failures, see this check.

Results for commit b2b4a62. ± Comparison against base commit 36020d6.

♻️ This comment has been updated with latest results.

@fjetter fjetter changed the title [WIP] do not initialize logging on import Do not initialize logging on import May 16, 2024
@fjetter fjetter marked this pull request as ready for review May 16, 2024 11:49
@hendrikmakait hendrikmakait self-requested a review May 16, 2024 12:12
Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks like an improvement to me.

I haven't tried how the silence_logs kwarg factors in but overall I think this change is a strictly positive improvement

Testing this would be good to help us understand if this causes a regression for silence_logs.

@fjetter
Copy link
Member Author

fjetter commented Aug 16, 2024

I checked the behavior again. If silence_logs=True we're still setting all the loggers to silent but with silent_logs=False it is respecting the config set in the dask.config ctx manager. I think this behavior makes sense.

@philippjfr
Copy link
Contributor

Sorry for doing the annoying thing and chiming in with a useless comment, but this is a continuous source of frustration when you're building Bokeh/Panel apps which import Dask. So we'd really love to see this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants