Refactoring the logging module by jauy123 · Pull Request #5367 · MDAnalysis/mdanalysis

jauy123 · 2026-04-23T05:14:41Z

Initial draft that address #5357 and #5368. It should be a quick relatively painless change to implement.

Changes made in this Pull Request:

LLM / AI generated code disclosure

LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes / no

PR Checklist

Issue raised/referenced?
Tests updated/added?
Documentation updated/added?
package/CHANGELOG file updated?
Is your name in package/AUTHORS? (If it is not, add it!)
LLM/AI disclosure was updated.

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.

📚 Documentation preview 📚: https://mdanalysis--5367.org.readthedocs.build/en/5367/

Need to add a fmt_string parameter if want to replicate earlier behavior

codecov · 2026-04-23T15:08:59Z

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.84%. Comparing base (9269430) to head (ab00172).
⚠️ Report is 3 commits behind head on develop.

Files with missing lines	Patch %	Lines
package/MDAnalysis/lib/log.py	85.71%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5367      +/-   ##
===========================================
- Coverage    93.84%   93.84%   -0.01%     
===========================================
  Files          182      182              
  Lines        22497    22495       -2     
  Branches      3200     3202       +2     
===========================================
- Hits         21113    21111       -2     
  Misses         922      922              
  Partials       462      462

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jauy123 · 2026-04-23T19:17:06Z

Can I ask for some feedback here? I feel this code here is fairly straightforward and readable, and it preserves existing behavior of the old code as well.

I'm a bit confused on where to write the tests. There are two test_log.py files, one in lib/ and utils/. I think I should write the new tests in utils?

jauy123 · 2026-04-23T19:18:01Z

I also need to write docs as well

jauy123 · 2026-04-23T19:22:39Z

+    create(
+        "MDAnalysis",
+        stream=stream,
+        level="DEBUG",
+        fmt="%(asctime)s %(name)-12s %(levelname)-8s %(message)s",
+    )
+    create(
+        "MDAnalysis",
+        stream=sys.stdout,
+        level="INFO",
+        fmt="%(name)-12s: %(levelname)-8s %(message)s",
+    )


All of this is intend to replicate old behavior from the original.

jauy123 · 2026-04-23T19:25:48Z

@@ -112,7 +120,9 @@ def stop_logging():
    clear_handlers(logger)  # this _should_ do the job...




In general, I guess this PR make create() a more general type of function which add handlers to a master logger, so should it get renamed?

Probably yes to renaming, but you'd need to

deprecate the old function

schedule for removal in 3.0.0

because we're following semantic versioning https://semver.org and we cannot remove documented functionality in a public function until we make a new major release.

Similarly, we cannot just rename kwargs in a public function like create(). You have to keep the old one around and deprecated them.

See #5367 (comment)

Probably should raise an issue of it as well

orbeckst

Overall this looks good and does not generate a lot more code. I'd be supportive of the change.

You'll need to deprecate kwargs if you change them. ALternatively, leave them and say in the docs that "logfile" can be any file-like or stream-like object.

I see that's WIP so you know that you'll also eventually need docs, tests, and CHANGELOG.

orbeckst · 2026-04-23T22:35:12Z



-def start_logging(logfile="MDAnalysis.log", version=version.__version__):
+def start_logging(stream="MDAnalysis.log", version=version.__version__):


Under semantic versioning, you need to maintain backwards compatibility. So something like

def start_logging(stream="MDAnalysis.log", logfile=None, version=version.__version__): if logfile is not None: warnings.warn(DeprecationWarning, "logfile kwarg will be removed in MDAnalysis 3.0.0, use stream instead" # we probably have a dedicated function for the deprecation warning... stream = logfile ...

orbeckst · 2026-04-23T22:37:54Z


-def create(logger_name="MDAnalysis", logfile="MDAnalysis.log"):
+def create(
+    logger_name="MDAnalysis", stream="MDAnalysis.log", level="DEBUG", fmt=None


as above, you need to deprecate kwargs if you really want to rename them

jauy123 · 2026-04-24T04:26:10Z

create() should be renamed into add_handler(). clear_handlers() should be renamed to clear_handler(handler, all=False) where all is a boolean that if enable clears all handlers from the logger.

jauy123 · 2026-04-24T04:40:05Z

Random thing I noticed there are so many unused imports in this module. I think my ide counted at least five modules that weren't be being used but was imported still.

jauy123 · 2026-04-24T04:44:22Z

-class RedirectedStderr(object):
-    """Temporarily replaces sys.stderr with *stream*.
-
-    Deals with cached stderr, see
-    http://stackoverflow.com/questions/6796492/temporarily-redirect-stdout-stderr
-    """
-
-    def __init__(self, stream=None):
-        self._stderr = sys.stderr
-        self.stream = stream or sys.stdout
-
-    def __enter__(self):
-        self.old_stderr = sys.stderr
-        self.old_stderr.flush()
-        sys.stderr = self.stream
-
-    def __exit__(self, exc_type, exc_value, traceback):
-        self._stderr.flush()
-        sys.stderr = self.old_stderr
-
-
-@pytest.fixture()
-def buffer():
-    return StringIO()
-
-
-def _assert_in(output, string):
-    assert (
-        string in output
-    ), "Output '{0}' does not match required format '{1}'.".format(
-        output.replace("\r", "\\r"), string.replace("\r", "\\r")
-    )


If you were to grep -F -r for these functions or classes, they don't exist anywhere else in the testsuite. So they should be removed!

jauy123 · 2026-04-24T04:48:39Z

-class NullHandler(logging.Handler):
-    """Silent Handler.
-
-    Useful as a default::
-
-      h = NullHandler()
-      logging.getLogger("MDAnalysis").addHandler(h)
-      del h
-
-    see the advice on logging and libraries in
-    http://docs.python.org/library/logging.html?#configuring-logging-for-a-library
-    """
-
-    def emit(self, record):
-        pass
-
-


jauy123 · 2026-04-24T06:16:56Z

-def test_start_stop_logging():
-    try:
-        MDAnalysis.log.start_logging()
-        logger = logging.getLogger("MDAnalysis")
-        logger.info("Using the MDAnalysis logger works")
-    except Exception as err:
-        raise AssertionError("Problem with logger: {0}".format(err))
-    finally:
-        MDAnalysis.log.stop_logging()


This test doesn't really test functionality or properties of the mda.start/stop_logging() functions. It just tests if the command succeeds. Need a rewrite

jauy123 · 2026-04-24T06:24:30Z

+    for h in list(logger.handlers):
        logger.removeHandler(h)


There's a really subtle bug with the original version. logger.removeHandler(h) mutates the original logger.handlers so it "skips" handlers in the list. Putting in a list creates a copy, so the entire elements get deleted

See test_stop_logging under TestConvenienceFunctions. The original version without list would have failed that test.

intial pseudocode

33a8005

jauy123 changed the title ~~intial pseudocode~~ Exposing the stream in the logging module Apr 23, 2026

jauy123 added 4 commits April 22, 2026 22:58

inital thought code (hadn't run it yet)

1bf0ee1

more comments

24a6c31

general cleanup

7a5c0b3

Post black

f1be0df

Need to add a fmt_string parameter if want to replicate earlier behavior

jauy123 added 5 commits April 23, 2026 11:28

removed loop based control

dbcfb1c

Added formatter to handler

93966fb

Added format to start_logging() to replicate old behavior

c5f6133

Applied flake8

e30d0ab

notes for test later

5a5906d

fix linter

f679251

jauy123 commented Apr 23, 2026

View reviewed changes

Comment thread package/MDAnalysis/lib/log.py Outdated

jauy123 commented Apr 23, 2026

View reviewed changes

Comment thread package/MDAnalysis/lib/log.py Outdated

jauy123 commented Apr 23, 2026

View reviewed changes

Comment thread testsuite/MDAnalysisTests/utils/test_log.py Outdated

orbeckst linked an issue Apr 23, 2026 that may be closed by this pull request

Exposing the stream in the logging module #5357

Open

orbeckst requested changes Apr 23, 2026

View reviewed changes

jauy123 added 2 commits April 23, 2026 20:30

put logger back to start_logging()

2ba1c2e

cleaned up create()

cca9258

jauy123 mentioned this pull request Apr 24, 2026

lib/log/NullHandler should be removed and be replaced with logging.NullHandler #5368

Open

removed MDAnalysis NullHandler

94f056f

jauy123 changed the title ~~Exposing the stream in the logging module~~ Refactoring the logging module Apr 24, 2026

readded top level import -- it broke some tests

7f5812f

ported over tests

ba9315e

jauy123 commented Apr 24, 2026

View reviewed changes

Used black

0e36231

jauy123 commented Apr 24, 2026

View reviewed changes

Added Tests for start_logging() and stop_logging()

e63f5ef

jauy123 commented Apr 24, 2026

View reviewed changes

jauy123 added 4 commits April 23, 2026 23:25

Added comment

a81e4ad

Added comment

f25aec7

post black

bdaf241

Merge branch 'logging' of github.com:jauy123/mdanalysis into logging

ab00172

		@@ -112,7 +120,9 @@ def stop_logging():
		clear_handlers(logger) # this _should_ do the job...



		def start_logging(logfile="MDAnalysis.log", version=version.__version__):
		def start_logging(stream="MDAnalysis.log", version=version.__version__):

Conversation

jauy123 commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LLM / AI generated code disclosure

PR Checklist

Developers Certificate of Origin

Uh oh!

codecov Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jauy123 commented Apr 23, 2026

Uh oh!

jauy123 commented Apr 23, 2026

Uh oh!

jauy123 Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jauy123 Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orbeckst Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

jauy123 Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

jauy123 Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orbeckst Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jauy123 commented Apr 24, 2026

Uh oh!

jauy123 commented Apr 24, 2026

Uh oh!

jauy123 Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jauy123 Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

jauy123 Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jauy123 Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jauy123 commented Apr 23, 2026 •

edited

Loading

codecov Bot commented Apr 23, 2026 •

edited

Loading

jauy123 Apr 23, 2026 •

edited

Loading

jauy123 Apr 24, 2026 •

edited

Loading

jauy123 Apr 24, 2026 •

edited

Loading

jauy123 Apr 24, 2026 •

edited

Loading