Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
anntzer wants to merge1 commit intomatplotlib:masterfromanntzer:more-logging

Conversation

anntzer
Copy link
Contributor

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
variablesafter 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

@anntzeranntzerforce-pushed themore-logging branch 2 times, most recently fromda0b7fd to0efa52fCompareNovember 13, 2017 03:10
@efiring
Copy link
Member

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, thelru_cache(1) usage seems like an odd idiom--almost an oxymoron. What is the advantage over a simple property?

@anntzer
Copy link
ContributorAuthor

get_home and friends cannot be properties because they are not methods (well, seehttps://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 wrappedthat guy into a memoizing wrapper (level-2). Becauseget_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 commentedNov 14, 2017
edited
Loading

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 Nonedefget_home():"""Find user's home directory if possible.    Otherwise, returns None.    """if_user_homeisnot0:return_user_home_user_home=Noneifsix.PY2andsys.platform=='win32':path=os.path.expanduser(b"~").decode(sys.getfilesystemencoding())else:path=os.path.expanduser("~")ifos.path.isdir(path):_user_home=pathelse:forevarin ('HOME','USERPROFILE','TMP'):path=os.environ.get(evar)ifpathisnotNoneandos.path.isdir(path):_user_home=pathbreak_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
ContributorAuthor

I don't feelvery 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

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

@anntzer
Copy link
ContributorAuthor

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.

@tacaswelltacaswell added this to thev2.2 milestoneNov 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. Ithink 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
ContributorAuthor

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 environmentvariables *after* matplotlib is imported.  In practice I strongly doubtthat we supported that use case to begin with (because the values areprobably cached in other places); I am also happy with saying "don't dothis" until someone brings up a good reason to change the cache dir inthe middle of the program...
@anntzer
Copy link
ContributorAuthor

Superseded (essentially) by#11398.

@anntzeranntzer deleted the more-logging branchJune 23, 2018 23:28
@story645story645 removed this from thefuture releases milestoneOct 6, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jklymakjklymakjklymak left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@anntzer@efiring@Kojoley@jklymak@tacaswell@story645

[8]ページ先頭

©2009-2025 Movatter.jp