GH-113421: Fix multiprocessing logger #113423

xu-song · 2023-12-23T06:26:26Z

Issue: multiprocessing logger get bad %(filename)s #113421

ghost · 2023-12-23T06:26:28Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2023-12-23T06:26:29Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

gaogaotiantian · 2023-12-23T07:14:22Z

The fix looks reasonable to me. You should probably add a regression test and a news entry to it.

bedevere-app · 2023-12-23T08:11:59Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

xu-song · 2023-12-23T10:05:21Z

Test case and news entry have been added to this pull request.

serhiy-storchaka

The test does not fail with unmodified code. But it produces superfluous output and warnings (because it modifies the global handlers list).

Please make a test that doues not produce unneeded output and warnings, fails when the bug is not fixed, and success if it is fixed.

bedevere-app · 2023-12-23T10:22:38Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

xu-song · 2023-12-23T11:42:43Z

@serhiy-storchaka
I have made the requested changes; please review again.

gaogaotiantian · 2023-12-23T20:49:05Z

@xu-song

You test still does NOT fail.

A regression test should:

Fail for unmodified code (your test does NOT). You should add some self.assert* to make this happen. Check the docs for unittest.
Pass after your modification. In this case, you should check the output of the logger for some specific strings (filenames).
Keep the original state before the test executes as much as possible. You added a handler to multiprocess logger and never remove it. You restored the log level, but the better way to do it is to restore the value to what it was, not a hardcoded LOG_LEVEL. You should also consider the cases when the test failed in the middle of your code and how to restore the state in those cases (try ... finally ... block).

In practice, the following should happen:

WITHOUT your source code changes (but with the regression test you added), ./python -m test test._test_multiprocessing should FAIL and report some error.

WITH your source code changes and the regression test you added, ./python -m test test._test_multiprocessing should PASS.

gaogaotiantian

I think you are doing a great job. That's what we meant for a regression test. I do have some suggestions to you, but that's basically what I got for now. I'll leave the final approval for @serhiy-storchaka .

gaogaotiantian · 2023-12-24T07:24:24Z

Lib/test/_test_multiprocessing.py

+
+    def test_filename(self):
+        logger = multiprocessing.get_logger()
+        default_level = logger.level


I think original_level might be a more proper name for this. It does not have to be the default level.

gaogaotiantian · 2023-12-24T07:33:19Z

Lib/test/_test_multiprocessing.py

+            util.info('2')
+            logger.debug('3')
+            filename = os.path.basename(__file__)
+            self.assert_log_lines([


I think having a utility function might be a bit of an overkill here. Your check is very thorough but it might be too strict. I think something like

output = stream.getvalue() self.assertIn(f'[INFO] [{filename}] 1', output) ...

should be good enough here. It's clear enough for people about what it's doing, and it's testing against the core issue this PR solves. Making sure the output matches exactly what's expected is not necessarily the best way to do it (especially if it makes the code harder to understand).

xu-song · 2023-12-24T07:50:02Z

Thanks for your instruction. I have made the requested changes.

serhiy-storchaka

LGTM.

miss-islington-app · 2023-12-24T10:04:16Z

Thanks @xu-song for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

…nGH-113423) (cherry picked from commit ce77ee5) Co-authored-by: Xu Song <xusong.vip@gmail.com>

bedevere-app · 2023-12-24T10:04:23Z

GH-113450 is a backport of this pull request to the 3.12 branch.

bedevere-app · 2023-12-24T10:04:28Z

GH-113451 is a backport of this pull request to the 3.11 branch.

…nGH-113423) (cherry picked from commit ce77ee5) Co-authored-by: Xu Song <xusong.vip@gmail.com>

serhiy-storchaka · 2023-12-24T10:05:30Z

Thank you @xu-song for your contribution and @gaogaotiantian for guiding it.

…13423) (GH-113451) (cherry picked from commit ce77ee5) Co-authored-by: Xu Song <xusong.vip@gmail.com>

…13423) (GH-113450) (cherry picked from commit ce77ee5) Co-authored-by: Xu Song <xusong.vip@gmail.com>

…nGH-113423)

Fix multiprocessing logger

772a742

bedevere-app bot added the awaiting review label Dec 23, 2023

xu-song mentioned this pull request Dec 23, 2023

multiprocessing logger get bad %(filename)s #113421

Closed

xu-song changed the title ~~GH-113422: Fix multiprocessing logger~~ GH-113421: Fix multiprocessing logger Dec 23, 2023

xu-song force-pushed the gh-113421-fix_multiprocessing_logger branch from 59d7229 to 772a742 Compare December 23, 2023 08:11

xu-song added 2 commits December 23, 2023 16:13

