Skip to content

Some more fixes for the verbose -> logging switch. #9761

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

Closed
wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 13, 2017

followup to #9313. would also like to rework logging of subprocess calls (basically, provide a logging wrapper in matplotlib.compat.subprocess instead of doing it again and again) but that'll happen after a decision is reached on #9639.

Technically, this PR changes the behavior of get_home(), get_cachedir(),
etc. making them insensitive to changes in the relevant environment
variables after matplotlib is imported. In practice I strongly doubt
that we supported that use case to begin with (because the values are
probably cached in other places); I am also happy with saying "don't do
this" until someone brings up a good reason to change the cache dir in
the middle of the program...

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer force-pushed the more-logging branch 2 times, most recently from da0b7fd to 0efa52f Compare November 13, 2017 03:10
@efiring
Copy link
Member

efiring commented Nov 14, 2017

It's not clear to me what this is "fixing"--is the point of it the change in behavior you describe above, the freezing of the returned value? Furthermore, the lru_cache(1) usage seems like an odd idiom--almost an oxymoron. What is the advantage over a simple property?

@anntzer
Copy link
Contributor Author

anntzer commented Nov 14, 2017

get_home and friends cannot be properties because they are not methods (well, see https://www.python.org/dev/peps/pep-0549/, not that it matters for us).

The point is that the previous implementation checked whether get_home (etc.) has already been called; regardless of whether it has already been called, the internal (_get_home) implementation would be called, and if this is the first call, the result would be logged (implicitly, this behavior assumes that _get_home always returns the same result (which is a reasonable assumption) so further calls do not need to be logged).

Instead, the proposed implementation first wraps the internal implementation (_get_home) into a logging wrapper (level-1 wrapper) (so that whenever the level-1 wrapper is called, the result is logged); then wrapped that guy into a memoizing wrapper (level-2). Because get_home takes no arguments, every call from the second one is going to hit the (single-value) cache, and thus not going to go to the level-1 wrapper. So we return the same value every time and don't need to track ourselves whether we have already been called. [Obviously it is easy to write this as a decorator ourselves (in fact this is what _wrap was). But the point is that this decorator is already provided by the stdlib, under the lru_cache name...]

@efiring
Copy link
Member

efiring commented Nov 14, 2017

Yes, I realized right after pressing "comment" that I didn't really mean "property"--but rather the sort of structure used with a property getter:

_user_home = 0  # dummy flag prior to first call, since we can't use None
def get_home():
    """Find user's home directory if possible.
    Otherwise, returns None.
    """
    if _user_home is not 0:
        return _user_home
    _user_home = None
    if six.PY2 and sys.platform == 'win32':
        path = os.path.expanduser(b"~").decode(sys.getfilesystemencoding())
    else:
        path = os.path.expanduser("~")
    if os.path.isdir(path):
        _user_home = path
    else:
        for evar in ('HOME', 'USERPROFILE', 'TMP'):
            path = os.environ.get(evar)
            if path is not None and os.path.isdir(path):
                _user_home = path
                break
    _log.log(logging.INFO, '$HOME=%s', _user_home)
    return _user_home

(Untested, probably can be written better.) Advantage: everything is explicit and in one place; it can be read without having to track down the decorators and figure out how they work.

@anntzer
Copy link
Contributor Author

anntzer commented Nov 14, 2017

I don't feel very strongly about the PR (I think the current situation is not so clear, your approach of making everything explicit would work too).

I just realized why the PR name was confusing: as mentioned in the original message I was initially planning to also fix subprocess calls (by moving the log to matplotlib.compat.subprocess.*, rather than having to repeat them every time), but I realized this would conflict with my other PR (on executable resolution) so I decided to leave that out for now, and ended up with a somewhat partial PR.

@Kojoley
Copy link
Member

Kojoley commented Nov 17, 2017

Why we are calculating paths on demand rather not on module import?

@anntzer
Copy link
Contributor Author

anntzer commented Nov 17, 2017

They are effectively computed at import time, the functions are only there to factor out common code and avoid polluting the module namespace with intermediate variables.

@tacaswell tacaswell added this to the v2.2 milestone Nov 20, 2017
@@ -578,7 +558,21 @@ def checkdep_usetex(s):
return flag


def _get_home():
def _log_result(fmt, level="INFO", func=None):
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to use this, it could use a comment along the lines of the explanation in the comments above. I understand it now, sort of, but would never have understood it if it wasn't explained, just due to an unfamiliarity w/ functools. OTOH, I can follow @efiring code. I think the advantage here over @efiring is that you've made it so all the different calls don't need to have the same logic to avoid duplicate system calls. Does that make the name a bit of a misnomer? The real purpose is the wrapping, isn't it?

@anntzer
Copy link
Contributor Author

anntzer commented Dec 28, 2017

I pushed a new version which should be hopefully a bit clearer and better documented.

Technically, this PR changes the behavior of get_home(), get_cachedir(),
etc. making them insensitive to changes in the relevant environment
variables *after* matplotlib is imported.  In practice I strongly doubt
that we supported that use case to begin with (because the values are
probably cached in other places); I am also happy with saying "don't do
this" until someone brings up a good reason to change the cache dir in
the middle of the program...
@anntzer
Copy link
Contributor Author

anntzer commented Jun 23, 2018

Superseded (essentially) by #11398.

@anntzer anntzer closed this Jun 23, 2018
@anntzer anntzer deleted the more-logging branch June 23, 2018 23:28
@story645 story645 removed this from the future releases milestone Oct 6, 2022
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.

6 participants