Skip to content

FIX: Standardize image-text converter font default to None#2232

Merged
romanlutz merged 5 commits into
microsoft:mainfrom
romanlutz:romanlutz-standardize-converter-font-default
Jul 21, 2026
Merged

FIX: Standardize image-text converter font default to None#2232
romanlutz merged 5 commits into
microsoft:mainfrom
romanlutz:romanlutz-standardize-converter-font-default

Conversation

@romanlutz

Copy link
Copy Markdown
Contributor

Description

PyRIT's two image-text converters had inconsistent font-name defaults. AddImageTextConverter already defaulted font_name to None (using Pillow's bundled default), but AddTextImageConverter defaulted to "helvetica.ttf", which does not exist on Linux. On Linux that caused ImageFont.truetype("helvetica.ttf", ...) to raise OSError and log a spurious Cannot open font resource: helvetica.ttf. Using Pillow built-in default font. warning before falling back. This warning surfaced in the 3_image_converters notebook during release validation, and the mismatched defaults made the two converters behave differently across platforms.

This change makes AddTextImageConverter mirror AddImageTextConverter:

  • Signature default changed from font_name: str = "helvetica.ttf" to font_name: str | None = None.
  • .ttf extension validation is now gated on font_name is not None.
  • Added the _font_load_failed = font_name is None pattern so _load_font goes straight to ImageFont.load_default(size=...) when None (no truetype attempt, no warning), while keeping the existing OSError -> load_default fallback for custom fonts.
  • Docstring updated to match AddImageTextConverter.

On the pinned Pillow (>=12.2.0), ImageFont.load_default(size=...) returns a real scalable TrueType font, so rendering was already fine; this cleans up the warning and makes both converters cross-platform consistent. No matplotlib/DejaVu font lookup was introduced. AddImageTextConverter was left untouched (already correct).

Tests and Documentation

  • Updated test_add_text_image_converter_initialization to construct with the default None, assert _font_name is None and a real FreeTypeFont, and assert (via caplog) that no Cannot open font resource warning is emitted. Kept the invalid-.ttf ValueError test and the nonexistent-custom-font fallback-warning test. Both converter test files pass (31 tests), including test_add_image_text_converter_equal_to_add_text_image, which now confirms both converters produce byte-identical output for the None default on all platforms (previously only matched on Linux via fallback).
  • Re-ran the doc/code/converters/3_image_converters notebook via JupyText (jupytext --to ipynb --execute). It executes end to end with 0 error outputs and 0 occurrences of Cannot open font resource/helvetica. The notebook source uses the plain converter constructors with no font workaround; the converter fix is what makes the defaults run cleanly.

AddTextImageConverter defaulted font_name to "helvetica.ttf", which does
not exist on Linux and triggered a spurious "Cannot open font resource"
warning (surfaced in the 3_image_converters notebook) before falling back
to Pillow's built-in font. AddImageTextConverter already defaults to None.

Default AddTextImageConverter.font_name to None and use Pillow's bundled
default directly when None (mirroring AddImageTextConverter via the
_font_load_failed pattern), so both converters render identically across
platforms. The .ttf validation is now gated on a non-None font_name.
Update tests to exercise the None default (and assert no font warning),
and re-execute the 3_image_converters notebook so the plain constructors
run cleanly with no "Cannot open font resource" warning.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 43b99657-b60e-47dc-a0fb-d8babd5c0860
@hannahwestra25 hannahwestra25 self-assigned this Jul 20, 2026
Comment thread pyrit/converter/add_text_image_converter.py Outdated
Copilot AI and others added 4 commits July 20, 2026 14:57
Address review feedback: the _font_load_failed boolean was redundant in
AddTextImageConverter since _load_font is only called once at construction.
Check self._font_name directly instead, which also lets the type checker
narrow font_name to str and removes the need for the type: ignore comment.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 43b99657-b60e-47dc-a0fb-d8babd5c0860
@romanlutz
romanlutz added this pull request to the merge queue Jul 21, 2026
Merged via the queue into microsoft:main with commit 5531da0 Jul 21, 2026
53 checks passed
@romanlutz
romanlutz deleted the romanlutz-standardize-converter-font-default branch July 21, 2026 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants