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

API・routers と 機能マネージャに関するアーキテクチャ #1449

Open
tarepan opened this issue Jun 29, 2024 · 22 comments
Open
Labels
機能向上 状態:必要性議論 必要性を議論している状態 要議論 実行する前に議論が必要そうなもの 非アクティブ

Comments

@tarepan
Copy link
Contributor

tarepan commented Jun 29, 2024

内容

概要: API・routers と 機能マネージャに関するアーキテクチャの共通認識を得たい

VOICEVOX ENGINE の長期的な発展を目指して、設計(アーキテクチャ)の必要性が複数の issue / PRs で指摘されている。その中でも最上層に近い「API・routers と 機能マネージャ」に関して 1 つのアーキテクチャの方向性が仮説としてあり、これが妥当か否か・どこまで共通認識でどこから議論が必要かを検討したい。
以下具体案と検討点。


VOICEVOX ENGINE は機能が充実・複雑化しており、APIルーターと機能マネージャが 1:1 対応しない前提を取っている。下図に示すように、現段階で既に 1:1 対応していない。

block-beta
  columns 6

    protocol\nserver:1

    block:routers:1
      columns 1
        R["rounters/APIs"]
          style R stroke:#333;
        tts_pipeline
        morphing
        preset
        character
        library
        user_dict
        setting
        engine_info
        portal_page
    end

    space

    block:modules:1
      columns 1
        instances
          style instances stroke:#333;
        TTSEngineManager
        CancellableEngine
        PresetManager
        MetasStore
        ResourceManager
        LibraryManager
        UserDictionary
        SettingHandler
    end

    block:internal:1
      columns 1
        space
        CoreManager
        space space space
    end

    TTSEngineManager  --> tts_pipeline
    PresetManager     --> tts_pipeline
    CancellableEngine --> tts_pipeline

    PresetManager     --> preset

    TTSEngineManager  --> morphing
    MetasStore        --> morphing

    ResourceManager   --> character
    MetasStore        --> character

    LibraryManager    --> library

    UserDictionary    --> user_dict

    SettingHandler    --> setting

    CoreManager --> TTSEngineManager
    CoreManager --> MetasStore
Loading

まず 1 点目として以下の認識確認がしたいです:

Q1: APIルーターと機能マネージャが 1:1 対応しない前提 は共通認識として良いか否か

この前提に立つと、API ルーターを定義するモジュール内では機能マネージャを定義・管理すべきでない(例: routers.tts_pipeline 内で TTSEngineManager は定義・インスタンス化すべきでない)。マネージャの定義場所を決めるルールが難しくなるし、マネージャの利用状況次第で定義場所を変えなくてはならなくなるし、circular dependency に気を配り続けなければいけなくなるからである。
この制約によりアーキテクチャとして「API モジュールと機能モジュールの分離」が要請される。

2 点目として以下の意見に同意か否かを確認したいです:

Q2: 1:1 対応しない前提に基づき「API モジュールと機能モジュールの分離」アーキテクチャを採用すべきである


この 2 点はほぼ全てのディレクトリ構造やクラス定義場所に影響する、アーキテクチャ上の重要な論点と認識しています。
コントリビューターの皆さん(特に @Hiroshiba さん)の意見を伺えれば幸いです。

@tarepan tarepan added 機能向上 要議論 実行する前に議論が必要そうなもの 状態:必要性議論 必要性を議論している状態 labels Jun 29, 2024
@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 29, 2024

図わかりやすいです!!
(全然関係ないけど、ライトテーマだからか背景と文字色が一緒になってるとこがあるかもです。)
image

Q1: APIルーターと機能マネージャが 1:1 対応しない前提 は共通認識として良いか否か

良いと思います。
もちろん、1:1対応しても実装が綺麗な機能はそのままでも良いと思います。user_dictとかsettingとか。

Q2: 1:1 対応しない前提に基づき「API モジュールと機能モジュールの分離」アーキテクチャを採用すべきである

こちらは質問で返してしまうのですが、今すでにそうなっているけど改めて共通認識を作る、という理解で合ってそうでしょうか 👀

@tarepan
Copy link
Contributor Author

tarepan commented Jun 29, 2024

今すでにそうなっているけど改めて共通認識を作る、という理解で合ってそうでしょうか

👍️
はい、その理解で合っています。
暗黙的な実装の構造でなく、明示的なアーキテクチャ方針として共通認識を得る、という立ち位置です。

@Hiroshiba
Copy link
Member

なるほどです、ありがとうございます!!

今のrouterと内部の機能は分ける方針は、実態にうまく刺さってると感じます。
これで問題ない間は、明示的な方針として進めたいです!!

@tarepan
Copy link
Contributor Author

tarepan commented Jul 1, 2024

以下具体案と検討点の続き。


VOICEVOX ENGINE は機能が充実・複雑化し APIs の数も増大している。現段階で 40 以上の API が定義されている。

