-
Notifications
You must be signed in to change notification settings - Fork 201
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
refactor: コアを各機能モジュールに内蔵 #1391
Comments
たしかにコアのラッパーとしてのTTSEngineを作るのではなく、コアを引数にした関数群にする手はあると思いますが、これだけだと短所が目立つ気がします。 たぶんどこか僕が認知してない部分に辛さがあり、それが解消できるのかなと想像してます。そこを知りたいです。 あと関係ないですが、TTSEngineと呼ぶのをやめてSynthesizerとかにすれば、「エンジン」という名前が衝突する問題は解決するかもです。 |
before def synthesis(..., core_version) -> FileResponse:
engine = tts_engines.get_engine(core_version)
wave = engine.synthesize_wave(
query, style_id, enable_interrogative_upspeak=enable_interrogative_upspeak
) after def synthesis(..., core_version) -> FileResponse:
core = core_manager.get_core(core_version)
wave = synthesize_wave(
core , query, style_id, enable_interrogative_upspeak=enable_interrogative_upspeak
)
#1317 により「
音声合成は 私視点だと「(今となっては)無意味に @Hiroshiba さんの指摘する「短所が目立つ」がピンと来ていないので、詳しく指摘して頂けると幸いです🙇 |
一番伝えたかった点はここです・・・!
まあつまり、routerとのやりとりでエンジンインスタンスを仲介させるか、させないか、どっちが良いのかですよね。
最近のrouterがコアを見てるのはそういう方針というより、そういう実装に寄ってるだけな認識です。 他に差は・・・TTSEngineにステートを持たせたくなるかどうかでしょうか・・・。 うーーーーーーーーーーーーーーーんどっちだろう!!!!! パッと思いつくのはバルク処理とかでしょうか。 他にはコアが推論キャンセル可能になったときとかどこかにステートが必要そう。 あと とはいえ現状解体は可能なのもそうで、2年間変わってない・・・ まとめると、「冗長性をなくす」わけで、パッと判断できない感じな気がしました! もし設計が得意でしたら、上でいくつかあげた例を実装する場合、TTSEngineが良いか、他の何かが良いかとかお聞きしたいです。。 |
あ、あとTTSEngineが無いことによりできるようになることがあれば知りたいです 🙏 あっ あと |
コアを抱えるか渡すかでコード上は以下の違いが生まれます。 wave = engine.synthesize_wave(
query, style_id, enable_interrogative_upspeak=enable_interrogative_upspeak
) wave = synthesize_wave(
core, query, style_id, enable_interrogative_upspeak=enable_interrogative_upspeak
) 違いは
👍️
👍️
逐次生成の場合、FastAPI の
以下の簡略化が可能になります:
以下の機能追加が容易になります:
現在の |
@sabonerune さんがDiscordでコメントしてくださった点を引用
たしかに、昔は変更が必要なとこだけ別クラスになっていたので、forkするときは大変かもです。 TTSEngineを解体する場合、それぞれの関数にコアが必須になり、コアが必要ないエンジンにとってはより不便になると思います。 あと、昔の設計と今の設計を見比べると、そもそも方針が大きく違ってそうでした。 coresやmanagerがなく、enginesのみで管理されてました。 コアが不要なコードは サンプリングレートがコアから毎回取得する形ではなく、全体で1つだけ持っておく方針でした。 |
@tarepan お待たせしました!
層を挟むことでコアと疎結合を保てる、という意図です。 設計について助かります!
レスポンス返す側は仰るとおりrouterに持たせる形だと思います。 でもよく考えると、逐次生成はTTSEngineではなく、リクエストごとに何かしらのインスタンスを作る戦略がシンプルで良い気がしてきました!
こちらもありがとうございます。コアとエンジンの一貫性をなくせるのは大きそうです。 ただ、先ほどのコメントを書くときに思い出したのですが、実は一連のリファクタリングがされる前はCoreManagerがなかったんですよね。 voicevox_engine/voicevox_engine/app/routers/tts_pipeline.py Lines 43 to 71 in a22767c
ぐるっと回ってきて違うとこに着地しようとしてる、って感じだと思います。 やる・やらないは置いといて、今のコードを読んだ感じも、EngineManagerのほうじゃなくCoreManagerの方を解体することもできる気がしました。 長々コメントしてしまってすみません。。。。。 ちょっと一旦のコメントになってしまうのですが、エンジン側じゃなくCoreManager側を解体する視点はどうか聞けると嬉しいです!!🙇 |
可能かつ有効であると考えます。 ただし、この方針を取るなら ENGINE 全体でも同様の方針を取るべきと考えます。 voicevox_engine/voicevox_engine/app/routers/speaker.py Lines 30 to 32 in 00fb774
でないと ゆえに (余談) |
なるほどです!! ほんとですね!! うーーーーーーーーーーん・・・そうかぁ・・・。 あれ、MetasStoreにも内蔵する形はよくない気がしてきました。 以前(0.14時代)は全部 たぶんManagerの乱立よりはEngineに全部押し付けるほうが楽そうな気はしますが・・・それで良いんだろうかという気持ちもありますね・・・。 (まとまってなくてすみません。。 🙇 ) |
はい、
この方針に反対です。 |
なるほどです。 複数のManagerが出来上がると、TTSEngineManagerだけでも辛かったのがもっと更にいろんなManagerが出てくることになりそうで、それで良かったんだっけという感じになるかもです。 仮にManagerを作る場合、Managerはいくつできそうでしょうか? |
次の仮定を起きます:
この仮定では、各クラスがコアを内蔵した結果として以下のマネージャークラスを要します:
よってマネージャーが 3 つになります。 |
ありがとうございます!! 現状整理してみます。 課題はコアをなるべく隠蔽したいことと、シンプルな実装にしたいことの二律背反。 以前の方法は全部をSynthesisEngineで隠蔽する手段。 2つめの方法、コアを前提にしてSynthesisEngineをなくす手段。 3つめの方法、コアに依存する3クラス用のManagerをそれぞれ作る手段。 CoreManager・EngineManager・MetasStoreManagerを作る方法ですが、コアをrouterに露出させるのは元の目的から考えると避けたほうが良さそうに思えてきました。 うーーーーーーーーん。。。。。。 |
こちらの方針に基づいてインターフェースレベルの仮実装をしました。
|
仮実装・・・!!!
!!!! 嬉しみ!!!
これは・・・難しいですね・・・
こちらは |
👍️
現段階ではシンプルで良い方法だと思います。
👍️ |
あーーーなるほどです。 Engine側が良いかなと思いました! (気づいたのですが、もしかしたらMetasは最新のものだけで良いかも・・・?ちょっと考えきれてないですが)
たーーーしかにです。 |
内容
TTSEngine
を解体してリファクタリング多数のリファクタリングの積み上げにより、現在の VOICEVOX ENGINE は機能モジュール間が疎結合になっている。特にコアのインターフェースは洗練され、また利用パターンが明確になってきている。典型的には以下のパターンで利用される:
core_manager
を router へ注入core_version
に基づき特定バージョンのコアを取得TTSEngine
は初期にリファクタリングされ、コアをTTSEngine.__init__()
で受け取る設計になっている。これは当時妥当な設計であったが、コアが洗練された現在では過剰に state をもつ設計になっている。
TTSEngine
のインスタンス変数はself._core
のみであるため、コア受け入れを廃止すれば stateless に関数で音声合成ができる。これは音声合成機能の実装を大幅に簡略化する。このような背景から、
TTSEngine
を解体するリファクタリングを提案します。Pros 良くなる点
Cons 悪くなる点
実現方法
VOICEVOXのバージョン
0.19.0
その他
以下の先行 PR に依存しています:
The text was updated successfully, but these errors were encountered: