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

整理: GET /supported_devices API を tts_pipeline router へ移動 #1444

Merged
merged 2 commits into from
Jun 29, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Jun 27, 2024

内容

#1391 に従い、デバイス情報を合成器情報と解釈して tts_pipeline router へ移動するリファクタリングを提案します。

本 PR は移動に特化しています。

関連 Issue

ref #1391 (comment)

@tarepan tarepan requested a review from a team as a code owner June 27, 2024 06:32
@tarepan tarepan requested review from Hiroshiba and removed request for a team June 27, 2024 06:32
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.

あれ、どういう設計になる感じでしょうか

現状、engine_info_routerにtts_engine_managerを渡す設計でも問題ない気が。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 27, 2024

どういう設計になる感じ

GET /supported_devices API のみを移動する設計になります。
よくよく考えたら実装箇所がズレていることに気づいた、という経緯です。以下詳細。

移動する論理

#1391 にて「デバイス情報を合成器情報とみなす」という形になりました(#1391 (comment) )。GET /supported_devices はコアバージョンに基づいてそのバージョンの合成器がサポートするCPU/GPU情報を返すため、これは理にかなっています。

現在の GET /supported_devices API が実装されているルーターは engine_info_router つまり VOICEVOX ENGINE 本体情報のルーターです。
VOICEVOX ENGINE 本体情報はコアバージョンに依存しないはずです。また合成器は ENGINE 本体とは異なる存在です。

合成器の情報を得る GET /is_initialized_speaker API が tts_pipeline router に実装されていることから、同様に合成器情報を得る GET /supported_devicestts_pipeline router が適切です。
ゆえに移動すべきと考えます。


(ちょこちょこ改名提案が出ている)TTSEngineEngine 部分が違和感を消してしまっている原因かもです。
Synthesizer だとすると、engine_info_routerget_synthesizer_supported_deivce(core_version) 的な API があることになり、違和感があります。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 27, 2024

あ、なるほどです!!

/supported_devicesはその(サーバーの方の)エンジンがどのデバイスに対応しているか、というAPIです!!
VOICEVOXはその情報がコアにあるので、コアから得てる・・・という形になっていそうです。

グローバルに対応デバイスが決まる仕様にしたのは特に理由があって決めたわけじゃないかもですが、心理的に「GPUを使うのか、使わないのか」が結構重要な選択なためだと思います。(CUDAの容量がとても重い)
コマンド引数でデバイスを決める・APIの引数でデバイスを変えられない、辺りがほんのりそういう思想寄りかも。

ちょっと設計の提案まで考えられてないのですが、いったんコメントまで 🙇

(以下蛇足かも)

本来はコアごとに対応デバイスが変わるので、これもcore_versionを受け取る形がより正しいのかも?ともちょっと思いました。
でもとりあえず今の仕様に合わせて、そのエンジン(サーバー)は全部GPU対応してるのかしてないのかを返す形が良さそう感。
今はそうなってるというのと、デバイス切り替えは起動コマンドでやるしかないのでそうするのが手っ取り早そう。
(引数指定する場合、結構大変な仕様変更になりそう?デフォルト値をコマンドライン引数から持ってくる的な。。)

音声ライブラリをインストールする場合、今のエンジンの対応デバイスに合うものを持ってこないとですね 😇
どうすべきか結構大変そうだ。。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 29, 2024

一部私が上手く読み取れない箇所があったので、私の理解が正しいか確認させてください🙇


(サーバーの方の)エンジンがどのデバイスに対応しているか

ここでいうエンジンは ONNXRuntime や libtorch という理解であっているでしょうか?


グローバルに対応デバイスが決まる仕様

グローバルというのはどういう意味合いでしょうか?
--runtime_dir で複数種類のランタイム(上記エンジン?)を指定でき、各 TTSEngine がどのデバイスを使えるかはランタイム次第なので、グローバルという言い回しがあまりピンと来ていない感じです。


本来はコアごとに対応デバイスが変わるので、これもcore_versionを受け取る形がより正しい

GET /supported_devices は現時点で core_version を受け取っています。

def supported_devices(
core_version: str | SkipJsonSchema[None] = None,
) -> SupportedDevicesInfo:

ここで指しているのはどの機能(?)に関するものでしょうか?

@Hiroshiba
Copy link
Member

あ、すみません!! /supported_deviceがコアバージョンを受け取らないと勘違いしていました!!
tts_pipelineへ移動する形で問題なさそうに感じます!

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!!

