fix(gif): stop making agents discover the API by failing

Closes mort#1522.

23% of all code_exec calls across two production runs (9 of 39) failed on
gifsmith API misuse, hitting 7 of 14 workers. The recovery was always the same
shape: another round trip spent printing gifsmith's own source — or, in one
case, running `dir(gifsmith)` — to learn what the module contains.

Three distinct causes, three fixes.

**Unguessable re-exports → `__all__`, plus the two that were missing.** The
module exported SOME matplotlib artists and not others, and the only way to find
out which was to fail: two workers wrote `from gifsmith import PathPatch` and
got an ImportError while `FancyBboxPatch` beside it worked. `PathPatch` and
`Path` are now exported, on the rule "the drawing vocabulary, whole". `__all__`
states the surface — and it also fixes what made self-service expensive:
`dir(gifsmith)` returned 24 names of which nine were incidental imports (glob,
os, shutil, subprocess, sys, traceback...), several being things a scene author
should never touch. SKILL.md now carries the same list, so the question should
not arise.

**Near-miss constructor kwargs → an error that names the miss.** `background=`
is accepted as an alias for `bg` (it is the obvious synonym, and an agent
reached for it). `force=` is rejected with the reason it was tempting —
`render(force="mp4")` really does take it, which makes it read like an
Animation-level setting. Anything else gets a difflib near-match and the valid
list. Both failures were one round trip each, and both are the kind a message
fixes for free.

**Scenes fail at render time → dry-run every scene at t=0 first.** A NameError
in scene 4 used to surface only after scenes 0-3 were fully rendered, and with
a streaming pipe open, after frames had gone into the encoder. Two workers lost
a whole render each to `RuntimeError: every scene failed`, learning one broken
name per attempt. One frame per scene now reports EVERY broken scene in a single
pass, before any scratch dir or ffmpeg pipe exists. Per-scene isolation is
preserved exactly: a scene that fails the dry run is marked and skipped rather
than re-run to fail identically, the others still ship, and only an
all-scenes-broken program raises early — which is precisely the case that was
paying for a full setup to learn nothing.

