Skip to content

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

Open
romanlutz wants to merge 1 commit into
microsoft:mainfrom
romanlutz:romanlutz-standardize-converter-font-default
Open

FIX: Standardize image-text converter font default to None#2232
romanlutz wants to merge 1 commit 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
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.

2 participants