Type-1 font subsetting #20716
Type-1 font subsetting #20716
Conversation
f6861ad to
d8ae364
Compare
|
Is this ready for review, now that #20715 has been merged? |
|
This is failing on Ubuntu 22.04 and Windows but passing on 24.04 and Mac. Here's one failing image (test_usetex_pdf.png, so converted from pdf to png on the test system).
This looks like the font is entirely broken. The expected image similarly converted looks like this:
Strangely enough, the generated pdf file looks fine on my Mac. |
|
I can repeat the error running on an Ubuntu 22.04 docker image: The subsetting is wrong in some way that breaks Ghostscript 9.55 but not the viewer in macOS or the newer Ghostscript in Ubuntu 24.04. (Ghostscript 9.56 has a completely rewritten PDF interpreter.) |
|
I hope I found the culprit... I was writing an extra delimiter between the Subrs and the Charstrings when one was already there. |
9c5d971 to
9a9dc05
Compare
lib/matplotlib/_type1font.py
Outdated
| @@ -35,10 +37,64 @@ | |||
|
|
|||
| from matplotlib.cbook import _format_approx | |||
| from . import _api | |||
| if T.TYPE_CHECKING: | |||
| from collections.abc import Iterable | |||
There was a problem hiding this comment.
We tend not to have inline type hints (and I personally very much don't like them), but there are a few exceptions (e.g. _mathtext.py) so I guess it's up to you whether you want to leave them in.
There was a problem hiding this comment.
Right – I saw that there are some files with type hints, and figured that the project might be in the process of adding them. I've found type hints pretty useful at work, but of course we should have a consistent style in the project. Has this been discussed in the past on the dev list or somewhere?
There was a problem hiding this comment.
I gave my opinion on our gitter, but I'll copy the main comment here for the sake of consolidation/potential for finding it in the future:
I would say type hints are a net positive in my opinion, though I acknowledge that there are problems (perhaps especially in a case like ours where APIs were designed well before type hints).
We went with stub files on initial implementation specifically to minimize some risk, specifically since the stub files have are not even loaded at runtime, there was no chance of them interfering.
However, I do think inline reduces a level ongoing maintenance risk, in particular the chance of the two files getting out of sync. (We have tooling/CI in place to help catch such things, but don't think it fully removes the risk)
The other factor is that stub files allow us draw the line at public APIs, and not have to worry about typing our internal logic. (Which has positives and negatives, positives being largely catching additional problems by type checker, negative largely being the surface area that needs to be covered.)
So in all, I think my personal recommendation would be a relatively slow uptake, where we keep the stub files for most public facing things in the interim, do inline type hints for internal logic when it makes sense to do so (e.g. when doing so is actively helpful for some refactor/feature addition/etc) then once the majority of internal logic has inline hints, consider inlining the hints of the more public facing APIs.
So looking at this example in particular, I think this does fall within my recommendation there. I am not personally opposed to it, but also not hard pulling for it. I do question a bit whether to advocate for "do minimal as you are working and motivated" or "the unit for adding type hints should be one file at a time". Doing the latter would carry the advantage that you get a better sense of how complete the hints are, but the disadvantage of asking people to type hint 3-4x what they were otherwise looking at (in this case, for example), which also impacts the review-ability of the PR as there are lots of changes that are actually orthogonal.
For what its worth, this particular file already has one function which got an inline typehint added in #27796
There was a problem hiding this comment.
and @tacaswell responded
It is worth re-opening the discussion of in-line type hints
the world seems to have stabilized and in other projects I have had type hints catch actual bugs before I ran the code (but I have also had to spend 15 minutes trying to sort out how to placate the type checker for code that clearly works a couple of times)
The type hints were useful for me while working on this, since VS Code pointed out some obvious mistakes in real time. But I agree that it is not ideal to leave the file only partially annotated, since it does not pass a full type check in its current state.
I could remove the extra type hints from this PR and make a separate PR to add hints to the whole file?
There was a problem hiding this comment.
Sure, let's just go for it. I'm not going to be able to resist this forever 🙄
The old optimization where the same font file and descriptor could be shared by multiple differently-encoded fonts is wrong in the presence of subsetting, unless we also take the exact glyph subset into account. It is very unlikely for the exact same subset to be used by different encodings, so just remove the optimization. Rearrange _embedTeXFont in neater blocks.
|
It looks like you'll need to install |
Yes, but Debian (and hence Ubuntu) bundles them into [Edit: Or we could see if we can install packages via tlmgr. Debian recommends against it but I don't think we need to worry about messing with package dependencies in a CI script.] |
f0c24a2 to
d8b5aec
Compare
|
Here are the times for the "Install OS dependencies" step in the previous 3 commits:
And for the 2 last commits with the additional 1 or 2 packages:
It looks like it would add about ~1m per job? It's unfortunate we aren't running on Fedora, where all texlive packages are available individually (though no guarantee that'd be faster.) |
|
Some ideas:
|
|
I think option 3 (only run on a subset). I would go with the newest Python. |
|
Should be able to set an optional addition in the |
c8e8c11 to
892df4f
Compare
|
I moved the packages texlive-latex-extra, texlive-fonts-extra and texlive-lang-cyrillic to one runner only. (The texlive-latex-extra package has been included for a long time and I'm not sure which exact LaTeX dependencies are used by our tests.) Complete run times:
For comparison, a recent commit on main:
In this case it seems to have added a little under two minutes to that runner, including both the time taken to install and the extra tests run. There seems to be quite a bit of variation from run to run, and most of the overall increase is due to the macos builders where I don't think we even install TeX. |
|
https://matplotlib.org/devdocs/install/dependencies.html#latex-dependencies says it's for |
The DejaVu and Heuristica fonts are used by the type-1 font subsetting tests. Heuristica has a Cyrillic encoding and apparently cannot be loaded without installing texlive-lang-cyrillic.
Agreed! I changed that package back. |
Congrats on closing a 3-digit issue. |
PR Summary
Type-1 subsetting
This reduces pdf file sizes when usetex is active, at the cost of
some complexity in the code. We implement a charstring bytecode
interpreter to keep track of subroutine calls in font programs.
Recommend merging to main to give people time to test this, not to
a 3.10 point release.
Give dviread.DviFont a fake filename attribute and a get_fontmap
method for character tracking.
Add type hints to the code this touches.
Closes #127.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).