Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

整理: TTSEngineManager クラスの新設 #1234

Merged
merged 7 commits into from
May 22, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 11, 2024

内容

概要: TTSEngines クラスの新設によるリファクタリング

現在の VOICEVOX ENGINE は TTS エンジン集合に対する操作が複数存在している。
それぞれの操作をそれぞれの箇所で実装しているため実装が重複しており、またテストも不十分である。
TTS エンジン集合に相当するクラスを新設し各種操作をメソッドとして集約すれば、無駄ない実装を確実にテストできる。

このような背景から、TTSEngines クラスの新設によるリファクタリングを提案します。

関連 Issue

無し

Review 向け補足情報

TTSEngines クラスの新設が主である。
置き換えを意図したコア集合に対する操作は以下の通り:

  • get_engines()
  • list(tts_engines.keys())
  • get_latest_version(list(tts_engines.keys()))
  • core_version in tts_engines
  • len(tts_engines)
  • tts_engines[ver] = TTSEngine(core.core)

@tarepan tarepan requested a review from a team as a code owner May 11, 2024 14:31
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 11, 2024 14:31
@tarepan tarepan changed the title refactor: TTSEngines クラスの新設 整理: TTSEngines クラスの新設 May 11, 2024
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

ほぼLGTMです!
コメントした点だけ・・・!

@@ -682,16 +683,50 @@ def frame_synthsize_wave(
return wave


def make_tts_engines_from_cores(cores: dict[str, CoreAdapter]) -> dict[str, TTSEngine]:
class TTSEngines:
Copy link
Member

Choose a reason for hiding this comment

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

クラス名を複数形にするのは色々ややこしくなる気がするので変更したいです!
Managerとかでも。

変更を最小限にする意図でしたらあとでもいいと思います!

Copy link
Contributor Author

@tarepan tarepan May 16, 2024

Choose a reason for hiding this comment

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

Manager

👍️
TTSEngineManager とします。

これに伴い変数名が tts_engine_manager / tts_engine_mgr あたりに変更になると考えます。
ここ弄ると diff が荒れるので本 PR は維持し、後続 PR にて早急に対応します。
ちなみに変数名はどちらがいいでしょうか?(後継 PR にて採用予定)

Copy link
Member

Choose a reason for hiding this comment

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

ありがとうございます!

preset_managerがあるので、揃えるならばtts_engine_managerかなと・・・!
(もちろんいろんな条件があると他のが良いかもですが)

voicevox_engine/tts_pipeline/tts_engine.py Outdated Show resolved Hide resolved
test/tts_pipeline/test_tts_engines.py Outdated Show resolved Hide resolved
voicevox_engine/tts_pipeline/tts_engine.py Show resolved Hide resolved
voicevox_engine/tts_pipeline/tts_engine.py Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented May 16, 2024

@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re-review よろしくお願いします。

@tarepan tarepan changed the title 整理: TTSEngines クラスの新設 整理: TTSEngineManager クラスの新設 May 16, 2024
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

ちょっと1点気になったとこをコメントしました!
けど大きな問題じゃないのでマージします!

voicevox_engine/tts_pipeline/tts_engine.py Show resolved Hide resolved
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.

None yet

2 participants