3 点目として以下の意見に同意か否かを確認したいです:

Q3: API 数を鑑みて「API を複数のルーターへ分割する」アーキテクチャを採用すべきである

このアーキテクチャを採用する場合、.include_router() でルーターを集約する application.py が必須になる。
ところで、VOICEVOX ENGINE は API インスタンス(FastAPI インスタンス)を複数の用途に用いる前提を置いている。現時点の ENGINE では製品版・テスト・ドキュメント生成に用いられる。

4 点目として以下の認識確認がしたいです:

Q4: 「API インスタンスを複数の用途に用いる前提」は共通認識として良いか否か

この前提に従うと、application.py から各ルーターへ渡される機能マネージャは設定が切り替わっていなければならない。このためには application.py が設定値を引数で受け取り内部で機能マネージャをインスタンス化するか、application.py が設定済みの機能マネージャインスタンスを引数で受け取る必要がある。
この 2 つの方法を比較したとき、「設定値の数 > 機能マネージャの数」「機能マネージャのインスタンス化は .include_router() と意味上の共通点が無い(機能的に凝集しえない)」点を鑑みると、機能マネージャを引数とするのが妥当である。ゆえに「API モジュールと機能モジュールの分離」アーキテクチャは、ルーターと機能モジュールの分離に留まらず「application.py / routers と機能モジュールインスタンス化の分離」として捉えるべきである(これをアーキテクチャとすべきである)。

5 点目として以下の意見に同意か否かを確認したいです:

Q5: 引数の数と機能的凝集を鑑みて「application.py と機能モジュールインスタンス化の分離」アーキテクチャを採用すべきである

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 1, 2024

Q3: API 数を鑑みて「API を複数のルーターへ分割する」アーキテクチャを採用すべきである

こちら賛成です!
確かにフレームワークによっては分割が公式でサポートされてないかもですが、その場合でも大体のフレームワークで「importして何かしらのオブジェクトに突っ込む」とかが可能だと思うので、大丈夫かなと!

Q4: 「API インスタンスを複数の用途に用いる前提」は共通認識として良いか否か

これちょっと問いの内容を正確にわかってないのですが、多分提言したいのは導かれる結果の方で、この2点ですよね 👀

  1. generate_approuterは密結合して扱う
  2. 機能マネージャはgenerate_app内で作らず、DIする

僕もトレーニングかねて考えてみたんですが、どう考えればいいのかすごい難しいですね。。
APIを使う側の視点(ユーザーとテスト)から考えてみました!
1は賛成なのですが、2はちょっと条件付きという気持ちです!

各routerとapplication(generate_app)を分けたくなるのはおそらくテストだけだと感じます。
で、テストもユースケースに沿うのであれば、分ける必要ないなと感じました。
なのでapplication / routersを一緒に考えるのはありだと思います!

一方で機能マネージャーをDIするかに関して、そもそもマネージャーに役割が3つありそうに感じました。
1つ目:設定値を元にいい感じに使いやすいインスタンスにすること。
2つ目:DIできること(=マネージャ内の状態を使ってテストできること)。
3つ目:routerを超えて状態を保持すること。

論点にあげたいのが3つ目に関してで、別にそのマネージャーを使ったテストがしたいわけではなく、routerを超えて状態を保持したいだけなのであれば、generate_app内でマネージャーを作った方が役割分担ができてるように感じます。
例えばResourceManagerはこのポジションでも良いかもしれません。
まあ、そういう理想論があるとしても、考えるのが大変だから とりあえず DI 側に全部倒す、みたいなのはそれはそれでありな気はしてます!

ちょっと話はそれちゃうのですが、この場合設定からマネージャを作る場所が複数個コピペになっちゃう問題はあると思います。(↑の3つの役割の1つ目)
これは例えば、プリミティブな設定値を全部所持するConfigを作り、そのConfigからgenerate_appに必要なマネージャーを作る関数を別途生やしてあげれば、DIもできるし設定をうまいことマネージャに落とし込めるしで嬉しいかもとか思いました!
Configの各パラメータ全部に初期値をつけてあげればなかなか使いやすいかも・・・・・?

Q5: 引数の数と機能的凝集を鑑みて「application.py と機能モジュールインスタンス化の分離」アーキテクチャを採用すべきである

前述の通り、「すべき」かは置いといて「メンテコストとの兼ね合いでそういうのもあり」とは思います!
ちなみに「機能マネージャ」が未定義かもです。(定義がめちゃくちゃ難しそうですが。。。)

@tarepan
Copy link
Contributor Author

tarepan commented Jul 2, 2024

routerを超えて状態を保持したいだけなのであれば、generate_app内でマネージャーを作った方が役割分担ができてる

