Skip to content

[bugfix] Fix unexpected argument 'is_finished' in function llm2code2wav_async_chunk of mimo-audio#1570

Merged
Gaohan123 merged 7 commits intovllm-project:mainfrom
qibaoyuan:fix-async
Mar 2, 2026
Merged

[bugfix] Fix unexpected argument 'is_finished' in function llm2code2wav_async_chunk of mimo-audio#1570
Gaohan123 merged 7 commits intovllm-project:mainfrom
qibaoyuan:fix-async

Conversation

@qibaoyuan
Copy link
Contributor

@qibaoyuan qibaoyuan commented Feb 28, 2026

bug: #1569

PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.

Purpose

Fix unexpected argument 'is_finished' in function llm2code2wav_async_chunk of mimo-audio:
(EngineCore_DP0 pid=51408) [Stage-0] ERROR 02-28 16:59:52 [chunk_transfer_adapter.py:213] Failed to use custom_process_input_func for payload extraction: llm2code2wav_async_chunk() got an unexpected keyword argument 'is_finished'

Test Plan

serving:

vllm-omni serve XiaomiMiMo/MiMo-Audio-7B-Instruct --omni \
--chat-template ./examples/online_serving/mimo_audio/chat_template.jinja  \
--stage-configs-path model_executor/stage_configs/mimo_audio_async_chunk.yaml

client:

python examples/online_serving/mimo_audio/openai_chat_completion_client_for_multimodal_generation.py --stream

Test Result

[  32.16s][req 0_chatcmpl-bc7df29b6f1f58d1] 需要
[  32.42s][req 0_chatcmpl-bc7df29b6f1f58d1] 更
[  32.44s][req 0_chatcmpl-bc7df29b6f1f58d1] 精确
[  32.47s][req 0_chatcmpl-bc7df29b6f1f58d1] 的数据

Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan. Please provide the test scripts & test commands. Please state the reasons if your codes don't require additional test scripts. For test file guidelines, please check the test style doc
  • The test results. Please paste the results comparison before and after, or the e2e results.
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model. Please run mkdocs serve to sync the documentation editions to ./docs.
  • (Optional) Release notes update. If your change is user-facing, please update the release notes draft.

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)

sync with upstream

Signed-off-by: Baoyuan Qi <qibaoyuan@126.com>
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c46d4d80ae

ℹ️ 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".

Fix type hint for pooling_output parameter

Signed-off-by: Baoyuan Qi <qibaoyuan@126.com>
Accumulates codes in connector per request_id,
returns payload only when chunk_size is full or request is finished; returns None when waiting.
"""
finished = bool(is_finished or request.is_finished())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we use save_async. In high concurrency scenarios, if request.is_finished is used, the output audio may be truncated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it means use is_finished directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only use is_finished, test passed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it means use is_finished directly?

it means use is_finished directly?

yes

Signed-off-by: 齐保元 <qibaoyuan@xiaomi.com>
@hsliuustc0106
Copy link
Collaborator

📝 PR Review Feedback

Thank you for fixing this issue! Here are some findings that need attention:

🔴 Critical: Indentation Error

Location: vllm_omni/model_executor/stage_input_processors/mimo_audio.py Lines 87-89

if not code_predictor_codes.any():
    if is_finished:
    return _make_finished_sentinel()   # ❌ Wrong indentation!

The return _make_finished_sentinel() statement is not properly indented under the if is_finished: block.

Suggested fix:

if not code_predictor_codes.any():
    if is_finished:
        return _make_finished_sentinel()  # ✅ Correct indentation
    return None

🟡 Medium Priority Issues

Issue Description
Callers need update The request parameter is still in the signature but no longer used. Callers should pass is_finished=request.is_finished()
Default value risk With is_finished=False as default, callers that don't update will cause streams to never terminate

🟢 Suggestions

  • Fix indentation error (required)
  • Verify all call sites pass the correct parameter
  • Update docstring to document the new is_finished parameter

Conclusion: The fix approach is correct ✅, but there's a critical indentation bug that must be fixed before merging.


Review by Claude Code via OpenClaw

@qibaoyuan
Copy link
Contributor Author

📝 PR Review Feedback

Thank you for fixing this issue! Here are some findings that need attention:

🔴 Critical: Indentation Error

Location: vllm_omni/model_executor/stage_input_processors/mimo_audio.py Lines 87-89

if not code_predictor_codes.any():
    if is_finished:
    return _make_finished_sentinel()   # ❌ Wrong indentation!

The return _make_finished_sentinel() statement is not properly indented under the if is_finished: block.

Suggested fix:

if not code_predictor_codes.any():
    if is_finished:
        return _make_finished_sentinel()  # ✅ Correct indentation
    return None

🟡 Medium Priority Issues

Issue Description
Callers need update The request parameter is still in the signature but no longer used. Callers should pass is_finished=request.is_finished()
Default value risk With is_finished=False as default, callers that don't update will cause streams to never terminate

🟢 Suggestions

  • Fix indentation error (required)
  • Verify all call sites pass the correct parameter
  • Update docstring to document the new is_finished parameter

Conclusion: The fix approach is correct ✅, but there's a critical indentation bug that must be fixed before merging.

Review by Claude Code via OpenClaw

Emmm, it seems that OpenClaw is doing something wrong.

  • There is no indentation error — this must be a mistake.
  • There’s no need to include this in the Medium Priority Issues section.

@amy-why-3459
Copy link
Contributor

LGTM

@Gaohan123 Gaohan123 added the ready label to trigger buildkite CI label Mar 2, 2026
Copy link
Collaborator

@Gaohan123 Gaohan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@Gaohan123 Gaohan123 merged commit f371f7b into vllm-project:main Mar 2, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready label to trigger buildkite CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants