Repository navigation
feat(toolkit): one file_upload log line per upload attempt - #1231
dennybritz wants to merge 7 commits into
Conversation
| source, content_type = _app_prepare_upload( | ||
| data, resolve_content_type(content_type, file_name), request | ||
| ) |
There was a problem hiding this comment.
🟡 (optional) Apps that use the new App.prepare_upload hook to transform an image (the PR's own add_border example) ship an Image whose width/height describe the pre-hook pixels, not what was actually uploaded. Image.from_bytes computes size from the PIL image (image.py:206) before calling File.from_bytes, which now runs the hook on the encoded bytes at file.py:354-356; obj.width/height (image.py:188-189) are set from that stale size, never recomputed from the hook's output. Before this diff nothing mutated bytes post-encode, so the fields were always accurate. Fix: derive width/height from the bytes actually uploaded (post-hook) in Image.from_bytes/from_pil, not from the pre-hook PIL image.
Why this was flagged
Trigger: an app overrides App.prepare_upload to resize/crop/add a border to image bytes (the PR description's own example), reached via Image.from_pil -> Image.from_bytes -> File.from_bytes (file.py:354-356) -> _app_prepare_upload. Image.from_bytes (image.py:164-189) computes size from pil_image.width/height at image.py:206 before the hook runs, then unconditionally sets obj.width/obj.height from that stale size at image.py:188-189 after super().from_bytes returns. Callers reading file.width/file.height (API responses, downstream cropping/layout logic) get dimensions that do not match the uploaded image. No safeguard recomputes dimensions from the hook's output; the PR's own 'Known gaps' section names this but does not change the code.
Verification: image.py:206 computes size from the PIL image before encoding. Image.from_bytes then calls File.from_bytes, which at file.py:354-356 runs the hook on the encoded bytes before upload. Afterward, image.py:188-189 sets obj.width/height from that stale pre-hook size, never re-decoding the hook's output. A hook that resizes, crops, or borders the image ships width/height describing pre-hook pixels.
| nbytes: int | None = None, | ||
| content_type: str | None = None, | ||
| ) -> Any: | ||
| request_id = _current_request_id() |
There was a problem hiding this comment.
🟡 (optional) The new file_upload telemetry line's request_id is read from the ambient current_request ContextVar, not from the request= argument the caller actually passed to from_bytes/from_path. When upload work runs off a ThreadPoolExecutor - which get_upload_policy's own docstring says doesn't see that ContextVar, requiring request= to be passed explicitly - the file_upload log line still reports request_id: null even though the explicit request carries a real id. That defeats the stated point of request_id: correlating an upload attempt/failure with its request. Fix: thread the actually-resolved request (or its id) from _save_bytes/_save_path into _try_with_fallback instead of having it call _current_request_id() independently.
Why this was flagged
_try_with_fallback (file.py:178) computes request_id = _current_request_id(), which (_upload_policy.py:902-903) returns getattr(_current_request(), 'request_id', None), and _current_request() (_upload_policy.py:521-523) reads current_app.current_request, the ContextVar-backed property - never the request argument. Entry: File.from_bytes/from_path(..., request=explicit_request) where explicit_request was passed because the call runs off-context (e.g. a ThreadPoolExecutor), exactly the case get_upload_policy's docstring (_upload_policy.py:530-532) calls out as invisible to that ContextVar. There, current_app.current_request is the default RequestContext(request_id=None,...), so every file_upload JSON line for that call logs request_id: null while get_upload_policy(request) and _app_prepare_upload(...,request) at the same call site correctly use the explicit request's real id. No existing check reconciles the two because _try_with_fallback is never given the resolved request at all, only nbytes/content_type.
Verification: nit. _try_with_fallback (projects/fal/src/fal/toolkit/file/file.py:172) is never passed the caller's request; its callers at file.py:418 and file.py:578 omit it. Inside, request_id = _current_request_id() (file.py:178), which reads current_app.current_request, the ContextVar-backed property, never the explicit request=.
|
Closing after the design review. Provenance stays in the registry, decided per output by the registry media helpers, so the SDK needs no upload hook. The log line is not needed either: the node uploader has upload metrics, REST and the CDN have traces, and File._try_with_fallback already reports failed attempts. No SDK code change is required for this work; the registry only needs a release that contains the node-local uploader. |
Problem
Registry apps get per-upload visibility today from
registry/cdn.py, which keeps seven Prometheus series per app. Apps are moving tofal.toolkit.File/Image/Video, andcdn.pywill be deleted. The toolkit upload path prints nothing when an upload succeeds, and only a free-text line on fallback. Oncecdn.pyis gone there is no record of which repository took an upload, how big it was, how long it took, or which request it belonged to.What this PR does
File._try_with_fallbackwrites one JSON line per repository it tries. It usesreport_json_line, the same helper that writesupload_policy_failure. A GCS fallback shows as two lines:{"file_upload": {"repository": "local", "outcome": "fallback", "bytes": 123456, "duration_ms": 87, "content_type": "image/png", "request_id": "0b1c...", "error": "FileUploadException: ..."}} {"file_upload": {"repository": "fal", "outcome": "ok", "bytes": 123456, "duration_ms": 412, "content_type": "image/png", "request_id": "0b1c..."}}outcomeis one of:ok;fallback: the attempt failed and the next repository is tried;error: the attempt failed on the last repository, or was an uncertain upload that must not be replayed.repositoryislocalwhenFAL_USE_LOCAL_UPLOADERswapped the node-local uploader in behindfal_v3, so local and direct-CDN uploads can be told apart. Otherwise it is the id string (fal_v3,fal,in_memory) or the class name. It is never the object itself, becauseS3RepositoryandGoogleStorageRepositorycarry credentials.bytesandcontent_typecome fromfrom_bytes(len(data)) andfrom_path(stat().st_size).request_idcomes from the app's request context, or isnulloutside a request.erroris present only on failures.{"file_upload": {...}}, not{"event": "file_upload", ...}, to match the existingupload_policy_*lines.This change only adds output. Upload behaviour is the same, the public API is the same, and the existing free-text fallback line is still printed.
Uploads through a caller's upload policy do not go through
_try_with_fallback. They keep their ownupload_policy_*lines.Code changes worth a look
_try_with_fallbacktakes two new optional arguments,nbytesandcontent_type, which are used only for the log line. Both callers infile.pypass them.from_pathnow stats the file once, before the upload, instead of after.test_file.pyassert thatprintis called exactly once on fallback. They now patch outreport_json_lineso they still see only the free-text line. The log line itself has no tests.Not in this PR
Testing
No integration or e2e runs: those need credentials.
Linear: PLAT-6215
Note
Low Risk
Observability-only logging on the existing fallback upload path; no API or upload semantics changes beyond an extra
stat()beforefrom_pathuploads.Overview
Adds structured per-attempt upload telemetry on the toolkit
Filerepository path so registry-style visibility survives removal ofcdn.py._try_with_fallbacknow emits one stdout JSON line per repository try viareport_json_line(same mechanism asupload_policy_*). Each record includesrepository(withlocalwhenFAL_USE_LOCAL_UPLOADERsubstitutes the node uploader),outcome(ok/fallback/error),bytes,duration_ms,content_type,request_id, anderroron failures.File.from_bytesandFile.from_pathpass size and content type into the helper;from_pathalso stats the file once before upload. Upload behavior and the existing free-text fallbackprintare unchanged; caller upload-policy uploads are untouched.Two unit tests that assert fallback
printoutput now patchreport_json_lineso they stay focused on the human-readable line.Reviewed by Cursor Bugbot for commit 1a0ab31. Bugbot is set up for automated code reviews on this repo. Configure here.