Use path instead of base64 for PIL handling

[evnchn]

Motivation

#5583 reveals that using base64 encoding for PIL images is a bad idea when the PIL image is large.

This is due especially to how we send the entire props dict over

Implementation

• Use pil_to_tempfile instead.
• SourceElement expose a new private property self._source_for_cleanup
  • Responsible for cleaning up the temporary file, when:
    • Either a new source arrives
    • Or the element is deleted

Progress

• I chose a meaningful title that completes the sentence: "If applied, this PR will..."
• The implementation is complete.
• If this PR addresses a security issue, it has been coordinated via the security advisory process.
• Pytests have been added (or are not necessary).
• Documentation is not necessary for an underlying performance uplift.

[falkoschindler]

@evnchn I just updated this branch and started with a quick AI review. There seem to be some problems with the current implementation:

1. Critical: weakref.finalize won't work correctly

In source_element.py:26:

weakref.finalize(self, self._cleanup_source)

This is problematic because self._cleanup_source is a bound method that holds a reference to self. This creates a reference cycle that prevents self from being garbage collected, meaning the finalizer will never trigger.

The correct pattern is to pass the data needing cleanup as arguments to a static/module-level function:

def _cleanup_temp_file(path: Path | None) -> None:
    if path is not None:
        path.unlink(missing_ok=True)

# In __init__:
# Need to use a mutable container since _source_for_cleanup changes
self._cleanup_holder: list[Path | None] = [None]
weakref.finalize(self, lambda holder=self._cleanup_holder: _cleanup_temp_file(holder[0]))

Or use atexit with a different cleanup strategy.

2. Temp file leak when source is set via property binding

When the source is changed via element.source = new_value or through bindings, _handle_source_change() is called but _cleanup_source() is not. The cleanup only happens through set_source().

In source_element.py:106-111:

def _handle_source_change(self, source: Any) -> None:
    self._set_props(source)  # No cleanup of old temp file!

Should be:

def _handle_source_change(self, source: Any) -> None:
    self._cleanup_source()  # Clean up old temp file first
    self._set_props(source)

And then set_source() may not need its own cleanup call since setting self.source will trigger _handle_source_change.

3. Missing _handle_delete override in Image/InteractiveImage

When an element is explicitly deleted (not garbage collected), _handle_delete() is called. The base SourceElement._handle_delete() doesn't clean up _source_for_cleanup. While the finalizer is supposed to handle this, given the finalizer issue above, temp files may leak when elements are deleted.

Consider adding cleanup in _handle_delete:

def _handle_delete(self) -> None:
    self._cleanup_source()
    return super()._handle_delete()

4. Minor: force_reload() warning message

In image.py:43-44, the check for data: prefix is still present. While this won't cause issues (PIL images won't have this prefix anymore), the warning message mentioning "base64" might be slightly confusing now.

Test Coverage

The checklist indicates tests haven't been added. Given the cleanup complexity, tests would be valuable:

• Test that temp files are created when using PIL images
• Test that temp files are cleaned up when source changes
• Test that temp files are cleaned up when element is deleted

Recommendation

The core idea is sound and addresses a real performance problem, but the cleanup mechanism has bugs that could cause temp file accumulation. I'd suggest:

1. Fix the weakref.finalize usage to properly clean up on GC
2. Add cleanup to _handle_source_change to prevent leaks via property/binding changes
3. Consider adding cleanup to _handle_delete as a belt-and-suspenders approach
4. Add tests for the cleanup behavior

[evnchn]

Got it. Should be good now.