Verified against the production failures: `Animation(background=…)` now works,
`Animation(force=…)` explains itself, `Animation(backgrnd=…)` suggests
`background`, every name in `__all__` resolves, no incidental import leaks into
it, and the dry run marks exactly the broken scenes while leaving the good ones
untouched.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
2026-07-26 12:12:00 -04:00
co-authored by Claude Opus 5
parent 7dee907d44
commit 9eb3ec49dd
2 changed files with 103 additions and 3 deletions
+10 -1
View File
@@ -76,9 +76,18 @@ bottom of this skill) as `{"name": "gifsmith.py", "file_id": "<that id>"}`, then
write ONE `code_exec` call: write ONE `code_exec` call:
```python ```python
# gifsmith re-exports the drawing vocabulary, so this one import is enough: # gifsmith re-exports the drawing vocabulary, so this one import is enough.
# The COMPLETE importable list (there is nothing else — don't guess, and don't
# spend a call on dir(gifsmith)):
# Animation
# Arc Circle Ellipse FancyBboxPatch PathPatch Path Polygon Rectangle Wedge
# Image ImageDraw ImageFont (PIL, for mode="pil")
# np plt (numpy / pyplot, for anything else)
# GIF_MAX_SECONDS GIF_MAX_BYTES
from gifsmith import Animation, Circle, Ellipse, Rectangle, Polygon, Wedge, np from gifsmith import Animation, Circle, Ellipse, Rectangle, Polygon, Wedge, np
anim = Animation(width=640, height=480, fps=15) # canvas locked here anim = Animation(width=640, height=480, fps=15) # canvas locked here
# Animation() takes ONLY width, height, fps, dpi, bg (background= also works).
# Everything about the output — format, size, audio — is set on render().
# ---- cast: defined ONCE, called from every scene ---- # ---- cast: defined ONCE, called from every scene ----
def draw_steve(ax, x, y, rage=0.0): def draw_steve(ax, x, y, rage=0.0):
+93 -2
View File
@@ -37,6 +37,7 @@ fix just that scene. It also writes /workspace/still.png: a labelled contact
sheet, one tile per scene, for the critique pass. sheet, one tile per scene, for the critique pass.
""" """
import difflib
import glob import glob
import math import math
import os import os
@@ -52,10 +53,34 @@ from PIL import Image, ImageDraw, ImageFont # noqa: E402
# Re-exported so one import line covers the whole drawing vocabulary — a missing # Re-exported so one import line covers the whole drawing vocabulary — a missing
# `from matplotlib.patches import ...` is otherwise a NameError that costs a pass. # `from matplotlib.patches import ...` is otherwise a NameError that costs a pass.
#
# PathPatch and Path are here because agents assumed they already were: two
# separate workers wrote `from gifsmith import PathPatch` and got an ImportError,
# because the module exported SOME matplotlib artists and not others with no way
# to tell which from outside. The rule now is "the drawing vocabulary, whole".
from matplotlib.patches import ( # noqa: E402,F401 from matplotlib.patches import ( # noqa: E402,F401
Arc, Circle, Ellipse, FancyBboxPatch, Polygon, Rectangle, Wedge) Arc, Circle, Ellipse, FancyBboxPatch, PathPatch, Polygon, Rectangle, Wedge)
from matplotlib.path import Path # noqa: E402,F401
import numpy as np # noqa: E402,F401 import numpy as np # noqa: E402,F401
# The public surface, stated rather than discovered. Without this, `dir(gifsmith)`
# returned 24 names of which nine were incidental imports (glob, os, shutil,
# subprocess, sys, traceback...) — several of them things a scene author should
# never touch — and a worker spent a whole code_exec call running exactly that
# to find out what it could import. Keep in sync when adding a re-export.
__all__ = [
"Animation",
# matplotlib artists (mode="mpl")
"Arc", "Circle", "Ellipse", "FancyBboxPatch", "PathPatch", "Path",
"Polygon", "Rectangle", "Wedge",
# PIL (mode="pil")
"Image", "ImageDraw", "ImageFont",
# the two libraries themselves, for anything not re-exported above
"np", "plt",
# caps a scene author may want to read
"GIF_MAX_SECONDS", "GIF_MAX_BYTES",
]
# A GIF longer than this balloons past what's worth shipping as a GIF. # A GIF longer than this balloons past what's worth shipping as a GIF.
GIF_MAX_SECONDS = 20 GIF_MAX_SECONDS = 20
# Keep a GIF comfortably under Discord's ~10 MiB inline limit. # Keep a GIF comfortably under Discord's ~10 MiB inline limit.
@@ -116,7 +141,35 @@ class _Scene:
class Animation: class Animation:
"""Collects scenes, renders them to a locked-size frame sequence, encodes.""" """Collects scenes, renders them to a locked-size frame sequence, encodes."""
def __init__(self, width=640, height=480, fps=15, dpi=100, bg="white"): def __init__(self, width=640, height=480, fps=15, dpi=100, bg="white", **kwargs):
# `background` is accepted as an alias for `bg` because it is the obvious
# synonym and agents reached for it; `force` is rejected loudly because
# `render(force="mp4")` DOES take it, which makes it read like an
# Animation-level setting. Both were real failures, one code_exec round
# trip each, and both are the kind a better message fixes for free.
if "background" in kwargs:
bg = kwargs.pop("background")
if kwargs:
valid = ["width", "height", "fps", "dpi", "bg (or background)"]
bad = sorted(kwargs)
hints = []
for k in bad:
near = difflib.get_close_matches(
k, ["width", "height", "fps", "dpi", "bg", "background"], n=1, cutoff=0.6)
if near:
hints.append(f"{k!r} — did you mean {near[0]!r}?")
elif k == "force":
hints.append("'force' belongs to render(force='mp4'/'gif'), "
"not to Animation()")
else:
hints.append(repr(k))
raise TypeError(
"Animation() got unexpected keyword argument(s): "
+ "; ".join(hints)
+ ". Valid: " + ", ".join(valid)
+ ". Everything about the OUTPUT (format, size, audio) is set on "
"render(), not here.")
# H.264 needs even dimensions; lock them here so the encoder never fails. # H.264 needs even dimensions; lock them here so the encoder never fails.
self.width = int(width) // 2 * 2 self.width = int(width) // 2 * 2
self.height = int(height) // 2 * 2 self.height = int(height) // 2 * 2
@@ -209,10 +262,46 @@ class Animation:
# ------------------------------------------------------------------- render # ------------------------------------------------------------------- render
def _dry_run_scenes(self):
"""Render every scene ONCE at t=0 before the real pass.
Scene functions fail at render time, not definition time, so a NameError
in scene 4 used to surface only after scenes 0-3 had been fully rendered
— and, when the pipe was open, after frames had already gone into the
encoder. Two production workers lost a whole render each to
`RuntimeError: every scene failed`, learning about one broken name per
attempt.
One frame per scene costs almost nothing and reports EVERY broken scene
in a single pass. A scene that fails here is marked and skipped by the
real loop rather than re-run to fail identically; if they ALL fail there
is nothing to encode, so say so now instead of after the setup.
"""
for i, sc in enumerate(self._scenes):
try:
(self._render_pil if sc.mode == "pil" else self._render_mpl)(sc, 0.0)
except Exception: # noqa: BLE001
sc.error = traceback.format_exc(limit=6)
print(f"[gifsmith] SCENE {i} ({sc.title!r}) FAILED its t=0 dry "
f"run — skipped, other scenes continue:\n{sc.error}",
file=sys.stderr, flush=True)
broken = [i for i, sc in enumerate(self._scenes) if sc.error]
if broken and len(broken) == len(self._scenes):
raise RuntimeError(
"every scene failed before rendering started — see the "
f"{len(broken)} traceback(s) above. They usually share ONE "
"cause: a helper called with a keyword it does not take, or a "
"name that differs by a character. Fix it and re-run the whole "
"program in a fresh code_exec call, remembering gifsmith.py in "
"files_in. Nothing was encoded and no GPU time was spent.")
return broken
def render(self, long_edge=None, force=None, audio=None): def render(self, long_edge=None, force=None, audio=None):
if not self._scenes: if not self._scenes:
raise RuntimeError("no scenes registered — decorate at least one " raise RuntimeError("no scenes registered — decorate at least one "
"function with @anim.scene(...)") "function with @anim.scene(...)")
# Before ANY setup: no scratch dir, no ffmpeg pipe, no frames.
self._dry_run_scenes()
self._scratch, free_mb = _pick_scratch() self._scratch, free_mb = _pick_scratch()
long_edge = long_edge or max(self.width, self.height) long_edge = long_edge or max(self.width, self.height)
@@ -242,6 +331,8 @@ class Animation:
f"{self.fps}fps, canvas {self.width}x{self.height}", flush=True) f"{self.fps}fps, canvas {self.width}x{self.height}", flush=True)
for i, sc in enumerate(self._scenes): for i, sc in enumerate(self._scenes):
if sc.error:
continue # already reported by the dry run; don't fail it twice
start = self._count start = self._count
n = max(1, int(round(sc.seconds * self.fps))) n = max(1, int(round(sc.seconds * self.fps)))
try: try: