run.pyの改善

[y-chan]

内容

run.pyに含まれるFastAPI関連のコードがFastAPIらしい(正確にはFastAPI公式がドキュメントに記している)書き方ではないです。
なので、この際書き直してみるとコードがより整理され、FastAPIらしい綺麗なコードがかけると思います。
要はただのリファクタです。

FastAPIは公式ドキュメント内で、依存するもの(今回であればSynthesis Engineクラス)がある場合、Dependency Injection(依存関係注入)を利用することが可能と示しています。
(参照: https://fastapi.tiangolo.com/tutorial/dependencies/ )
おそらく、現在のような関数内でAPIを定義する形よりも、グローバル下で定義するのが正しい形なのだと思います。

Pros 良くなる点

FastAPIを利用する際の一般的なコードにすることができ、保守性が向上すると思われます。

Cons 悪くなる点

Dependency Injectionを利用すると、各関数にDependsを差し込む引数を用意する必要があり、少し汚く見えてしまうかもしれません。

また、Dependsを利用すると、現状の構造ではCoreをいちいちimportし直す可能性があり、応答速度の低下が起こる可能性があります。まだ未検証事項ではあるので、ここら辺をちゃんと検証したうえでこれをPRに起こすべきか否かを判断すべきと考えます。
(よって、このIssueを取り下げる可能性は十分にあります。)

実現方法

run.pyを書き換える

VOICEVOXのバージョン

0.6.0

[Hiroshiba]

たしかにrun.pyはかなり膨れ上がっているので、改善していきたいです。
FastAPIに合わせていけばきれいになるのかがちょっと判断が難しいですね・・・
一旦それを置いておいて、何を目指したいかを合わせると良いのかなと思いました。

[aoirint]

手の付けやすそうなところから、 run.py を分割していきたいと思っています。

目指す先としては、ENGINEを単体で動くPythonパッケージ化（PyPI上で公開）することを考えています（何を目指したいか、の意図と合っているかわからないですが）。メンテナンスが大変になるかもなので、実際にそこまで進めるのがいいのかはわかりません。

ひとまず、リファクタリングはやったほうが良さそうに思うので、run.py内の実装のモジュール化を細かく進めていこうと思っています（やらない方がよさそうな変更があるかもなので、PRの方でご指摘くださればと思います）。

[aoirint]

SynthesisEngineのインスタンス管理について、FastAPIと関係なく、シングルトンみたいなものを作って管理する方法がありそうだなと思いました（SynthesisEngineのインスタンスを保持するSynthesisEngineRegistryのようなモジュールを作って、初期化などのための関数を用意するとか）。

[Hiroshiba]

目指す先としては、ENGINEを単体で動くPythonパッケージ化（PyPI上で公開）することを考えています

面白そうです！

今ENGINEにはサーバー機能とTTS機能（テキスト→音声機能）が混じっていて、おそらくPyPIになっていて嬉しいのはどちらかというと後者の方かなと感じています。
とりあえずこの２つを分離することを目指し、気力が持てば後者を別リポジトリにしてしまってPyPI化を目指す、というのはどうでしょう。

（今のCOREは音素やら音高やらを処理したベクトルを音声にしているのでかなり玄人向けになっていて、TTSだけをライブラリ化すれば気軽に利用できてユーザーが増えそうなので、すごくやりたいなぁと思っています。）

[aoirint]

とりあえずこの２つを分離することを目指し、気力が持てば後者を別リポジトリにしてしまってPyPI化を目指す、というのはどうでしょう。

いいと思います！

（前者はシェルにコマンドを追加するconsole_scripts機能を使う想定でPyPIに上げることを考えていました。コアライブラリの読み込み・Nuitkaビルドとの兼ね合いなどのメンテナンス性、ライセンス周知など課題があるなと思っています）

[Hiroshiba]

なるほどです、そちらも面白そうですね！
ライセンスに関しては、初回実行時に同意を取ってからダウンロードするという手を思いつきました。
PyPIをインストールしてコマンドとして使うだけなら、Nuitkaビルドは要らないかも・・・？

[qwerty2501]

issue立てるか迷いましたが開発で感じた改善点はここで書く様？なので書きます。
レイヤードアーキテクチャ を採用したほうが良いように感じました。理由はProsに書きます。

Pros

Fast APIで使用する型と内部処理で使用する型を分離できる

例えば #229 では内部で使用されているデータ型がそのままFastAPIでの戻り値の型として使用されていたのでフロントに影響を与えてしまうため安易にフィールドの変更が行えない状況でした。これによりPRのレビュー停滞と回避するためにSynthesisEngineBase内部でWorkaround的な実装する必要がありました。経験上この問題は開発が進んで機能追加が進んでいく毎に顕著に現れやすくなるのでなるべく早く解消したほうが良いと思います。
これはFastAPIを取り扱う層を設けてそこに専用の型を作り、内部で使用している型と変換を行うことにより解消できます。

複数種類のインターフェースを提供しやすくなる

現状engineはhttp 通信のインターフェースしかないですが、開発者のニーズとしては通常のライブラリとして使用したいというのはあるかと。すでにaoirintさんがengineのPythonパッケージ化を提案されてますが、私個人としてはengineをdllで配布し、一般的なC APIの形でも提供しても良いかなと感じました。こうすることにより理論上はほぼ全ての言語に対してengineの機能を提供できます。
dll化による配布がPythonでできるかはいったん置いとくとして、それをやりやすくするために適切なレイヤー分けをしておいたほうが良いかなと思いました。
これはVoiceVoxが掲げてるバリュー向上にも貢献するかと。

Cons

engineをメンテする際の学習コストが上がる

メンテナーがレイヤードアーキテクチャを理解してる必要があるので若干参加するためのハードルが上がります。特に採用したての頃はある程度混乱を招くだろうと予見されます。

リファクタリングコストが高い

完全に採用しようとするとけっこう修正しなければならないと思います。段階的に採用するにしても長い道のりになるかと。

実装する量が増える

典型的なのがレイヤー間の型の詰替え処理を書かなくてはならなくなります。人によってこれは結構苦痛になるようです。
例えばdomain層でAccentPhraseクラスが定義されており、presentation層内部のFastAPIを取り扱う層でもAccentPhraseが定義されていた場合でレスポンスでAccentPhraseを返す必要がある場合は domain層のAccentPhrase->FastAPIでのAccentPhraseへの詰め替えをする処理を書く必要があります。

部分的にアイディアを採用する方法もなくはないですが、それを見極めるためにはやはり理解している必要があります。

[y-chan]

レイヤードアーキテクチャの採用

私には判断しかねますが、個人的にちょっとconsが大きすぎる気がしました。

engineをdllで配布

こちら、Pythonパッケージ化とは別に VOICEVOX/voicevox_core#43 で議論していますので、その件はこちらに移動するとよいかなと思いました...!
まだ議論段階で、やるかどうかは不明ですし、Pythonで書くことは諦めるのが現実的な解決策となりそうな気がしますが...

[Hiroshiba]

僕もレイヤードアーキテクチャに関しては @.y-chan さんとほぼ同じ意見です。
ただ、

Fast APIで使用する型と内部処理で使用する型を分離できる

こちらは確かに必要になってくるだろうなと感じました。
エディタでも同じような課題（サーバーAPIのmodelを流用して実装が大変になる）があったので、そろそろデータ構造の分離はあっても良いと感じています。

[qwerty2501]

そうですね。データ構造の分離だけでも効果はあると思います。

[qwerty2501]

リファクタリングしてしまいたいですが、run.pyに対するテストコードがなくて怖くてできず、かといってリファクタリングしないとSynthesisEngineをDIできないようなので適切なテストコードが書けないという状態になってると思います。
まとめてやってしまうのも方法の一つですが、範囲がかなり大きくなりそうなのでやめたほうが良さそうです。
そこで以下の順で実装するのはどうでしょう？

とりあえず今書けるだけのrun.pyに対するテストコードを書く -> run.pyのリファクタリング(fastAPI部分のみ) -> 本当に必要なrun.pyのテストコードを書く(ここで #230 完了)

[tarepan]

こちらの議論を前提として、現在は以下の個別 issue が走っているとの認識です：

• API-内部データ構造 → model.pyの型からFastAPI依存だったものを取り除く #262
• run.py の分割 → run.pyのgenerate_appなどの関数を分離していく #501
• run.py e2e テスト実装 → run.py・e2eテストを追加していく #730

本 issue は役割を終えたとの認識で合っているでしょうか？（close 可能?）

[Hiroshiba]

@tarepan ほぼ同感です！

あとは「現在のような関数内でAPIを定義する形よりも、グローバル下で定義するのが正しい形なのだと思います」ってのもあるかもです。
個人的にglobalに置くのはテストしにくいので、今の形で良いと思っています。チュートリアルとしてはグローバルにおいた方が見やすいと思います。

ほかは確かに別issueになってたりするので、closeで良いのではと感じました。
一旦closeさせて頂きますが、抜け漏れあればご指摘いただけると助かります🙇