8000 Log about font manager generation beforehand. by ngnpope · Pull Request #16374 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Log about font manager generation beforehand. #16374

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 2, 2020
Merged

Log about font manager generation beforehand. #16374

merged 1 commit into from
Feb 2, 2020

Conversation

ngnpope
Copy link
Contributor
@ngnpope ngnpope commented Jan 30, 2020

PR Summary

Changes an info level log message to debug. This reduces noise on the root logger, e.g.

2020-01-30 13:45:33,150 INFO matplotlib.font_manager generated new fontManager
2020-01-30 13:45:33,150 INFO matplotlib.font_manager generated new fontManager

It's not very descriptive or helpful in general.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 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

@jklymak jklymak requested a review from anntzer January 30, 2020 20:59
@jklymak
Copy link
Member
jklymak commented Jan 30, 2020

This should rarely ever happen, though, hence it was pushed up to info I think. Its also slow and users wanted to know why matplotlib was locking up. However, I agree the message could be clearer?

@anntzer
Copy link
Contributor
anntzer commented Jan 30, 2020

(I don't really care about the log level, but yes, this should only happen once...)

@ngnpope
Copy link
Contributor Author
ngnpope commented Jan 30, 2020

This should rarely ever happen, though, hence it was pushed up to info I think. Its also slow and users wanted to know why matplotlib was locking up. However, I agree the message could be clearer?

Is it that slow? I hadn't really noticed. Is the speed dependent on how many fronts you have installed?

I suggested this change because it didn't seem all that useful to know in general. If users really needed to work out what was going on they could enable debug level logging.

Also this message occurs after the slow bit based on how it is phrased? Surely beforehand saying "Generating new font manager, this may take some time..." would make more sense?

(I don't really care about the log level, but yes, this should only happen once...)

Sorry, it should be only once - I think that I added a separate handler for matplotlib to only log at info level due to the excess noise at debug level and forgot to stop propagation.

@jklymak
Copy link
Member
jklymak commented Jan 30, 2020

By only "once" we mean it should only happen once ever, unless you clear your font cache.

@ngnpope
Copy link
Contributor Author
ngnpope commented Jan 30, 2020

Ah, yes. So I see it a lot because I have multiple docker containers spawned in parallel and the cache is lost when containers are shut down, so it is always rather than once. I guess that it should be possible to change the cache path to put it on a volume and share/reuse it?

That is also probably why it doesn't take (too) much time because there are limited fonts installed within the container.

@jklymak
Copy link
Member
jklymak commented Jan 30, 2020

Can you just make .matplotlibrc part of your container?

@tacaswell
Copy link
Member

It does depend on the number of fonts you have installed (at one point we were looking at the full OSX system font directory and people were reporting ~minutes of run time).

The MPLCONFIGDIR env (https://matplotlib.org/faq/environment_variables_faq.html?highlight=mplconfigdir#envvar-MPLCONFIGDIR) controls where the font cache goes.

I suggest importing matplotlib as part of building the docker container (python -c "import matplotlib" should be enough). It will make the docker container marginally bigger, but make your start up marginally faster.

@ngnpope
Copy link
Contributor Author
ngnpope commented Jan 31, 2020 •
edited
Loading

Ok. So I see this really amounts to a configuration issue for me to address, although it wasn't obvious. I'll look into your suggestions, @tacaswell - Thanks.

I've tweaked this patch to restore the info level logging, but moved the message ahead of the generation - after all, it makes sense to tell the user that something is about to happen that takes ~minutes, rather than saying "oh, by the way that big pause was because..."

Thanks everyone.

@ngnpope ngnpope changed the title Changed an info log message to debug. Log about font manager generation beforehand. Jan 31, 2020
@timhoffm timhoffm added this to the v3.3.0 milestone Feb 2, 2020
@timhoffm
Copy link
Member
timhoffm commented Feb 2, 2020

Thanks!

@timhoffm timhoffm merged commit b47fc9d into matplotlib:master Feb 2, 2020
@ngnpope ngnpope deleted the patch-1 branch February 2, 2020 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm approved these changes

@dstansby dstansby dstansby approved these changes

@anntzer anntzer Awaiting requested review from anntzer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ngnpope @jklymak @anntzer @tacaswell @timhoffm @dstansby
0