ニュアンスとしては「共有変数 x のポインタを router_arouter_b に分配したいだけなら、分配直前の generate_app() 内で定義した方が見通し良い」という感じでしょうか?
妥当な指摘に感じます。

考えるのが大変だから とりあえず DI 側に全部倒す

これに似た観点として「完璧に設計できない限り不安定さを増す」が挙げられると考えます。
3種の役割に応じて generate_app() 内外へ配置する場合、「新機能のテストで使うようになったから外へ移動しよう」「テストがイマイチだからリバートして内へ戻そう」といった具合で、完璧に設計しない限り移動が増えそうです。


上記の議論を正しく進めるには 別にそのマネージャーを使ったテストがしたいわけではなく、routerを超えて状態を保持したいだけ があると想定すべきか、「各機能は複数の用途に用いる」と想定すべきか、議論の前提を揃える必要があります。
Q4: 「API インスタンスを複数の用途に用いる前提」は共通認識として良いか否か はこれを意図した質問でした。より質問を明確化をすると以下になります:

Q4': 「generate_app() には複数用途がある前提であり、ゆえに各機能マネージャも複数の用途があると想定する」は共通認識として良いか否か

各機能マネージャーが 3 役割のどこまで担うか明確かつそれが安定であるなら、Q4' の想定を置く必要はないと考えます。
各機能マネージャーが 3 役割のどこまで担うか微妙かつ状況によって変わるなら、Q4' の想定を置くと考え事が減って見通しが非常に良くなると考えます。
Q4' の認識確認ができればと思います。

@Hiroshiba
Copy link
Member

Q4': 「generate_app() には複数用途がある前提であり、ゆえに各機能マネージャも複数の用途があると想定する」は共通認識として良いか否か

質問を質問で返してしまうのですが(すみません 🙇 )、generate_appの複数用途というのはどういう意味でしょうか・・・?
ユーザーが起動するエンジン・テスト・APIドキュメントを作るためのインスタンス、みたいな・・・?

「各機能マネージャの複数の用途」もちゃんとわかってないかもです。
複数のrouterで使われるという意味だと思っているのですが、であればgenerate_appは関係なさそうなので、違いそうだなと気づきました。

「generate_app内でクラスのインスタンスを作るのはとりあえずダメ!generate_appの外で作る!理由は実装を統一させるため!」とかならわかります。
それが良いのか自信が持てないので弱気ですが、なにか問題が発覚するまでその流れでも良いのかなと思っています。

@tarepan
Copy link
Contributor Author

tarepan commented Jul 2, 2024

質問を質問で返してしまうのですが(すみません 🙇 )

いえいえ、アーキテクチャの議論は忌憚無い意見が重要ですので、遠慮なく不明瞭点・不明点は指摘して頂ければ🙏

generate_appの複数用途というのはどういう意味 ...
ユーザーが起動するエンジン・テスト・APIドキュメントを作るためのインスタンス、みたいな

👍️
はい、その通りです。
Q4' の前半は「generate_app() は『VOICEVOX ENGINE 生成』『E2E テスト』『docs 生成』等の異なる複数の用途を持つ。このことを前提として良いか」という意味合いです。

「各機能マネージャの複数の用途」もちゃんとわかってないかもです。

こちらは「generate_app() 内の各機能マネージャはどれも『VOICEVOX ENGINE 生成』『E2E テスト』『docs 生成』等の複数用途で使われると想定される」という意味合いです。

generate_app() が複数の用途を持つならば、各機能マネージャの 総体 がこれら複数の用途を実現していることになります。その場合、総体の内実は以下の 2 通りのどちらかだと想定されます:

  • A. 各機能マネージャ(XMng, YMng)がどれも『VOICEVOX ENGINE 生成』『E2E テスト』『docs 生成』等の用途を全て担う
  • B. XMng は全用途、YMngは『VOICEVOX ENGINE 生成』のみで使われる等、機能マネージャごとに用途範囲が違う

Q4' の後半は「A の想定で良いか」という意味合いです。

各機能マネージャーの用途範囲(『VOICEVOX ENGINE 生成』『E2E テスト』『docs 生成』等のどれまで責務があるか)が明確かつそれが安定であるなら、Q4' の想定を置く必要はないと考えます。B のように、routerを超えて状態を保持したいだけ のマネージャーと マネージャ内の状態を使ってテスト もしたいマネージャーを区別して扱えるため、この区別をアーキテクチャへ反映できます。routerを超えて状態を保持したいだけ のマネージャーは generate_app() 内でインスタンス化するとか。
一方、各機能マネージャーの用途範囲が微妙かつ状況によって変わるなら、A の想定を置くと考え事が減って見通しが非常に良くなると考えます。そもそも用途範囲はカッチリ設計できるものじゃない、と割り切るということになります。


理由は実装を統一させるため ...
それが良いのか自信が持てないので弱気

ここの自信の源泉となるロジックを積み上げている形です。
Q4' を前提とした場合、routerを超えて状態を保持したいだけ が無くなるため generate_app() 内でインスタンス化する理由が消滅します。generate_app() 外でインスタンス化する(= DI する)理由は変わらずあるため、DI の方が筋が通ることになります。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 3, 2024

なるほどです!!
丁寧にありがとうございます!! しっくり来てない箇所がわかったかもです。

いろんな機能マネージャーをDIする形は賛成なのですが、その理由が「いろんな用途に使われるから」だという部分に違和感がありました。
いろんな用途に使われないものもある(出てくる)はずなので。

なので多分言葉尻を微調整してこんな感じかなと。
各機能マネージャ(XMng, YMng)がどれも『VOICEVOX ENGINE 生成』『E2E テスト』『docs 生成』等の用途を全て担う・・・かどうかわからないからとりあえず外に出しておく
短く言うなら「設計に柔軟性を持たせるためにgenerate_appで使う機能はDIで渡す」とか・・・?
(理由の1つが「別々の目的で使うことがありえるから」、みたいな。)

この言い回しの感じで認識が一緒、あるいは言ってることは一緒っぽかったらOK だと思います!!
将来、DIしてるインスタンスが結局1回も別の使われ方をしていないなってなったら別の道も考えてもいいかもとは思います!

考え方勉強になりました、ありがとうございます 🙇

@tarepan
Copy link
Contributor Author

tarepan commented Jul 4, 2024

