Repository navigation
Conversation
crisng95
left a comment
There was a problem hiding this comment.
Thanks for this — restoring native 1080p export is genuinely wanted, and half of this PR is exactly the shape we want new Flow surfaces to take. I'd like to land it, but with the polling half pointed somewhere else. Details below, with the reasoning, so this doesn't read as a drive-by.
Keep as-is — this half is the pattern
agent/services/flow_batch.py: RPC_UPSCALE = "p0UkFb", upscale_request(), read_upscaled_media_id(), and calling it through self._batch_payload(...) in FlowClient.
That's the right decomposition: the wire format lives in one module as a pure function, and the transport stays in FlowClient. It's the same shape #41 used for Omni text-to-video, and it's why #41 survived the migration intact. Nothing to change here.
Please redirect — agent/services/upscale_polling.py
upscale_polling.py is a copy of agent/services/omni_flash.py:169-220. Comparing them on main:
| this PR | omni_flash.py on main |
|---|---|
_normalize_workflow() |
_normalize_workflow() — same body, minus the project_id branch |
extract_upscale_workflows() |
extract_omni_workflows() — identical |
annotate_upscale_polling() |
_annotate_polling() — identical apart from the mode string |
_fetch_media_url() |
_fetch_media_url() — byte-identical in the tRPC branch, same URL, same params |
This isn't a nitpick about tidiness. That block is already duplicated once (omni_flash vs the REST methods in flow_client), and that duplication is precisely why the September migration only got applied to one of the two copies — Omni frame/reference generation has been returning UNSUPPORTED_ON_BATCH ever since, because the fix landed next to it rather than on it. A third copy makes the next endpoint change three times the work.
We're extracting a shared agent/services/flow_poll.py (normalize_workflow / extract_workflows / annotate_polling / check_workflow_status) for exactly this. Two options, your call:
- You own it. Add
flow_poll.pyin this PR and have upscale be its first consumer. It's your code being promoted, and it lands with the feature that motivated it. - We land it first, you rebase onto it and import. Slower for you, no writing involved.
Either is fine — say which and we'll sequence around it.
Please redirect — agent/api/upscale_status.py
/check-export-status would be the third endpoint doing one poll pass:
POST /api/flow/check-status— already branches onbody.workflows(agent/api/flow.py:297)POST /api/flow/check-omni-status(agent/api/flow.py:320) — structurally identical to that branch, one indent level apartPOST /api/flow/check-export-status— this PR
We're collapsing the first two behind /check-status with the other kept as a deprecated shim. Could export polling go through /check-status with a mode discriminator instead of a new route? Same handler, one less thing for callers to learn.
Minor
upscale_polling._fetch_media_url calls client._send(...). That's a private of another module, and it's the reason omni_flash is hard to migrate today — it reaches into five FlowClient privates (_send, _build_url, _client_context, _batch_project_id, _batch_payload), so any transport change has to be replayed by hand in both places. Going through a public method keeps upscale out of that trap.
Practical note
GitHub currently reports this branch as CONFLICTING against main (#41, #44 and the #43 revert all landed since you opened it), so a rebase is needed regardless — which makes this the cheapest moment to change direction rather than an expensive one.
Happy to pair on the flow_poll.py extraction if that's useful. And to be explicit: the flow_batch.py work here is good and is staying.
6884a69 to
a7a7dd9
Compare
|
Addressed the requested changes and rebased onto current main (including #42). The export wire work remains in flow_batch/FlowClient, but the duplicate upscale poller and separate /check-export-status router are gone. Added shared agent/services/flow_poll.py, moved media/project transport behind public FlowClient methods, and native export now polls through existing POST /api/flow/check-status with mode=export. Existing Omni polling was migrated to the same shared helpers. Focused flow_batch + flow_client + flow_poll + Omni suite: 153 passed; git diff --check clean. GitHub now reports the branch CLEAN/MERGEABLE. |
a7a7dd9 to
879ab89
Compare
|
Updated #40 on current upstream
The refreshed branch is up to date, without merge conflicts. Focused tests: 156 passed. GitHub's Python 3.10 CI job passed; Python 3.13 is still running at the time of this comment. No paid live generation was triggered by this update. Could you take another look when convenient? |
|
CI note after reviewing the logs: the Python 3.13 job was canceled while apt was downloading ffmpeg dependencies, before pytest started. Python 3.10 passed. #79 had a similar apt download timeout on 3.10. The polling changes requested in the earlier review are already in this branch, with the focused tests reported in my October 27 update. Could you re-review #40 when you have a chance? The failed CI job does not give us a test assertion to fix. |
Summary
Restores Google Flow's native Full HD (1080p) video export through the migrated
flow.google.combatchexecute API.The request uses
p0UkFbwithveo_3_1_upsampler_1080p; the resulting<media-id>_upsampledis resolved through the authenticatedas29smedia lookup.Updated for upstream main (2026-10-27)
Rebased the three PR commits onto upstream
19cb87d. This revision addresses the polling architecture review:flow_batch.py; transport stays inFlowClient.flow_poll.pyfor Omni workflow polling and native export polling.POST /api/flow/export-video; poll through existingPOST /api/flow/check-statuswithmode: "export".labs.googleREST/tRPC support because upstream uses the signed-in batch transport exclusively.project_id, and accept explicit project IDs.Verification
git diff --check: clean./tmp/xfilesystem entry. The YouTube test module could not collect because the local production venv lacks optionalgoogleapiclient.Live verification history
The original implementation was live-tested on 2026-09-08: 720p generation →
p0UkFb→as29s→ downloaded 1080×1920 H.264 24fps / 8.0s MP4. This refactor has been unit tested; a fresh paid live export has not been triggered in the refresh. 4K remains plan-gated and is not claimed as live-verified.This PR is based on current upstream and omits the fork's extra Playwright/UI fallback work.