Logging.warning vs. warnings.warn

akulakov · December 18, 2021, 1:41am

A question on the difference between logging.warning and warnings.warn came up in the review of my PR:

github.com/python/cpython

bpo-20907: shutil._unpack_zipfile add warnings for skipped files

python:main ← akulakov:20907-unpack_zipfile-add-warnings-for-skipped-files

opened 12:12AM - 04 Dec 21 UTC

akulakov

+22 -0

Note that test zip only has a '..' path component for testing, and no absolute p…ath. My zip archiver (as well as Python zip archiver) do not create absolute paths. It seems like all (?) modern zip archivers don't create absolute paths. If someone knows of an easy way to add such a path to the test zip file, I can update the unit test for that. https://bugs.python.org/issue20907

The logging how to has this to say:

https://docs.python.org/3/howto/logging.html#when-to-use-logging

As noted in the comments on the PR, there are a couple of places in the standard library that don’t follow the docs:

github.com

python/cpython/blob/eb483c46d62707bdf705491f76cf1fa9642fb47e/Lib/zoneinfo/_tzpath.py#L44

    
      
          
          
    raw_tzpath = env_var.split(os.pathsep)
              new_tzpath = tuple(filter(os.path.isabs, raw_tzpath))
          
          
    # If anything has been filtered out, we will warn about it
              if len(new_tzpath) != len(raw_tzpath):
                  import warnings
          
          
        msg = _get_invalid_paths_message(raw_tzpath)
          
          
        warnings.warn(
                      "Invalid paths specified in PYTHONTZPATH environment variable. "
                      + msg,
                      InvalidTZPathWarning,
                  )
          
          
    return new_tzpath
          
          

          
def _get_invalid_paths_message(tzpaths):
              invalid_paths = (path for path in tzpaths if not os.path.isabs(path))

github.com

python/cpython/blob/f42a06ba279c916fb67289e47f9bc60dc5dee4ee/Lib/zipfile.py#L1480

    
      
          @property
          def comment(self):
              """The comment text associated with the ZIP file."""
              return self._comment
          
          
@comment.setter
          def comment(self, comment):
              if not isinstance(comment, bytes):
                  raise TypeError("comment: expected bytes, got %s" % type(comment).__name__)
              # check for valid comment length
              if len(comment) > ZIP_MAX_COMMENT:
                  import warnings
                  warnings.warn('Archive comment is too long; truncating to %d bytes'
                                % ZIP_MAX_COMMENT, stacklevel=2)
                  comment = comment[:ZIP_MAX_COMMENT]
              self._comment = comment
              self._didModify = True
          
          
def read(self, name, pwd=None):
              """Return file bytes for name."""
              with self.open(name, "r", pwd) as fp:

I’ve opened an issue to track these two (and possibly more) instances where we diverge from the docs:

https://bugs.python.org/issue46068

I have also looked for examples of use of logging.warning in the standard lib and didn’t find any except for a few command line scripts. warnings.warn is used in quite a few places. (137 lines match, excluding deprecation warnings)

My PR has to do with the zipfile module, so the considerations are probably different than with command line scripts that are part of Python distributions.

Please share your thoughts on which of the two should be used in cases similar to my PR and in the two existing examples linked above. Thanks!

vsajip · December 18, 2021, 2:57pm

In those two examples you give, it may be that a developer can change the situation (comment length, environment variables) so it’s not clear-cutas to which of logging.warning() or warnings.warn() is more appropriate. Also, as Raymond said in the issue, there’s no need to change anything if the status quo isn’t leading to a specific problem. The “How to use logging” docs are advisory and are not specifically aimed at core developers (the stdlib could be considered a special case for certain things).

rhettinger · December 18, 2021, 6:59pm

For those not reading the tracker issues and PR’s in detail, here’s a quick summary

The OP got conflicting advice on two PRs about whether to use logging.warn() or warning.warn(). He is seeking a solid rule that applies to all cases (his current PR is changes existing, stable code in an effort to apply a universal rule as he understands it).

My thoughts:

The new PR is a breaking change and should not be done without a strong motivation.
In general, the Python standard library should only use the warnings module in our library code. Partly, this is because we can’t know in advance how users are configuring their loggers. Party, this is because the warnings module provides rich standardized controls over filtering, reporting frequency, etc, We should not deprive tool makers of those controls. Partly, this is because we don’t want users to grapple with different mechanisms depending on the module being used.

I also have a general thought about this kind of PR. IMHO, beginning core devs should focus on specific cases with a known user issue or well motivated feature request. This allows concrete reasoning about a specific problems where the pros and cons tend to be knowable.

In general, a new dev should not form notions of absolute coding rules and then sweep through the standard library changing every example they can find. This puts them in the position of second guessing code written by the seasoned developers who came before them, code that has survived deployment and widespread use, and code that has no reported issues. This is perilous even for seasoned devs – more than one of us has made mistakes and trampled correct or mostly correct but stable code during a sweep.

Also, the quest for absolute rules that apply everywhere is a bit dubious, coding is more nuanced than that. Further, if such a sweep does get made, it should be done one PR at a time and assigned to either the module maintainer, the person who wrote the original code, or anyone else who has wrestled with what that module is trying to accomplish.

akulakov · December 20, 2021, 7:11pm

Vinay:
I’m not sure how an arbitrary invalid tzpath can be changed by developer into a valid path. In second case, the developer [EDIT: the current logic is already] changing the comment length, the warning is to notify that the length was changed. It makes sense that HOWTO docs are advisory and that more generally rules set out in the documentation may not apply to Stdlib and Python core. I was thinking that may be the case but wasn’t sure.

Raymond:
I want to clarify that there’s only one PR as far as I know, it introduces new warning but doesn’t change any existing warnings. There’s no other PRs. I’ve created the issue to track the contradictions between the HOWTO advice and instances in Stdlib; as well as to get exactly the type of feedback that you’ve given. I’ve only recently started to contribute to Python so it makes perfect sense not to make sweeping changes.

encukou · December 21, 2021, 1:34pm

I don’t think there’s a difference here between new and old core devs. Sweeping changes across the codebase should always be done extremely carefully, no matter how experienced one is.

But logging has rich controls as well, just different ones.
It’s a bit sad that there are two ways to do things, TBH. I wonder if logging.captureWarnings(True) should be considered best practice, and libraries that can do with warnings and exceptions (like most of stdlib) should be encouraged to do so?

EpicWink · December 21, 2021, 1:48pm

In my experience logging (level INFO and above) is generally used in applications (and tools, across, etc) and warnings are used in libraries. There are of course exceptions, but the difference does make sense.

storchaka · December 21, 2021, 3:13pm

Warnings in the zipfile module replaced prints:

    if self.debug:
        print(...)

See Issue 20262: Convert some debugging prints in zipfile to warnings - Python tracker.

The advantage of warnings is that they can be converted to errors with corresponding filters.

I think that it may be time to replace these warnings with errrors.