Repository navigation
[Feature] add a new response_format is file type when image generate - #1673
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20374714ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| else: | ||
| zip_buffer = io.BytesIO() | ||
| with zipfile.ZipFile(zip_buffer, "w", zipfile.ZIP_DEFLATED) as zf: | ||
| for idx, item in enumerate(response.data): |
There was a problem hiding this comment.
Iterate over self.data when zipping image file responses
When response_format=file and more than one image is generated (n > 1), this branch crashes because it references response.data, but response is undefined in this method. That raises a NameError and returns a 500 instead of the expected zip download, so the new multi-image file response path is currently unusable.
Useful? React with 👍 / 👎.
| status_code=HTTPStatus.INTERNAL_SERVER_ERROR.value, | ||
| detail="No video data available for file response.", | ||
| ) | ||
| video_bytes = base64.b64decode(self.data[0].b64_json) |
There was a problem hiding this comment.
Return all requested videos for file response format
This file-response implementation always serves only self.data[0], so requests that ask for multiple videos (n > 1) silently lose every generated output except the first when response_format=file is used. The endpoint still accepts multi-output requests, so clients can receive incomplete results without any error.
Useful? React with 👍 / 👎.
Gaohan123
left a comment
There was a problem hiding this comment.
The title and PR description have some typos and a bit confusing. Please make it clear. And please supplement some Unit Tests. Besides, does it have potential to support video streaming output with the StreamingResponse?
Is the stream you described generated and output simultaneously? the current approach is to return all the generated JSON data as a stream. |
|
@Gaohan123 PTAL, thanks ~ |
669c0ad to
4ef7c36
Compare
|
Please resolve conflicts. Thanks |
fc70202 to
48e3df4
Compare
|
@Gaohan123 Hi, code conflicts have resolve, please take a look, thanks ~ |
35ed527 to
a5afd34
Compare
Signed-off-by: rongfu.leng <lenronfu@gmail.com>
Signed-off-by: rongfu.leng <lenronfu@gmail.com>
26a13a0 to
d343360
Compare
Signed-off-by: rongfu.leng <lenronfu@gmail.com>
|
These two failed CI runs are unrelated to my changes. |
david6666666
left a comment
There was a problem hiding this comment.
Please fix these response_format=file issues.
| filename = f"image_{uuid.uuid4().hex[:8]}.png" | ||
| return StreamingResponse( | ||
| io.BytesIO(image_bytes), | ||
| media_type="image/png", |
There was a problem hiding this comment.
Use self.output_format for the media type and filename extension here. With output_format="jpeg" or "webp", the body is encoded in that format but the download still advertises image/png and a .png filename; the ZIP member names below have the same issue.
| json={ | ||
| "prompt": "a dog", | ||
| "n": 1, | ||
| "response_format": "file", |
There was a problem hiding this comment.
Please add coverage for output_format="jpeg" or "webp" with response_format="file". The current tests only hit the default PNG path, so the response header / filename mismatch can slip through.
|
|
||
| # save to file | ||
| content_disposition = response.headers.get("Content-Disposition", "") | ||
| match = re.search(r'filename="?(.+)"?', content_disposition) |
There was a problem hiding this comment.
This regex is greedy, so attachment; filename="image.png" captures image.png" with the trailing quote. Use a non-greedy or character-class match here so the example writes the intended filename.
|
In the official OpenAI Images API, the standard value for |
|
@SamitHuang @wtomin WDYT |
…llm-project#1673) Signed-off-by: rongfu.leng <lenronfu@gmail.com> Co-authored-by: Gao Han <hgaoaf@connect.ust.hk>
PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.
Purpose
Current image generate response format is json data, need user to handler this data, this is not very convenient, e.g:
So, i add a new
response_formattype, it isfiletype, when use set this paramresponse_format=filein http request , user can don't handler any response data, e.gDefault save video file name is
dragon.png.Test Plan
Test Result
supported_models.mdandexamplesfor a new model. Please runmkdocs serveto sync the documentation editions to./docs.BEFORE SUBMITTING, PLEASE READ https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md (anything written below this line will be removed by GitHub Actions)