add news entry

0c71144

Add format unittest for multiprocessing logger

493414b

serhiy-storchaka self-requested a review December 23, 2023 10:15

serhiy-storchaka requested changes Dec 23, 2023

View reviewed changes

bedevere-app bot removed the awaiting review label Dec 23, 2023

bedevere-app bot added the awaiting changes label Dec 23, 2023

Initialize handler with io.StringIO()

5313957

xu-song requested a review from serhiy-storchaka December 24, 2023 02:59

xu-song marked this pull request as draft December 24, 2023 03:28

bedevere-app bot removed the awaiting changes label Dec 24, 2023

xu-song added 3 commits December 24, 2023 14:24

Add assert; Remove handler after test

f42181a

Add try finnaly block; Restore logger level

9705a0b

remove unused import

04509a7

gaogaotiantian reviewed Dec 24, 2023

View reviewed changes

Simplify unittest

a49b399

xu-song marked this pull request as ready for review December 24, 2023 07:53

bedevere-app bot added the awaiting review label Dec 24, 2023

serhiy-storchaka approved these changes Dec 24, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Dec 24, 2023

serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Dec 24, 2023

serhiy-storchaka merged commit ce77ee5 into python:main Dec 24, 2023

bedevere-app bot removed the awaiting merge label Dec 24, 2023

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 24, 2023

pythongh-113421: Fix multiprocessing logger for "%(filename)s" (pytho…

0a62f67

…nGH-113423) (cherry picked from commit ce77ee5) Co-authored-by: Xu Song <xusong.vip@gmail.com>

bedevere-app bot removed the needs backport to 3.12 only security fixes label Dec 24, 2023

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 24, 2023

pythongh-113421: Fix multiprocessing logger for "%(filename)s" (pytho…

305aa08

…nGH-113423) (cherry picked from commit ce77ee5) Co-authored-by: Xu Song <xusong.vip@gmail.com>

bedevere-app bot removed the needs backport to 3.11 only security fixes label Dec 24, 2023

serhiy-storchaka pushed a commit that referenced this pull request Dec 24, 2023

[3.11] gh-113421: Fix multiprocessing logger for "%(filename)s" (GH-1…

d04ca4c

…13423) (GH-113451) (cherry picked from commit ce77ee5) Co-authored-by: Xu Song <xusong.vip@gmail.com>

serhiy-storchaka pushed a commit that referenced this pull request Dec 24, 2023

[3.12] gh-113421: Fix multiprocessing logger for "%(filename)s" (GH-1…

1bff2fd

…13423) (GH-113450) (cherry picked from commit ce77ee5) Co-authored-by: Xu Song <xusong.vip@gmail.com>

ryan-duve pushed a commit to ryan-duve/cpython that referenced this pull request Dec 26, 2023

pythongh-113421: Fix multiprocessing logger for "%(filename)s" (pytho…

ffdadc7

…nGH-113423)

kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024

pythongh-113421: Fix multiprocessing logger for "%(filename)s" (pytho…

90aa8ea

…nGH-113423)

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-113421: Fix multiprocessing logger for "%(filename)s" (pytho…

9eb3270

…nGH-113423)

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-113421: Fix multiprocessing logger for "%(filename)s" (pytho…

f3923c8

…nGH-113423)

Uh oh!

GH-113421: Fix multiprocessing logger #113423

GH-113421: Fix multiprocessing logger #113423

Uh oh!

Conversation

xu-song commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 23, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Dec 23, 2023

Uh oh!

gaogaotiantian commented Dec 23, 2023

Uh oh!

bedevere-app bot commented Dec 23, 2023

Uh oh!

xu-song commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Dec 23, 2023

Uh oh!

xu-song commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaogaotiantian commented Dec 23, 2023

Uh oh!

gaogaotiantian left a comment

Choose a reason for hiding this comment

Uh oh!

gaogaotiantian Dec 24, 2023

Choose a reason for hiding this comment

Uh oh!

gaogaotiantian Dec 24, 2023

Choose a reason for hiding this comment

Uh oh!

xu-song commented Dec 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app bot commented Dec 24, 2023

Uh oh!

bedevere-app bot commented Dec 24, 2023

Uh oh!

bedevere-app bot commented Dec 24, 2023

Uh oh!

serhiy-storchaka commented Dec 24, 2023

Uh oh!

Uh oh!

xu-song commented Dec 23, 2023 •

edited

Loading

ghost commented Dec 23, 2023 •

edited by ghost

Loading

xu-song commented Dec 23, 2023 •

edited

Loading

xu-song commented Dec 23, 2023 •

edited

Loading

xu-song commented Dec 24, 2023 •

edited

Loading