Skip to content

fix(font manager): Allow Rebuilding Cache #30406

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Servinjesus1
Copy link

One could never rebuild the cache given the existing conditional logic (premature return to exception based solely on fallback_to_default). Now, if a user disables fallback and enables rebuild, the rebuild logic will execute.

This allows a user to install a font and run:

import matplotlib.font_manager as fm
fm.findfont("Font Spec", fallback_to_default=False, rebuild_if_missing=True)

to force a rebuild of the font cache.

It was a simple fix to the conditional logic: if fallback is False, don't return with error if rebuild is True, but instead set result to empty string (used later in the rebuild code) so that the rebuild code can actually be proceeded to.

PR summary

PR checklist

  • new and changed code is tested

Tested on my font cache locally in my normal non-dev env.
Unless other ideas exist, a unit test would require:

  1. A dummy font file
  2. A way to create a temporary cache that does not affect the user's cache
  3. Updating of said cache with dummy font using above code
  4. Validation that cache has been updated
  5. Someone who's written unit tests (not me)

One could never rebuild the cache given the existing conditional logic (premature return to exception based solely on fallback_to_default). Now, if a user disables fallback and enables rebuild, the rebuild logic will execute.
@Servinjesus1
Copy link
Author

Failed tests include the garbage collector holding on to locals following production of a graphic involving text with a non-existent font family, and warnings in usetex regarding unavailable serif.

The new logic pathway opened up by this change could be old. It includes a self-reference which may be when failure 2 is produced. Not sure how locals could be held onto by way of this code, however (failure 1)

@@ -1504,6 +1504,9 @@ def _findfont_cached(self, prop, fontext, directory, fallback_to_default,
default_prop.set_family(self.defaultFamily[fontext])
return self.findfont(default_prop, fontext, directory,
fallback_to_default=False)
elif rebuild_if_missing:
# Continue to rebuild condition
result = ''
Copy link
Member

Choose a reason for hiding this comment

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

Can we use None hear instead (which might require adjusting the check below) setting the result to empty string is a bit odd.

Copy link
Author

Choose a reason for hiding this comment

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

I chose a value that is "not-filey" because I don't know what isfile does with a None, and modifying the check below has the potential to fundamentally change the logic pathway, so I didn't feel comfortable changing that. Open to alternatives, though.

@tacaswell
Copy link
Member

I don't quite understand what you mean by that. Are you saying the the text related tests that are failing should not have been failing (as a rebuild of the fontcache discovers a font that we should select)?

@Servinjesus1
Copy link
Author

@tacaswell The tests that are failing are:

  1. test_font_manager.py: test_donot_cache_tracebacks. This test basically creates a text object using a font that doesn't exist in a figure that's saved while a dummy class is instantiated in the scope of an inner calling function. To my understanding, something about text must then have the potential to cache locals when the font doesn't exist, which is bewildering. This should not find a new font and should trigger a cache rebuild, after which it still shouldn't find a font. However, when _load_font_manager is called and it generates a new font manager, I can't understand how that would trigger saving locals. Otherwise, the code returns to logic paths that I didn't modify. So, in summary, I can't imagine how my change is the direct cause of this failure.
  2. test_legend.py: test_usetex_no_warn. The assertion that fails is that "Font family ['serif'] not found." is found in caplog.text when constructing a legend using usetex. _findfont_cache will warn about fonts not found regardless of my change, the only difference now is that a cache rebuild is triggered first, and then the warning is generated.

In short, I don't see how this change is a direct cause of these tests, though I recognize my understanding is limited. I made this pull request to solve a very simple issue: in no way can a user - using public methods - tell matplotlib to rebuild its font cache, and instead the default MO has been to tell users to manually delete the cache (c.f., I don't consider adding a font to be a viable workaround for rebuilding the cache).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

3 participants