@Hiroshiba Hiroshiba merged commit abcfa78 into VOICEVOX:master Jun 29, 2024
4 checks passed
@Hiroshiba
Copy link
Member

ちょっと別件なのですが相談が!

リファクタリングの影響でblameを追うのがかなりしんどくなるということに気づきました。
ファイル移動の回数をなるべく減らすことがメンテナンス性に寄与しているんだなぁと。
ということで将来の設計を考えておいて、なるべく移動回数を減らす努力をしたいです。

今移動が発生しそうなのがTTSEngine系とtts_pipeline系だと思ってます。
少なくともttsではなくなるとは思っていて、かといってsongをどう足せば良いだろうという感じです。
この辺り先に設計を考えちゃうのはどうでしょうか?

(別issueでやりとりしたほうが後から見返しやすいかも・・・?)

@tarepan tarepan deleted the refactor/move_supported_device_api branch June 29, 2024 06:45
@tarepan
Copy link
Contributor Author

tarepan commented Jun 29, 2024

別issueでやりとりしたほうが後から見返しやすい

👍️
具体的なコードに関する議論(例: tts_pipeline)は別 issue が後で嬉しそうです。
@Hiroshiba さんが感じている課題を issue として具体化して頂ければと思います。


リファクタリングの影響でblameを追うのがかなりしんどくなる ...
ファイル移動の回数をなるべく減らすことがメンテナンス性に寄与

👍️
同意です。
リファクタリングのために ENGINE でちょこちょこ blame しますが、それなりに大変です。

ということで将来の設計を考えておいて、なるべく移動回数を減らす努力をしたい

👍️
総論として設計を努力することに同意です。
ただし(設計努力する前提で)トレードオフはきっちり認識するのが重要と考えます。

「将来移動しなくて済む設計」をするには以下の困難さがあります:

  • 現在の機能・コードを隅々まで理解
  • アーキテクチャの選択肢をもちそれらの利点・欠点を網羅的に理解
  • 将来の機能・コード変更を正しく予測
  • 掛けられる設計コストを見積もるためにプロジェクト成長を正しく予測
  • 議論に参加した OSS コントリビュータが離脱しない短期間のうちに決着
  • 決定したアーキテクチャを OSS コントリビュータへ周知
  • 決定したアーキテクチャをレビュープロセスで保護

私の経験では「当時はカッコいい設計ができたと思ったが、成長したら想像と違う形で結局移動が増えた」「アーキテクチャに関する理解がコントリビュータ間で揃わず議論が紛糾」「移動を恐れて完璧主義ぽくなり議論が停滞、結局頓挫して全部汚いまま」等がわりとありがちです。
移動を減らす方向でちゃんと設計をしつつ多少の移動が出るであろうことは現実として許容する、くらいの温度感が何だかんだ上手くいくと私は理解しています。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 29, 2024

なるほどです!!

個人的には、今リファクタリングしてくださってる @tarepan さんに設計図書いていただければと思ってます…!!
(昔、tts pipelineになったときとかのように)

たぶん現リポジトリ全体の理解度が桁違いに高いのと、たぶん僕よりも疎結合にするのが上手いと思うので力お借りしたいなと思った次第です。
このsupported_deviceとかもそうですが、僕の持ってる先入観より、あるべき設計に沿うことでより良い形になっえいると感じます。
(あと、僕はエディタの方のリファクタリングの設計に四苦八苦してるというのもあります…😇)

設計があると良いなと思う理由の一つに、どこに向かってるかが無いことで、プルリクレビューの精度下がってしまうと実感したためのもあります。
特にコアとWebAPIを密結合させる方針に気付けず、結果として大きめな切り戻しになってしまったなと。

設計がある程度決まってると、どこになにがあるべきか明確になってそれはそれで進めやすいのかなと思ってたりもします!
もしよければ…!🙇

@Hiroshiba
Copy link
Member

あっすみません、こちらのコメントにお答えできてませんでした 🙇

総論として設計を努力することに同意です。
ただし(設計努力する前提で)トレードオフはきっちり認識するのが重要と考えます。
「将来移動しなくて済む設計」をするには以下の困難さがあります:

なるほどです、たしかに設計を尊びすぎるのは良くないと感じます。
そこのトレードオフはわかっているつもりですが、コストとのトレードオフは割と無視してしまうので気をつけたいと思います。
(時間コストをどう考えれば良いかはまだわからないでいます。。)

設計の周知等は、今の量であれば僕が全部見て気にかけられるので大丈夫そうです。
忙しいときは @tarepan さんの力も借りられると心強いです・・・!!

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.

2 participants