👍️
「用途範囲がいずれ明確化しうる」という立場を取り(= Q4' の前提を取らない)、現時点では「用途範囲が不明確なので、広い用途に対応可能な DI で統一する」ということですね。

どの前提を置くかはプロジェクトの方針次第なので「用途範囲がいずれ明確化しうる」という前提に同意です。
この前提に基づくアーキテクチャとして DI 統一とするのにも賛成です。


現時点で合意が取れたアーキテクチャをまとめます:

アーキテクチャ @ 2024-07-04

VOICEVOX ENGINE は機能が充実し 40 以上の API を定義している。よって『APIs をルーターモジュールへ分割』するアーキテクチャを採用する。ルーターは生成された後に 1 つの APIs インスタンスへ合体される。

機能の複雑化によりルーターと機能マネージャは 1:1 対応していない。よって『ルーターモジュールから機能マネージャの定義・生成を分離』するアーキテクチャを採用する。

APIs インスタンスはサービス提供・テスト・ドキュメント生成等の複数用途で利用される。機能マネージャごとにこれら用途全てを担うか一部のみを担うかは異なるが、現時点で用途範囲は不明確である。不明確なあいだは広い用途に対応可能であるのが望ましい。よって『ルーター生成・APIs 生成と機能マネージャ生成を DI で分離』するアーキテクチャを採用する。


以下、アーキテクチャ検討の続き。


上記アーキテクチャにより、VOICEVOX ENGINE は以下の 3 レイヤーに分離された:

  • A. 機能マネージャを定義し生成する
  • B. 生成済みの機能マネージャから APIs を生成する
  • C. 生成済みの APIs を HTTP サーバーに載せて提供する

依存関係は以下の通り:

  • C は A・B に依存
  • B は A に依存
  • A は独立

A は ENGINE が担う処理の実体であり ENGINE の中核である。B は A の中核機能を外部へ提供するための変換レイヤー・インターフェースレイヤーである。C は APIs を WebAPI としてユーザーへ提供する最外層である。
このように意味としても依存関係としてもレイヤー分けが可能に整理されている。

しかし現在の VOICEVOX ENGINE レポジトリではモジュール・ファイルが以下のように散在している:

  • A: /voicevox_engine/tts_pipeline, /voicevox_engine/metas, /voicevox_engine/resource_manager.py など
  • B: /voicevox_engine/app
  • C: /run.py

そのためレポジトリからレイヤー構造が読み取れない。すなわちレイヤー構造がディレクトリ構造へ反映できていない。
例えば以下のディレクトリ構造を取ればレイヤー構造が明示できる:

/
    voicevox_engine/
        features/
        apis/
        run.py

これにより一目で 3 層がわかり、見通しが改善する。またレビュー時に「features/apis/run.py に依存してはならない」などのルールを容易にチェックし PR Author へ伝えられる。

このような背景から、レイヤー構造(アーキテクチャ)を共通認識としこれをディレクトリ構造へ反映することを提案します。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 4, 2024

現状のまとめありがとうございます!

ディレクトリ分けるのは賛成です!
apisじゃない方のディレクトリ名をfeaturesにすべきかちょっと迷ってます。

ちょっと抽象度が高いですが、modulesとかどうでしょう。
というのも、featuresだと役割が「機能を放り込む場所」になりそうですが、そもそもどれが機能なのか判断がずれそうだなと。
特にmodel.pyが厄介そう。

考えたのですが、役割は「機能を置いておきたい」というより「apiコントローラ以外をまとめときたい」なのかな〜と思いました。
となると抽象度を上げる理由になって、例えばmodulesかなと思った次第です。

あとfeaturesディレクトリがあまり一般的ではないイメージがあり、分かりづらいかなと思ったというのもあります。
こういうディレクトリを作りたいときにfeaturesがよく使われる、とかだったらそちらでも良いのかなと思いました!

@tarepan
Copy link
Contributor Author

tarepan commented Jul 4, 2024

ディレクトリ分けるのは賛成

👍️
良い名称を模索できればと思います。

ちょっと抽象度が高いですが、modulesとかどうでしょう

Python の言語仕様として module が定義されているので、混同しそうです。初見だと「apis も Python module では…?」と感じそうかも?

役割は「機能を置いておきたい」というより「apiコントローラ以外をまとめときたい」なのかな

apis からも run.py からも独立、というのが必要条件であるため、「apiコントローラ以外をまとめる」というよりは「ENGINE 中核処理をまとめる」がニュアンスとしては近いと考えます。
(思考実験として)将来 ENGINE に PyQt 製の GUI 機能を付けるとしたら、「apiコントローラじゃないから features に入れる」ではなく「apis と同じレイヤーなので voicevox_engine/presentations モジュールを作る」となるかと思います。これはfeatures が「ENGINE 中核処理をまとめる」というニュアンスであるがゆえと考えます。

他に良さそうな名前となると、中々悩みますね。
クリーンアーキテクチャ・DDD 周りでありそうな名前だと以下とか?

  • domain
  • models
  • logics
  • business_logics

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 4, 2024

(思考実験として)将来 ENGINE に PyQt 製の GUI 機能を付けるとしたら、「apiコントローラじゃないから features に入れる」ではなく「apis と同じレイヤーなので voicevox_engine/presentations モジュールを作る」となるかと思います。これはfeatures が「ENGINE 中核処理をまとめる」というニュアンスであるがゆえと考えます。

ニュアンスわかります。
ただ実情、appやpresentationに似て層っぽさのあるmodelを内包してしまっているので、「APIかそれ以外」かな~と思った次第でした 😇

他に良さそうな名前となると、中々悩みますね。
クリーンアーキテクチャ・DDD 周りでありそうな名前だと以下とか?

おーーー!!!ありがとうございます!!

business_logicsがまさしくなのですが、まあ伝わら無さそうですよねぇ。。。いわゆるビジネスじゃないので。。。(もちろんそういうことではないのですが)
domainもmodulesと同じく勘違いされそうではありますね。。。modulesよりはだいぶマシそう!

んーーーーーー!!!
logicsdomainですかね!!!
レイヤードアーキテクチャ知ってるとdomainはデータ構造だと勘違いされるかもなので、logicsかなと!!!!!

追記:違いました、レイヤードアーキテクチャにおけるdomainはビジネスロジックでした 😇
うーん。。。domainも良さそう。。。 domainでどうでしょう・・・?(不安)

@tarepan
Copy link
Contributor Author

tarepan commented Jul 5, 2024

domainでどうでしょう

👍️
中核処理というニュアンスも感じ取れ、model が入っていても違和感ないため、domain に賛成です。


他のモジュールはどういう名称が相応しいでしょうか?
現時点だと以下のようになりそうです:

/
    voicevox_engine/
        domain/
        app/
        run.py

appapi にするのは 1 つの手だと考えます。ASGI / FastAPI の文脈だと application で間違いないんですが、初見だと曖昧に見えるという側面もありそうです。ASGI / FastAPI の採用を実装詳細だと見做すと、このモジュールの責務は API 定義なので api が正しいとも言えそうです。


上記アーキテクチャにより、機能マネージャの定義は domain/ へ集約された。
一方、機能マネージャの生成(インスタンス化)の実装は各所に散在している。現在の実装では generate_app() の利用箇所でそれぞれインスタンスが生成されている。

voicevox_engine/run.py

Lines 390 to 395 in abcfa78

if not character_info_dir.exists():
character_info_dir = root_dir / "speaker_info"
# ASGI に準拠した VOICEVOX ENGINE アプリケーションを生成する
app = generate_app(
tts_engines,

app = generate_app(
tts_engines=tts_engines,
core_manager=core_manager,
setting_loader=SettingHandler(USER_SETTING_PATH),
preset_manager=PresetManager( # FIXME: impl MockPresetManager
preset_path=engine_root() / "presets.yaml",
),

@pytest.fixture()
def app(app_params: dict) -> FastAPI:
return generate_app(**app_params)

library_manager = LibraryManager(
get_save_dir() / "installed_libraries",
engine_manifest.supported_vvlib_manifest_version,
engine_manifest.brand_name,
engine_manifest.name,
engine_manifest.uuid,
)
app = generate_app(
tts_engines=tts_engines,

これらの箇所では共通して「設定値の取得 → 設定を反映したインスタンスの生成 → generate_app(instances) の呼び出し」の段階を踏んでいる。
また設定値は基本的に共通しており、各用途ごとに特別な設定の機能マネージャが数個のみ用意されている。

設定値に基づくインスタンス生成は domain の責務と解釈でき、また関数として表現できる。そして「設定値は基本的に共通」は関数のデフォルト引数として表現できる。
つまり現在は各所でバラバラに実装されているインスタンス生成を domain/ 下の def generate_domain(configs=defaults) へ集約できる。各所では generate_domain(specific_config) を呼びその返り値を generate_app() へ渡す形にできる。

このような背景から、インスタンス生成を domain/ 下の関数として共通化することを提案します。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 5, 2024

app を api にするのは 1 つの手だと考えます。ASGI / FastAPI の文脈だと application で間違いないんですが、初見だと曖昧に見えるという側面もありそうです。ASGI / FastAPI の採用を実装詳細だと見做すと、このモジュールの責務は API 定義なので api が正しいとも言えそうです。

apiにするの賛成です!
もしかしたらいろいろ考えていくうちに変わるかもですが、appはややこしい&レイヤードアーキテクチャのどれにも属してないので、一旦わかりやすさを取ってapiの方針で良さそう・・・?

あーーーでもgenerate_appの名前と矛盾しちゃうかも・・・?generate_apiは変な気がしますし・・・。うーん。
うーーーーん。正しさを取ってappかなぁ。。。。applicationでも良いかも・・・?
レイヤードアーキテクチャにapplication層ってありましたっけ。そのapplication層がまさしくappのことであれば、domainとも名前が兄弟感出るし、applicationも良さそう感。

このような背景から、インスタンス生成を domain/ 下の関数として共通化することを提案します。

集約には賛成です!場所もdomain/が良さそうに感じます。
generate関数の名前をどうしようか、あとconfigsが誰の責務なのかも考慮点かもです。
1つ1つがdomainだと思うのでgenerate_all_domainとかが良さそう?
configsの置き場所は・・・・・・・・めちゃくちゃ難しいですね。。。"起動"に関わるところって、レイヤードアーキテクチャのどこになるんでしょう。。。

@tarepan
Copy link
Contributor Author

tarepan commented Jul 5, 2024

application層

domain の外にあるユースケース寄りの層を application と呼ぶのはわりと一般的かと思います。

レイヤードアーキテクチャにapplication層ってありましたっけ

レイヤードアーキテクチャでどう呼ぶかは寡聞にして知らないです🙏(Layerd はレトロニウム寄りなため語彙が整理されていない印象はある)

そのapplication層がまさしくappのことであれば

なんとも微妙なとこですね。application 層が登場する場合は domain-application-presentaion と割っているのが恐らく一般的で、現 app は application+presentation の働きをしていそうです。
機能マネージャーが中核機能を果たし(domain)、app の path operation 関数ボディがオーケストレーションと外部向け型変換の機能を果たし(application)、app@router デコレータが WebAPI インターフェースを提供している(presentation)という理解です。

application とリネームすると「WebAPI 以外のユースケース(application)が出てきたときどうするんだ」となりそうなのもネックです。
となると app 維持が丸いですかね?


generate_all_domainとかが良さそう

👍️
こちらの名称で進めます。

configsが誰の責務なのか

configs は機能マネージャーの設定を全部まとめたものなので(注記: ここでいう "configs" は ENGINE 引数・環境変数そのものとは異なる)、 domain に責務があり domain が正しい置き場所と考えます。application 層・presentation 層・起動方式がどうなっても configs は不変です。

"起動"に関わるところって、レイヤードアーキテクチャのどこになる

"起動" は「ENGINE 引数や env を読み込み、サーバーを実行する」を指している感じでしょうか?
であれば、最外層におかれるという認識です。application 層は起動に依存せず、逆に起動は application 層を呼び出すからです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 5, 2024

なんとも微妙なとこですね。application 層が登場する場合は domain-application-presentaion と割っているのが恐らく一般的で、現 app は application+presentation の働きをしていそうです。
機能マネージャーが中核機能を果たし(domain)、app の path operation 関数ボディがオーケストレーションと外部向け型変換の機能を果たし(application)、app の @router デコレータが WebAPI インターフェースを提供している(presentation)という理解です。

あーーーーなるほどです。2つになっている状況、たしかに。

application とリネームすると「WebAPI 以外のユースケース(application)が出てきたときどうするんだ」となりそうなのもネックです。
となると app 維持が丸いですかね?

ありえるとしたら、WebAPIをAWSにデプロイするコマンドとか、デバッグ用にTTSできるUIとか、UIコンポーネントの提供くらいだと思うので、「アプリケーション」との衝突は無いとは思います!
appという略称を使っているのも微妙かもなので(fastapiの例に合わせた形)、どちらかというとapplicationのが良さそうな気もします。
とはいえぽんぽんディレクトリ名を変えるべきではないという気もするので、appもアリ・・・!
一旦appで置いといて、いろいろ決まっていったあとに再考とかかなぁ。run.pyの名前も変えるなら、ついでに変えても良さそう。

configs は機能マネージャーの設定を全部まとめたものなので(注記: ここでいう "configs" は ENGINE 引数・環境変数そのものとは異なる)、 domain に責務があり domain が正しい置き場所と考えます。

実装コストとの兼ね合いは置いといて理想論でいうと、configsは任意のdomainの情報にアクセスすることになるので、domainとは層が別な気はちょっとします。
とはいえ専用のアダプターを作るのを考えるのも大変なので、とりあえずdomain直下に置くのが落とし所なのかなーと・・・!

"起動" は「ENGINE 引数や env を読み込み、サーバーを実行する」を指している感じでしょうか?

そのつもりだったのですが、思っていることと違うことに気づきました 🙇 🙇
今のコードでいうところのappを作る一連の流れをイメージしてました!
configの型があり、デフォルト値があり、domainを作ってappを作る部分で、run.py・テスト・apiドキュメントから共通で使われるとこです。

まとまってないのですが、もしかしたらconfigの型・configのデフォルト値・generate_all_domaingenerate_appを抱えるアダプター的なモジュールを生やすと綺麗になったりしないかな・・・みたいなことを少し思いました。
・・・微妙そうだったらすみません!!!

@tarepan
Copy link
Contributor Author

tarepan commented Jul 5, 2024

「アプリケーション」との衝突は無い ...

👍️
衝突は想定しないという想定ですね。この方向性に同意です。

appという略称を使っているのも微妙 ... applicationのが良さそう ...
とはいえぽんぽんディレクトリ名を変えるべきではないという気もするので、appもアリ

👍️
一旦 app で現状維持しアーキテクチャ検討とその実装がある程度終わり次第再考、で良いと思います。


configの型・configのデフォルト値・generate_all_domain・generate_appを抱えるアダプター的なモジュールを生やす

「組み立て」的な関数やクラスを集めたモジュールを作る、というニュアンスでしょうか?
時間的凝集・手続き的凝集 vs 機能的凝集、の議論になると思います。つまり、処理の流れが一箇所でわかるようにコードを集めるのが良いか、再利用可能な機能を 1 単位としてコードを集めるのが良いか、という議論です。
機能マネージャーやレイヤー的アーキテクチャとの整合性を考えると、機能的凝集に寄せる(generate_all_domain()domain/ に、generate_app()app/ に)のは自然に感じます。
「綺麗になったりしないかな」の論理・感覚をお聞きできると議論を深められそうです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 5, 2024

一旦 app で現状維持しアーキテクチャ検討とその実装がある程度終わり次第再考

賛成です!

「組み立て」的な関数やクラスを集めたモジュールを作る、というニュアンスでしょうか?
「綺麗になったりしないかな」の論理・感覚をお聞きできると議論を深められそうです。

助かります!!!

始まりは、便利になるからというのもそうですが、そういうレイヤーがあるように感じる感じです!

わかりやすいのがconfigsで、例えばdomain AがBどちらかを選択する設定があった場合、domainではなく使う側の責務に感じます。
あとはデフォルト値を何にするかとかも、使う側の責務に感じるなぁと。ユーザー辞書の保存場所とか。
あと、generate_all_domainは「domain全部見れるもの」になるのですが、全部見れるものって別レイヤーな気がするなーとか。これはそうでもないかも・・・?

あとは、domainに使わずにappだけに使うconfigが出てきたときに困るかも・・・?
と考えてたのですが、これはconfigsではなくdomains_configと考えるとdomain内にあっても良さそうに感じました!

層をまたぐデータ変換をデータトランスファー層として扱う設計を見かけたことがあります。
configsやgenerate_appは、層とユーザーを繋ぐなにかに感じた、というニュアンスです。

・・・・・普通のサービスだとサービス起動方法はユーザーに露出しないけど、VOICEVOX ENGINEは実行もサービスの1つなので、実行レイヤーが考えられる、みたいな感じかも・・・? いやちょっとこじつけかな・・・。
まとまってなくてすみません 🙇 🙇 🙇

@tarepan
Copy link
Contributor Author

tarepan commented Jul 9, 2024

デフォルト値を何にするかとかも、使う側の責務に感じる

👍️
妥当な指摘です。
domain をエンドユーザー向けソフトウェア化するユースケースがありそのデフォルト設定を用意する場合、それは domain ではなく application/usecase での実装が妥当そうです。

domain AがBどちらかを選択する設定があった場合、domainではなく使う側の責務に感じます。
...
全部見れるものって別レイヤーな気がする

domain/generate_domain.py をどう見做すか、という理解に関係していそうです。

  • domain/
    • generate_domain.py
    • subdomain_a/
      • a_mng.py
    • subdomain_b/
      • b_mng.py

とあったとき、下層のサブドメインを統括し単一の domain へまとめ上げるのが generate_domain.py だと理解すると、選択や config がここにあるのは自然に見えます。
別の視点として、generate_domain.py も(subdomain_x 親ディレクトリがたまたま無い)サブドメインの 1 つとして見做すと、たしかに不自然な配置に見えます。

層をまたぐデータ変換をデータトランスファー層として扱う設計 ...
configsやgenerate_appは、層とユーザーを繋ぐなにかに感じた、というニュアンス

👍️
各機能マネージャや各 router といった具体的・個別的な実装と、それを API サービスとしてユーザーへ一体提供する app・run.py の間にある、仲介役的なレイヤーを感じた、と認識しました。
その役割を果たす関数・クラスが実在している、という点に同意です。


generate_all_domain()generate_app() をどこに置くかについて、以下の考察をおこないました。

API 数が少ない場合、API 定義は個別ルーターモジュールではなく generate_app() 内部でおこなわれるはずである。実際、かつて run.py 内で generate_app() が定義されていたときは API が generate_app() 内で定義されていた。アーキテクチャにあるとおり、VOICEVOX ENGINE の複雑化に伴う API 数増加に起因してルーターモジュールへ分離された。つまり generate_app() と各 router は実装詳細として分離されているに過ぎず、機能的に分離しているわけではない。
もし generate_app_1()generate_app_2() があり各ルーターの利用法がそれぞれで違うのであれば、個別機能とアプリ向け組み立てという意味的な分離がありレイヤーを分けるのが正しい。しかし VOICEVOX ENGINE はこのような利用を想定していない。単にモジュールが巨大化したから分離しているに過ぎない。
ゆえに generate_app() を特別なレイヤーへ分離する意義は小さい。

generate_all_domain() はより弱い論拠であるが、generate_app() に近い状況と考える。
generate_all_domain_1() / generate_all_domain_2() で domain を違う使い方する予定もなく、あくまで subdomains が単一の方法で連携・一体化し domain として機能する前提を取っている。
ゆえに generate_all_domain() を特別なレイヤーへ分離する意義は小さい。

(デフォルト値・デフォルト config の置き場所はさておき、)
generate_all_domain()generate_app() のレイヤー化に関する上記考察をどう感じるかお聞きできれば幸いです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 11, 2024

generate_all_domain() と generate_app() のレイヤー化に関する上記考察をどう感じるかお聞きできれば幸いです。

generate_appは仰るとおりappにあって良さそうに感じました!
これは「appを一塊にしたいか」に依存してる気がします。
他の要因がなければappひとかたまりが良さそう。
一方でもし仮に解体するならroutersという名前にもできそう。

generate_all_domainはdomain直下にあるのはどっちかというと少し不自然に思います。
appは全部で1つという思想だけど、、domainは個々を疎結合に作りたいというモチベなので、合わないんだろうなと。
ただ、appをひとかたまりにするなら、domainも一塊のが収まりが良いと感じます。
これがgenerate_all_domain() はより弱い論拠であるが、generate_app() に近い状況と考えるということだと思っていて、ちょっと消去法感はありますが、妥当な範囲かなと。


起動周りをまとめるrunnerレイヤー(名前は説明のために適当につけました)もちょっと考えを進めてみました!

有用性は2点で、使う側(ユーザー・テスト・ドキュメント作成)に合わせて設計できるのと、使う側→domain→app→使う側のデータ変換部分の設計を柔軟に組めることだと思います。
前者の利点があるので、デフォルト値や設定との兼ね合いがしやすくなるはずです。
後者の利点は値をちょっと加工するのがやりやすくなる感じで、条件に応じて作るdomainやappを少し書き換えるとかがやりやすくなりそうです(ResourceManagerにis_development()を渡しているのがこの辺りに該当しそう)。

提案ですが、どういう設計が良いのかはまだ資料が集まっておらず、正確な結論が出せない気がします。
なのでいったんgenerate_appはapp内、generate_all_domainsはdomain内にあるものとして設計を進め、もし有用そうであればrunnerレイヤーを採用を検討、もしくは別の設計を考えるとかどうでしょうか・・・!

という方針で良さそうであれば、ぜひ続きをお願いしたいです!! 🙏

Copy link

本 Issue は直近 30 日間で活動がありません。今後の方針について VOICEVOX チームによる再検討がおこなわれる予定です。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
機能向上 状態:必要性議論 必要性を議論している状態 要議論 実行する前に議論が必要そうなもの 非アクティブ
Projects
None yet
Development

No branches or pull requests

2 participants