Merge pull request 'fix(gif): stop making agents discover the API by failing (closes mort#1522)' (#4) from fix/1522-gifsmith-api-discoverable into main

gif/SKILL.md

@@ -76,9 +76,18 @@ bottom of this skill) as `{"name": "gifsmith.py", "file_id": "<that id>"}`, then
 write ONE `code_exec` call:
 
 ```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
 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 ----
 def draw_steve(ax, x, y, rage=0.0):

gif/scripts/gifsmith.py

@@ -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.
 """
 
+import difflib
 import glob
 import math
 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
 # `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
-    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
 
+# 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.
 GIF_MAX_SECONDS = 20
 # Keep a GIF comfortably under Discord's ~10 MiB inline limit.
@@ -124,7 +149,35 @@ class _Scene:
 class Animation:
     """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.
         self.width = int(width) // 2 * 2
         self.height = int(height) // 2 * 2
@@ -217,10 +270,46 @@ class Animation:
 
     # ------------------------------------------------------------------- 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):
         if not self._scenes:
             raise RuntimeError("no scenes registered — decorate at least one "
                                "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()
         long_edge = long_edge or max(self.width, self.height)
 
@@ -256,6 +345,8 @@ class Animation:
         per_scene = max(1, CONTACT_SHEET_MAX_TILES // len(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
             n = max(1, int(round(sc.seconds * self.fps)))
             try: