Skip to content

Capture and log messages emitted by C modules when importing them #1514

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

Merged
merged 4 commits into from
May 23, 2022

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Apr 17, 2022

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

Pipe stdout to stderr Capture and log messages emitted while importing C modules. This prevents contaminating programmatic output, e.g. pylint's JSON reporter.

An alternative would be to silence stdout altogether, but there may be users who appreciate the notice that code is being executed. This way no information is lost, while still allowing clean programmatic output.

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes pylint-dev/pylint#3518

/cc @piotr-machura, what do you think of this approach?

@coveralls
Copy link

coveralls commented Apr 17, 2022

Pull Request Test Coverage Report for Build 2374232989

  • 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 91.916%

Totals Coverage Status
Change from base Build 2371242414: 0.01%
Covered Lines: 9233
Relevant Lines: 10045

💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Apr 17, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone Apr 17, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I'm wondering if the solution would be to add debug logging to astroid. It seems to me that this output is not error so technically should not be printed to stderr (although it's convenient to do so because we would not have to setup logging). I don't think we have logging in astroid yet, but we don't have to do the configuration, probably just clarifying in the documentation that you need to activate logging with debug level if you want to see the stdout from loading c module.

@jacobtylerwalls
Copy link
Member Author

It seems to me that this output is not error so technically should not be printed to stderr (although it's convenient to do so because we would not have to setup logging).

Thanks, I thought about this for a bit. I think the fact that astroid imports C modules (that are allowlisted, or even just that have a brain, see #562), is worth warning the user, and that's an okay use for stderr. "Welcome to pygame!" is not an error in pygame, but IMO when it happens in astroid it's worth a warning because it could be unexpected.

I found a couple interesting things at the Python logging docs for library developers:

If the using application does not use logging, and library code makes logging calls, then (as described in the previous section) events of severity WARNING and greater will be printed to sys.stderr. This is regarded as the best default behaviour.

So I think even if we go the logging route we'll end up generating messages in stderr, is that right?

@jacobtylerwalls
Copy link
Member Author

Ah, maybe we could wrap it in a UserWarning or something that can be filtered idiomatically.

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Apr 18, 2022

So I think even if we go the logging route we'll end up generating messages in stderr,

I would use the "info" log level for what comes from stdout, debug is a little low. If the output comes from stderr originally we should use the error level though. Reading your documentation it seems we should add a NullHandler so nothing is printed unless lib users add a configuration.

If for some reason you don’t want these messages printed in the absence of any logging configuration, you can attach a do-nothing handler to the top-level logger for your library. This avoids the message being printed, since a handler will always be found for the library’s events: it just doesn’t produce any output. If the library user configures logging for application use, presumably that configuration will add some handlers, and if levels are suitably configured then logging calls made in library code will send output to those handlers, as normal.

A do-nothing handler is included in the logging package: NullHandler (since Python 3.1). An instance of this handler could be added to the top-level logger of the logging namespace used by the library (if you want to prevent your library’s logged events being output to sys.stderr in the absence of logging configuration).

import logging
logger = logging.getLogger(astroid).addHandler(logging.NullHandler())

# (logger = logging.getLogger(__file__))
...
if stderr:
    logger.error(...)
else:
    logger.info(...)

@jacobtylerwalls jacobtylerwalls changed the title Pipe stdout to stderr when importing C modules Capture and log messages emitted by C modules when importing them Apr 19, 2022
jacobtylerwalls and others added 2 commits May 23, 2022 16:11
This prevents contaminating programmatic output, e.g. pylint's JSON reporter.

Co-authored-by: Pierre Sassoulas <[email protected]>
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM, that's a nice addition to 2.12 !

@@ -5,7 +5,6 @@ coveralls~=3.3
coverage~=6.3.3
pre-commit~=2.19
pytest-cov~=3.0
contributors-txt>=0.7.3
Copy link
Member Author

@jacobtylerwalls jacobtylerwalls May 23, 2022

Choose a reason for hiding this comment

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

@Pierre-Sassoulas did you want to remove this in another MR? I reverted this change here.

Copy link
Member

Choose a reason for hiding this comment

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

It happened to prevent installing with pip locally I guess it's not that important. (Duplicated entry)

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

Successfully merging this pull request may close these issues.

Pylint not suppressing the stdout of C loaded extensions
3 participants