Refactor(handler part 12): vae decode#591
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR refactors tiled VAE decoding functionality by extracting it from AceStepHandler into two new mixin classes (VaeDecodeMixin and VaeDecodeChunksMixin) with orchestrated fallback paths spanning MLX, PyTorch, and CPU execution. Concurrently, it adds distributed state cleanup for LLMHandler, enhances Gradio audio validation with i18n support, and includes comprehensive test coverage for the new components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@acestep/core/generation/handler/vae_decode_test.py`:
- Around line 1-244: Split this large test module into two smaller modules to
stay under 200 LOC: move the VaeDecodeMixinTests class (and the minimal stubs it
uses: _DecodeOutput, _FakeVae, _DecodeHost) into a new test file (e.g.,
vae_decode_mixin_test.py) and move VaeDecodeChunksMixinTests (and its stubs:
_ChunksHost) into a separate file (e.g., vae_decode_chunks_test.py); if any
helpers are shared between both (e.g., _DecodeOutput or _FakeVae), extract them
into a small shared test helper module to avoid duplication and import that from
both new test files; ensure each new module imports VaeDecodeMixin and
VaeDecodeChunksMixin respectively and that test discovery still finds the tests.
In `@acestep/gradio_ui/events/generation_handlers.py`:
- Around line 932-943: Update the docstring for _extract_audio_path to clearly
state its purpose, accepted input types (None, str, list/tuple containing a
string), the normalization it performs (stripping whitespace, returning None for
empty strings or None input), and the return value (Optional[str] — file path or
None); mention that it does not raise exceptions and note any special handling
of list/tuple by returning the first element if it is a string.
- Around line 964-968: The hardcoded user-facing strings in the audio-format
warning must be localized: replace the inline role_label logic and the
gr.Warning message to use the i18n translator (t) from the module used in this
package (e.g., import t from i18n). Build the role label via t("reference") /
t("source") (or appropriate keys) based on audio_role and pass a translated
message key to gr.Warning (e.g., t("audio.format_invalid", {"role":
role_label})) so both the label and the full toast text come from i18n keys;
update the call site using role_label, audio_role, and gr.Warning accordingly.
In `@acestep/llm_inference_dist_cleanup_test.py`:
- Around line 6-11: The test currently silences all exceptions during import by
catching Exception, which can hide real errors in acestep.llm_inference; change
the except clause in the import block to except ImportError (so only
import/module-not-found issues are caught) and keep assigning LLMHandler = None
and _IMPORT_ERROR = exc as before (preserve the pragma: no cover comment if
needed) — locate the try/except that imports LLMHandler and update the exception
type from Exception to ImportError.
🧹 Nitpick comments (3)
acestep/llm_inference.py (1)
117-125: Module exceeds 200 LOC — consider splitting.
This file is far beyond the 200 LOC cap; consider extracting backend-specific helpers into separate modules for maintainability.As per coding guidelines, "Target module size: optimal <= 150 LOC, hard cap 200 LOC."
acestep/core/generation/handler/vae_decode_chunks.py (1)
13-15: Add type hints to the chunked decode helper signatures.The new methods are untyped; annotate inputs/outputs for clarity and consistency.
Apply the same pattern to `_tiled_decode_gpu` and `_tiled_decode_offload_cpu`. As per coding guidelines: Add type hints for new/modified functions when practical in Python.Suggested signature pattern
- def _tiled_decode_inner(self, latents, chunk_size, overlap, offload_wav_to_cpu): + def _tiled_decode_inner( + self, + latents: torch.Tensor, + chunk_size: int, + overlap: int, + offload_wav_to_cpu: bool, + ) -> torch.Tensor:Also applies to: 83-85, 114-116
acestep/core/generation/handler/vae_decode.py (1)
16-22: Add full type hints for VAE decode mixin methods.
latentsand return types are unannotated; addtorch.Tensor(and return) across the mixin methods.Apply the same pattern to `_tiled_decode_cpu_fallback` and `_decode_on_cpu`. As per coding guidelines: Add type hints for new/modified functions when practical in Python.Suggested signature pattern
- def tiled_decode( - self, - latents, - chunk_size: Optional[int] = None, - overlap: int = 64, - offload_wav_to_cpu: Optional[bool] = None, - ): + def tiled_decode( + self, + latents: torch.Tensor, + chunk_size: Optional[int] = None, + overlap: int = 64, + offload_wav_to_cpu: Optional[bool] = None, + ) -> torch.Tensor:Also applies to: 87-88, 103-104
|
some conflicts need to solve |
317138e to
f7ab5ca
Compare
Summary
Decompose VAE decode logic from
acestep/handler.pyinto focused mixins, plus runtime hardening fixes discovered during validation:gr.skip/gr.update) to avoidFileDatavalidation errors.trying to initialize the default process group twice!.Changes
A) Handler decomposition: VAE decode extraction
acestep/core/generation/handler/vae_decode.pytiled_decode_tiled_decode_cpu_fallback_decode_on_cpuacestep/core/generation/handler/vae_decode_chunks.py_tiled_decode_inner_tiled_decode_gpu_tiled_decode_offload_cpuacestep/core/generation/handler/__init__.pyacestep/handler.pyhandler.pyfacade area (behaviour preserved).B) Audio upload validation hotfix
acestep/gradio_ui/events/generation_handlers.pyvalidate_uploaded_audio_file(...)now returns:gr.skip()for valid/no-op casesgr.update(value=None)for invalid filesacestep/gradio_ui/events/generation_handlers_test.pyC) vLLM re-initialization fix
acestep/llm_inference.py_cleanup_torch_distributed_state()acestep/llm_inference_dist_cleanup_test.pyBehavioural parity / reliability notes
Tests
Added/updated unit tests
acestep/core/generation/handler/vae_decode_test.pyacestep/gradio_ui/events/generation_handlers_test.pyacestep/llm_inference_dist_cleanup_test.pyValidation run
py_compilepassed for changed files.unittestruns are environment-limited by optional deps (torchaudio,gradio) import chain in this environment.Manual UI testing
Manual UI checks passed on available platform paths:
Not executed:
Summary by CodeRabbit
Release Notes
New Features
Tests