Conversation
|
@Hiroshiba |
Hiroshiba
left a comment
Hiroshiba
left a comment
There was a problem hiding this comment.
名前がちょっとややこしくなっていると感じたので再度コメントしました 🙇
ちなみに「内部型」というのは一般的な呼び方でしょうか。
よく使われてる単語があればよいのですが･･･。
voicevox_engine/engine_manifest.py
Outdated
| return ManifestWrapper.from_file(manifest_path) | ||
|
|
||
|
|
||
| def generate_engine_manifest(manifest_wrapper: ManifestWrapper) -> EngineManifest: |
There was a problem hiding this comment.
この関数だけ見ると何をやっているのかかなりややこしいですね･･･
ManifestWrapperが何用なのか、wrapperからwrapperじゃないのを作るのがなぜgenerateなのか、だいぶ混乱しそうです。
ちょっと解決策は思いついてないのですが、どういう名付けをしていくのか考える良い機会な気がしました！
このファイルはAPI用なのか内部用なのかも混乱ポイントかもです。他のファイルに揃える形にすると良さそうかも（揃ってるかもですが。。状況がわかっておらず。。）
There was a problem hiding this comment.
この関数だけ見ると何をやっているのかかなりややこしい
👍️
妥当な指摘です。
まあ、wrapperとかadapterとかでも良さそう。 とのレビューコメントに基づきこの命名にしましたが、たしかにパッと見、よくわからないですね。
このあたり、本質的にややこしいのが厄介です。以下の3種類のクラスが必ず必要になります：
- A. マニフェストファイル JSON をそのまま Python 化したもの（
EngineManifestJson） - B. マニフェストの情報を利用可能に整理したもの（
ManifestWrapper） - C. マニフェストの情報を API 返り値用に一部削除・データ読み込みしたもの（
EngineManifest）
どれもマニフェストの実体に関連するクラスなので、さてどう命名したもんか、という感じです。
B→C は cast ではあるけどファイル読み込みもしてるし、generate_api_manifest() は 別物かと思っちゃう のと指摘を頂いているし…。
良い案があれば提案いただきたいです🙇
どういう名付けをしていくのか考える良い機会
👍️
同意です。
ref #1385 (comment)
このファイルはAPI用なのか内部用なのかも混乱ポイントかもです。
👍️
妥当な指摘です。
本 PR merge 後に router へ移動する PR を出す予定です。これにより API / router 用クラス（EngineManifest）と内部用クラス（ManifestWrapper）がモジュール単位で分離します。
There was a problem hiding this comment.
まあ、wrapperとかadapterとかでも良さそう。 とのレビューコメントに基づきこの命名にしましたが、たしかにパッと見、よくわからないですね。
すみません、これが内部用のデータ構造だと思ってなくてそうコメントしてました。。
良い名付けは思いつかないです。。ちょっと考えるの任せたいです…😇
良い前例に倣うのが手っ取り早い気はしています。どこに前例があるのか知らないのですが…。
とりあえず前例を調べまくってましたが、データ変換はDTOと呼ぶくらいしか出てきませんでした。
そもそもこのファイルは何の概念の中にあるのか、Api側は何の層の概念なのかとかを整理して名付けするとか…？
頭文字だけつける…？Model…？Inner…？
みたいな感じです…😇
There was a problem hiding this comment.
本 PR merge 後に router へ移動する PR を出す予定です。これにより API / router 用クラス（EngineManifest）と内部用クラス（ManifestWrapper）がモジュール単位で分離します。
ちなみに、そこまで実装したものをPRにするとどれくらいの変更量になりそうでしょうか。
プラマイ合計150くらいならそこそこコンパクトな気がしてます。
というのも、「途中なので一旦これで」となってるのは、実は最小機能よりも細かい変更をしていて、プルリクを小さく分けすぎてるかも？と思ったためです。
@tarepan さんにとって名付けに違和感少ないのであれば、もしかしたら完成系が浮かんでるからで、完成系ならこの名付け問題も起こってないかも、みたいな…。
ちょっと試しになのですが、この辺り完成まで一気に変更するとどうなるかPRお願いしても良いでしょうか……🙇
There was a problem hiding this comment.
考えるの任せたい
👍️
承知しました。
命名はとても大事である一方、思い付かないときは思い付かないし最悪変な名前でも動く、との認識です。
author/reviewer 双方が名案を思い付かない場合、FIXME をつけつつ妥協案の名前を採用するのがベターだと考えます。良い変数名が思い付かないから機能追加/リファクタリングを merge しない、はプロジェクト全体の進行を考えるとアンバランスなケースが多そうです。
試しになのですが、この辺り完成まで一気に変更するとどうなるかPRお願いしても良いでしょうか
👍️
本 PR 内で移動含めてリファクタリングします。
プルリクを小さく分けすぎてるかも
以前のレビューで @Hiroshiba さんから「コード移動とコード変更を同時にしないでほしい」との旨の指摘を頂きました（無変更の純粋なコード移動であればレビューツールで自動検知できる的な話だったような）。
それ以降はこれに従い、サイズ関わらず移動と変更を別 PR に分割する方針を取っています。
「同時に移動/変更するのは OK」へ方針変更した、という認識で良いでしょうか？あるいは「今回は命名が難しいので全体像優先で同時に移動/変更」という感じでしょうか？
There was a problem hiding this comment.
author/reviewer 双方が名案を思い付かない場合、FIXME をつけつつ妥協案の名前を採用するのがベターだと考えます。良い変数名が思い付かないから機能追加/リファクタリングを merge しない、はプロジェクト全体の進行を考えるとアンバランスなケースが多そうです。
その時は僕がなんとかして考えます！
極端な話、関数名・変数名がぜんぶA B C D EみたいになってるPRは弾くと思います。
ちゃんと考えれてないですが、納得する言い方としては「機能が揃ってからmergeがよく、名前は機能の１つ」とかなのかなと。
There was a problem hiding this comment.
以前のレビューで @Hiroshiba さんから「コード移動とコード変更を同時にしないでほしい」との旨の指摘を頂きました（無変更の純粋なコード移動であればレビューツールで自動検知できる的な話だったような）。
それ以降はこれに従い、サイズ関わらず移動と変更を別 PR に分割する方針を取っています。「同時に移動/変更するのは OK」へ方針変更した、という認識で良いでしょうか？あるいは「今回は命名が難しいので全体像優先で同時に移動/変更」という感じでしょうか？
なるほどです！！
そういう意図だということに気づいていませんでした。
実装→移動 ではなく 移動→リファクタリング にするとどうでしょう･･･？
There was a problem hiding this comment.
その時は僕がなんとかして考えます！
👍️
困った場合はお任せします。
実装→移動 ではなく 移動→リファクタリング
👍️
可能なケースではその順序で PR 提出する方針に変更します。
一般的でないと思います。「内部向け構造体」にリネームしました。 @Hiroshiba |
|
@Hiroshiba |
Hiroshiba
left a comment
Hiroshiba
left a comment
There was a problem hiding this comment.
実装意図がとても良くわかりました！！
ちょっといくつか課題点がありそうだったのでコメントしました！
Modelの場所をどこにするか決まれば、Modelの場所だけ移動するPRを別で作っていただけるととてもありがたいです･･･！！
| def generate_engine_manifest( | ||
| internal_manifest: EngineManifestInternal, | ||
| ) -> EngineManifest: | ||
| """API 向けのエンジンマニフェストオブジェクトを生成する。""" | ||
| root_dir = internal_manifest.root | ||
| manifest = internal_manifest.model_dump() | ||
|
|
||
| return EngineManifest( | ||
| manifest_version=manifest["manifest_version"], | ||
| name=manifest["name"], | ||
| brand_name=manifest["brand_name"], | ||
| uuid=manifest["uuid"], |
There was a problem hiding this comment.
（ちょっとどっちが良いかわかっておらず、議論コメントです）
Internalの時点でファイル読み込みを終わらせておく手もありそうですが･･････どっちのが良いでしょうか････。
なんとなく、遅延読み込みな今の実装が良さそうな雰囲気はあります。
メリデメ考えられてたら聞きたいです！
There was a problem hiding this comment.
Internalの時点でファイル読み込みを終わらせておく手もありそう
👍️
ご指摘の通り 2 つの実装があり得ます。以下のトレードオフがあります：
「app 立ち上げ時に一度読む」
ENGINE 立ち上げが遅くなるが、GET /engine_manifest レスポンスは速くなる。
「GET /engine_manifest 要求時に都度読む」
ENGINE 立ち上げが速くなり、引き換えに GET /engine_manifest レスポンスが毎回遅くなる（ただし GET /engine_manifest は 1 回しか呼ばれないのが普通の利用方法と思われる）。
ENGINE はスタートアップ速度に心配がある状態なので、後者の遅延読み込み方式が好ましい、つまりこのリファクタリングの方式が好ましいと考えています。
| ) | ||
|
|
||
|
|
||
| class UpdateInfo(BaseModel): |
There was a problem hiding this comment.
モデルはengine_manifest/model.pyとかで持つ方針だったけど、これはrouterで持っておく感じでしょうか？
物によってはinner側がmodelを見ないといけなかったりするので、どこに配置するか迷うことになりそうです。
あと複数routerから見るmodelはrouter内に置けないかもです。
他のmodelはどうなるのか考えて歩調合わせると良さそうですが、どうなりそうでしょう･･･？
（例えば全部router側に置くとか考えられてたり？）
There was a problem hiding this comment.
全部router側に置くとか考えられてたり
👍️
その方針です。以下詳細。
（どこかで方針を説明したつもりだったんですが、してなかったかもです🙇）
（追記: A と B をひっくり返している箇所があったので修正）
API 用モデルと内部用構造体を分離するケース（#1249）では、以下の 2 種類のモデル/構造体が出現します：
- A. ENGINE 内部向けの構造体（例:
EngineManifestInternal） - B. API 返り値用に一部削除・データ読み込みしたもの（例:
EngineManifest）
現在の xx/model.py モジュールには A/B 兼用の構造体が定義されています（各モジュールの docstring に明記済み）。
API は外側のレイヤーでありそのコードは app/ モジュールに分離されています。ゆえに B は本来 app/ 下で定義されるのがアーキテクチャとして正しいです。B の側面をもつ現在の model.py が app/ 下にないのは「内側のレイヤーが参照する A/B 兼用モデル」を外側のレイヤーには置けないからです。
A と B が分かれた場合、上の制約が無くなるため外側レイヤーに属する B を app/ 内へ素直に配置できるようになります。これにより、API の処理とモデルが同モジュール内で書かれて見通しが圧倒的に改善します。
ゆえに、AとBを分離するリファクタリングでは B（API 用モデル）を全部 router 側に置く方針です。
There was a problem hiding this comment.
あれ、なるほどです。（途中からAとBが逆になってるかも）
方針はわかるのですが、考慮がいくつか漏れてるかもです。
- 複数のrouterから必要で、内部から一部変えたものも必要なものはどこに配置するか
- 内部に一部変えたものが必要なものはModelをmodel.pyではなくrouter側に書く、となると、物によってmodel.pyにあったりrouter側にあったりするけど分かりづらくないか
- 内部の実装によってmodelの定義場所が変わる、ということになる
全部model.pyに書く感じかなと思ってました。
強い主張はないけど、↑の問題は解決しときたいな、くらいの気持ちです。
パッと思いついただけなので他にも問題あるかも。
There was a problem hiding this comment.
複数のrouterから必要で、内部から一部変えたものも必要なものはどこに配置するか
機能別に router 分割しているため、ほぼ発生しない見込みです。
現在だと AudioQuery が tts_pipeline と morphing に跨って利用されていますが、よくよく考えるとこれらは別 router であるべきか怪しい気もします（どちらも本質的に音声合成 / AudioQuery → wave の処理をしてる）。
内部の実装によってmodelの定義場所が変わる
👍️
妥当な指摘です。
「全部分かりづらい場所に統一して置いてある」のと「一部が分かりづらい場所に置いてある」のどちらが良いか、という問題だと理解しています。
後者を取る場合、A と B の分割が進めば進むほど「殆どの API モデルは直感的に配置され、一部例外的な兼用モデルのみ xx/model.py にある」状態へ近づいていきます。こうなれば「全部分かりづらい場所に統一して置いてある」よりベターだと考えます。
もし 1 から ENGINE を再実装する人がいたら「API モデルを router に素直に書く。兼用しないと変換コードが煩雑で面倒なモデルがあった場合、妥協案として xx/model.py へ兼用モデルを移す」が自然な方向だとも感じます。
これらを考慮して、定義場所が分散するのは甘んじて受け入れるのが良いと考えます。
There was a problem hiding this comment.
ちょっと考えてみましたが、一旦返信から･･･！
複数のrouterから必要で、内部から一部変えたものも必要なものはどこに配置するか
機能別に router 分割しているため、ほぼ発生しない見込みです。
まあ今はそうかもですが･･･という気持ちです。
よほど簡素なAPI routerじゃないとmodelと１：１対応しなそうで、VOICEVOX ENGINEはすでにその域を超えてるように思います。
例えば音声ライブラリがCharacterに依存しそう、とか。
もし 1 から ENGINE を再実装する人がいたら「API モデルを router に素直に書く。兼用しないと変換コードが煩雑で面倒なモデルがあった場合、妥協案として xx/model.py へ兼用モデルを移す」が自然な方向だとも感じます。
今のENGINE固定ならそういう考え方もあるかもですが、ENGINE APIの成長を考えるともう少し先を見た設計にしても良いと思います。
メンテナ視点、これからも複雑度は上がっていくだろうと感じているので、今の仕様だけ見るのは危ないかもです。
まとめると、もうENGINEの複雑度はmodelとAPI routerが1:1対応する域を超えている（あるいは超え始めている）感じかなと思いました。
「全部分かりづらい場所に統一して置いてある」のと「一部が分かりづらい場所に置いてある」のどちらが良いか、という問題だと理解しています。
これ、そもそもなんでわかりにくくなってるかと言うと、ディレクトリ構造をレイヤーで区切ってる場所とオブジェクトで区切ってる場所があるためなのではとちょっと思ってます。
voicevox_engine/appはプレゼンテーション層な一方、voicevox_engine/presetはオブジェクトで区切られているなぁと。
やるやらは置いといて、またModel/Controller/Presentationで分ければわかりやすくはなる気はしてます。
まとめると、たぶんVOICEVOX ENGINEの複雑度はModelごとにAPIを分けられるようにならなくなり始めてるから、定石にしたがってAPI/Model/Controllerを分けた方が良い･･･気がする、という感じです！
･･･modelごとに全部分けることもできなくはない気もしつつ、でもこっちのほうが良さそうな気がする、みたいな微妙な直感です。
There was a problem hiding this comment.
先を見た設計にしても良い
👍️
YAGNI リスクと将来の綺麗さは天秤なので、1 つの方針として妥当だと感じます。
メンテナ判断の領域だと思うのでこの方針に同意です。
Model/Controller/Presentationで分ければわかりやすくはなる
👍️
非常に同意です。
#1279 はまさにその提案でした。当時の判断は NoGo 寄りだったようですが、タイミングが来たのかもしれません。
「先を見越した設計 & Model/Controller/Presentation 的レイヤー分けを志向する」との認識合わせができたと理解しています。
となると、API 用 BaseModel（上記 B）を app/ 化に移すのはまさに正しい方向性だと考えます。なぜなら分離された B は Model 用クラスでなく Presentation 用クラスだからです。
router への移動は「機能（オブジェクト）区切り」であるため将来の拡張に耐えられない、との部分には同意です。であれば、app/model.py を作り、ここに B を集約すべきと考えます。そうすれば本 PR の意図である 外側レイヤーに属する B を app/ 内へ素直に配置 を実現できます。
There was a problem hiding this comment.
Model/Controller/Presentationで分ければわかりやすくはなる
#1279 はまさにその提案でした。当時の判断は NoGo 寄りだったようですが、タイミングが来たのかもしれません。
#1279 はControllerの分離ですかね。
意図はModelの分離でした。Object/model.pyにしてたのを戻して、model/Object.pyにする的な。
もちろんControllerも分けたほうが良いと思います。
となると、API 用 BaseModel（上記 B）を
app/化に移すのはまさに正しい方向性だと考えます。なぜなら分離された B は Model 用クラスでなく Presentation 用クラスだからです。
router への移動は「機能（オブジェクト）区切り」であるため将来の拡張に耐えられない、との部分には同意です。であれば、app/model.pyを作り、ここに B を集約すべきと考えます。そうすれば本 PR の意図である外側レイヤーに属する B を app/ 内へ素直に配置を実現できます。
あ、いえ、仰ることはわかるのですが、僕が気にしてる点とずれてると思います。
外部用のmodelが置かれる場所がばらばらになってわかりにくくなるのを気にしてます。
例えば、内外で別の構造体hogeは、外用をapi/hoge/model.pyに、内用をhoge/model.pyに書く感じですよね。
一方で内外から使われるfugaは、api/内に置けないので、fuga/model.pyに書くと思います。
こうすると外部用のmodel（制約がいっぱいある）が散らばってわかりにくくなるよなーと。
「内外で共有するものがある」
「内部から外部はimportできない」
「外部用のmodelは特殊」
の時点で、一部のmodelだけを外部のとこに定義するのはできない気がしてます。
外部用modelが書かれた場所がいろんなレイヤーに散らばるややこしさを許容するか（これはやめたい）、外部用modelを書く場所を用意するか（よくある一般的なdomainかentityレイヤーはこれ？）、あるいは共有するのをなくすか（大変そう）かなと。
There was a problem hiding this comment.
意図はModelの分離でした。Object/model.pyにしてたのを戻して、model/Object.pyにする的な。
Model というのは「内部レイヤーを構成する domain、及びその中心クラス」という意味合いではなく、「外部用 BaseModel および 内外兼用 BaseModel」というニュアンスという感じでしょうか？
気にしてる点とずれてる
気にしている点は一致していて、何を良しとするか意見が以下のように分かれている、と思われます。
- @Hiroshiba さんの意見:
外部用modelが書かれた場所がいろんなレイヤーに散らばるややこしさを許容するか（これはやめたい） - @tarepan の意見:
Model/Controller/Presentation 的レイヤー分けを志向するのであれば hoge のように配置するのが筋で fuga (AB兼用) に関して定義場所が分散するのは甘んじて受け入れるのが良い
ただ Model/Controller/Presentation 的レイヤー分けを志向する の解釈が「Model」の指す意味合いで随分変わるので、意見の分かれ云々の前に一旦 ↑ のニュアンス確認をさせて頂けると🙏
There was a problem hiding this comment.
Model というのは「内部レイヤーを構成する domain、及びその中心クラス」という意味合いではなく、「外部用 BaseModel および 内外兼用 BaseModel」というニュアンスという感じでしょうか？
「外に露出するデータ構造」が近そうです！（ちゃんと整理できてなくて、自分の中でもまだずれてるかもです）
内部用のデータ構造も同じだった場合は兼用になったり、 @tarepan さんの仰る中心クラスになったりするかもです。（雰囲気ですが）
|
@Hiroshiba |
Hiroshiba
force-pushed
the
master
branch
3 times, most recently
from
e2f0df9 to
5a6877e
Compare
2 participants
内容
概要: 内部向け構造体
ManifestWrapper内部型を追加してリファクタリングManifestContainer現在の VOICEVOX ENGINE はエンジンマニフェストに関して API 定義クラスと内部型を共有している（
EngineManifest）。その結果「マニフェストファイルに存在している、かつ、
GET /engine_manifestAPI では返らない」情報をエンジン内部で使えなくなっている。つまり API を優先してエンジン内部向け情報を早々に捨ててしまっている。エンジンバージョン情報がその一例である。これは共有デメリットの典型であり（c.f. #1249）、マニフェストに関して内部型を独立させるタイミングが来たといえる。
このような背景から、内部型
ManifestContainerを追加するリファクタリングを提案します。本 PR は
__version__廃止（c.f. #1232）の先行 PR となります。関連 Issue
無し