Skip to content

[Cleanup] Remove dead/stale modules with no repo references (fixes #4009)#4044

Open
yangyonggit wants to merge 2 commits into
vllm-project:mainfrom
yangyonggit:cleanup/remove-dead-code-modules
Open

[Cleanup] Remove dead/stale modules with no repo references (fixes #4009)#4044
yangyonggit wants to merge 2 commits into
vllm-project:mainfrom
yangyonggit:cleanup/remove-dead-code-modules

Conversation

@yangyonggit

Copy link
Copy Markdown
Contributor

Remove six files confirmed unreferenced across all Python imports, init.py exports, dynamic import strings, and YAML configs:

  • vllm_omni/assets/video.py
  • vllm_omni/benchmarks/data_modules/daily_omni_text_audio.py
  • vllm_omni/distributed/kv_transfer/monkey_patch.py
  • vllm_omni/model_executor/models/qwen3_omni/qwen3_moe.py
  • vllm_omni/model_executor/models/moss_tts_nano/configuration_moss_tts_nano.py
  • vllm_omni/model_executor/stage_input_processors/omnivoice.py

Part of codebase cleanup audit #4009.

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

Purpose

Remove six dead/stale modules confirmed unreferenced across all Python imports, init.py exports, dynamic import strings, and YAML configs. Part of codebase cleanup audit #4009.

Test Plan

Static reference check (no GPU required):

uvx ruff check vllm_omni/ --select F401,F821,F811
Import smoke test:

python -c "
import vllm_omni
from vllm_omni.model_executor.models.moss_tts_nano import MossTTSNanoForGeneration
from vllm_omni.model_executor.models.qwen3_omni import Qwen3OmniMoeForConditionalGeneration
print('imports OK')
"

Test Result

uvx ruff check vllm_omni/ --select F401,F821,F811
All checks passed!

imports OK

Remove six files confirmed unreferenced across all Python imports,
__init__.py exports, dynamic import strings, and YAML configs:

- vllm_omni/assets/video.py
- vllm_omni/benchmarks/data_modules/daily_omni_text_audio.py
- vllm_omni/distributed/kv_transfer/monkey_patch.py
- vllm_omni/model_executor/models/qwen3_omni/qwen3_moe.py
- vllm_omni/model_executor/models/moss_tts_nano/configuration_moss_tts_nano.py
- vllm_omni/model_executor/stage_input_processors/omnivoice.py

Part of codebase cleanup audit vllm-project#4009.

Signed-off-by: leo.yang <leo.yang.engineer@gmail.com>

@linyueqian linyueqian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified all six deletions against main (static grep + dynamic-import / YAML / trust_remote_code paths, since the PR's ruff F401 check only covers Python imports). Findings below.

[LGTM] assets/video.py, benchmarks/data_modules/daily_omni_text_audio.py, model_executor/models/qwen3_omni/qwen3_moe.py — no callers in vllm_omni/, tests/, examples/, or any YAML. The vllm.assets.video imports in examples/ all point at upstream vLLM, not the deleted vllm_omni.assets.video. The qwen3_omni stack imports Qwen3MoeForCausalLM directly from upstream vLLM (qwen3_omni_moe_thinker.py:84), not the local subclass.

[LGTM, authoritative] model_executor/stage_input_processors/omnivoice.py — I wrote this file as part of the original 2-stage OmniVoice serving (#2463). It was orphaned when omnivoice migrated to the current single-stage diffusion pipeline (see vllm_omni/deploy/omnivoice.yaml, which has stage_type: diffusion and no stage_input_processor field). tokens2audio has no callers. Safe to delete.

[LGTM, authoritative] model_executor/models/moss_tts_nano/configuration_moss_tts_nano.py — I added this in #2753. The class is never imported, no AutoConfig.register("moss_tts_nano", ...) exists, and modeling_moss_tts_nano.py / pipeline.py / __init__.py do not reference it. All "moss_tts_nano" mentions in serving_speech.py are string comparisons against _tts_model_type, not class references. The model loads its config via trust_remote_code from OpenMOSS-Team/MOSS-TTS-Nano, which ships its own config class. The in-tree copy is a stub from an earlier integration attempt and is safe to remove.

[SUGGESTION, non-blocking] distributed/kv_transfer/monkey_patch.pyapply_mooncake_patch is never imported, including by its own package __init__.py. However, the surviving vllm_omni/distributed/kv_transfer/__init__.py docstring still claims:

This package provides monkey-patched versions of vLLM's native KV transfer connectors (e.g. MooncakeConnector) that fix the request-ID mismatch problem in prefill-decode disaggregation.

After this PR, that becomes a false promise. Could you also delete kv_transfer/__init__.py (or strip the docstring down to a one-line "removed, see omni_connectors/" note)? Otherwise readers will go looking for code that no longer exists. Worth one sanity check that the remote_request_id fix from #1863 has actually been superseded inside vllm_omni/distributed/omni_connectors/ — I did not find any equivalent thread-through there, but I am not the PD-disagg owner.

[NIT] The test plan (ruff F401,F821,F811 + import smoke) only catches Python references. It does not catch dynamic-import strings, YAML model class names, or trust_remote_code paths, which is where these particular files could have been load-bearing. I checked those manually above and they're clean, but a one-line note in the PR description acknowledging this scope would help future cleanup PRs reuse the same template.

Overall: ship after the kv_transfer/__init__.py cleanup. Thanks for the audit.

Signed-off-by: leo.yang <leo.yang.engineer@gmail.com>
@yangyonggit

Copy link
Copy Markdown
Contributor Author

Verified all six deletions against main (static grep + dynamic-import / YAML / trust_remote_code paths, since the PR's ruff F401 check only covers Python imports). Findings below.

[LGTM] assets/video.py, benchmarks/data_modules/daily_omni_text_audio.py, model_executor/models/qwen3_omni/qwen3_moe.py — no callers in vllm_omni/, tests/, examples/, or any YAML. The vllm.assets.video imports in examples/ all point at upstream vLLM, not the deleted vllm_omni.assets.video. The qwen3_omni stack imports Qwen3MoeForCausalLM directly from upstream vLLM (qwen3_omni_moe_thinker.py:84), not the local subclass.

[LGTM, authoritative] model_executor/stage_input_processors/omnivoice.py — I wrote this file as part of the original 2-stage OmniVoice serving (#2463). It was orphaned when omnivoice migrated to the current single-stage diffusion pipeline (see vllm_omni/deploy/omnivoice.yaml, which has stage_type: diffusion and no stage_input_processor field). tokens2audio has no callers. Safe to delete.

[LGTM, authoritative] model_executor/models/moss_tts_nano/configuration_moss_tts_nano.py — I added this in #2753. The class is never imported, no AutoConfig.register("moss_tts_nano", ...) exists, and modeling_moss_tts_nano.py / pipeline.py / __init__.py do not reference it. All "moss_tts_nano" mentions in serving_speech.py are string comparisons against _tts_model_type, not class references. The model loads its config via trust_remote_code from OpenMOSS-Team/MOSS-TTS-Nano, which ships its own config class. The in-tree copy is a stub from an earlier integration attempt and is safe to remove.

[SUGGESTION, non-blocking] distributed/kv_transfer/monkey_patch.pyapply_mooncake_patch is never imported, including by its own package __init__.py. However, the surviving vllm_omni/distributed/kv_transfer/__init__.py docstring still claims:

This package provides monkey-patched versions of vLLM's native KV transfer connectors (e.g. MooncakeConnector) that fix the request-ID mismatch problem in prefill-decode disaggregation.

After this PR, that becomes a false promise. Could you also delete kv_transfer/__init__.py (or strip the docstring down to a one-line "removed, see omni_connectors/" note)? Otherwise readers will go looking for code that no longer exists. Worth one sanity check that the remote_request_id fix from #1863 has actually been superseded inside vllm_omni/distributed/omni_connectors/ — I did not find any equivalent thread-through there, but I am not the PD-disagg owner.

[NIT] The test plan (ruff F401,F821,F811 + import smoke) only catches Python references. It does not catch dynamic-import strings, YAML model class names, or trust_remote_code paths, which is where these particular files could have been load-bearing. I checked those manually above and they're clean, but a one-line note in the PR description acknowledging this scope would help future cleanup PRs reuse the same template.

Overall: ship after the kv_transfer/__init__.py cleanup. Thanks for the audit.

Thanks for the thorough review! Fixed in the latest commit — removed the entire kv_transfer/ package since nothing imports from it.

On the NIT: dynamic import strings and YAML model class names were also verified with grep. Updated the PR description to note that ruff only covers Python imports and the remaining paths were verified manually.

@hsliuustc0106

Copy link
Copy Markdown
Collaborator

@ZeldaHuang @amy-why-3459 PTAL the deletion for omni related files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants