Repository navigation
Type-1 font subsetting - #20716
Type-1 font subsetting#20716QuLogic merged 6 commits into
Conversation
Sorry, something went wrong.
|
Is this ready for review, now that #20715 has been merged? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I can repeat the error running on an Ubuntu 22.04 docker image: GPL Ghostscript 9.55.0 (2021-09-27)
Copyright (C) 2021 Artifex Software, Inc. All rights reserved.
This software is supplied under the GNU AGPLv3 and comes with NO WARRANTY:
see the file COPYING for details.
Processing pages 1 through 1.
Page 1
**** Error: can't process embedded font stream,
attempting to load the font using its name.
Output may be incorrect.
Querying operating system for font files...
Substituting font Helvetica for MPLAAD+CMR17.
Loading NimbusSans-Regular font from /usr/share/ghostscript/9.55.0/Resource/Font/NimbusSans-Regular... 4467044 2957798 6795968 5451925 4 done.
**** Error: can't process embedded font stream,
attempting to load the font using its name.
Output may be incorrect.
Substituting font Helvetica for MPLAAC+CMR12.
**** Error: can't process embedded font stream,
attempting to load the font using its name.
Output may be incorrect.
Substituting font Helvetica for MPLAAA+CMEX10.
**** Error: can't process embedded font stream,
attempting to load the font using its name.
Output may be incorrect.
Substituting font Helvetica-Oblique for MPLAAB+CMMI12.
Loading NimbusSans-Italic font from /usr/share/ghostscript/9.55.0/Resource/Font/NimbusSans-Italic... 4534308 3175939 7138400 5761600 4 done.
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.) |
Sorry, something went wrong.
|
I hope I found the culprit... I was writing an extra delimiter between the Subrs and the Charstrings when one was already there. |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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?
Sorry, something went wrong.
Sorry, something went wrong.
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
Sorry, something went wrong.
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?
Sorry, something went wrong.
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 🙄
Sorry, something went wrong.
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 heuristica and DejaVuSans texlive packages in .github/workflows/tests.yml to get those new tests running. |
Sorry, something went wrong.
Yes, but Debian (and hence Ubuntu) bundles them into texlive-fonts-extra with a large number of other fonts. Not sure that slowing down all test runners is worth it, but perhaps we could do it in just one runner? [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.] |
Sorry, something went wrong.
|
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.) |
Sorry, something went wrong.
|
Some ideas:
|
Sorry, something went wrong.
|
I think option 3 (only run on a subset). I would go with the newest Python. |
Sorry, something went wrong.
|
Should be able to set an optional addition in the matrix just like we do with matrix.extra-requirements. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
https://matplotlib.org/devdocs/install/dependencies.html#latex-dependencies says it's for import, sfmath, and type1cm, and type1cm is used unconditionally in the preamble. According to the log, texlive-latex-extra is already being installed by dependency of something else which is why it didn't break, so I don't think it makes sense to move it to the extras job only. It should stay for explicitness in all jobs. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Congrats on closing a 3-digit issue. |
Sorry, something went wrong.
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