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向けと内部向けで分離 #1359

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build_util/make_docs.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def generate_api_docs_html(schema: str) -> str:
engine_manifest = load_manifest(engine_manifest_path())
library_manager = LibraryManager(
get_save_dir() / "installed_libraries",
engine_manifest.supported_vvlib_manifest_version,
None, # NOTE: `engine_manifest.supported_vvlib_manifest_version` を実装予定
engine_manifest.brand_name,
engine_manifest.name,
engine_manifest.uuid,
Expand Down
2 changes: 1 addition & 1 deletion run.py
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ def main() -> None:

library_manager = LibraryManager(
get_save_dir() / "installed_libraries",
engine_manifest.supported_vvlib_manifest_version,
None, # NOTE: `engine_manifest.supported_vvlib_manifest_version` を実装予定
engine_manifest.brand_name,
engine_manifest.name,
engine_manifest.uuid,
Expand Down
2 changes: 1 addition & 1 deletion test/benchmark/engine_preparation.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def _generate_engine_fake_server(root_dir: Path) -> TestClient:
engine_manifest = load_manifest(engine_manifest_path())
library_manager = LibraryManager(
get_save_dir() / "installed_libraries",
engine_manifest.supported_vvlib_manifest_version,
None, # NOTE: `engine_manifest.supported_vvlib_manifest_version` を実装予定
engine_manifest.brand_name,
engine_manifest.name,
engine_manifest.uuid,
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def app_params(tmp_path: Path) -> dict[str, Any]:
engine_manifest = load_manifest(engine_manifest_path())
library_manager = LibraryManager(
get_save_dir() / "installed_libraries",
engine_manifest.supported_vvlib_manifest_version,
None, # NOTE: `engine_manifest.supported_vvlib_manifest_version` を実装予定
engine_manifest.brand_name,
engine_manifest.name,
engine_manifest.uuid,
Expand Down
Empty file added test/unit/__init__.py
Empty file.
18 changes: 18 additions & 0 deletions test/unit/test_engine_manifest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
"""エンジンマニフェストのテスト"""

from pathlib import Path

from voicevox_engine.engine_manifest import EngineManifestInternal


def test_manifest_internal_init() -> None:
"""`EngineManifestInternal.from_file()` でインスタンスが生成される。"""
EngineManifestInternal.from_file(Path("engine_manifest.json"))
assert True


def test_manifest_internal_relative_path() -> None:
"""`EngineManifestInternal.root` を用いて相対パスが解決できる。"""
wrapper = EngineManifestInternal.from_file(Path("engine_manifest.json"))
tos_path = wrapper.root / wrapper.terms_of_service
assert tos_path.exists()
8 changes: 4 additions & 4 deletions voicevox_engine/app/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from voicevox_engine.app.routers.user_dict import generate_user_dict_router
from voicevox_engine.cancellable_engine import CancellableEngine
from voicevox_engine.core.core_initializer import CoreManager
from voicevox_engine.engine_manifest import EngineManifest
from voicevox_engine.engine_manifest import EngineManifestInternal
from voicevox_engine.library.library_manager import LibraryManager
from voicevox_engine.metas.MetasStore import MetasStore
from voicevox_engine.preset.preset_manager import PresetManager
Expand All @@ -37,7 +37,7 @@ def generate_app(
setting_loader: SettingHandler,
preset_manager: PresetManager,
user_dict: UserDictionary,
engine_manifest: EngineManifest,
engine_manifest: EngineManifestInternal,
library_manager: LibraryManager,
cancellable_engine: CancellableEngine | None = None,
speaker_info_dir: Path | None = None,
Expand Down Expand Up @@ -73,7 +73,7 @@ def generate_app(
app.include_router(
generate_speaker_router(core_manager, metas_store, speaker_info_dir)
)
if engine_manifest.supported_features.manage_library:
if engine_manifest.supported_features.manage_library.value:
app.include_router(generate_library_router(library_manager))
app.include_router(generate_user_dict_router(user_dict))
app.include_router(generate_engine_info_router(core_manager, engine_manifest))
Expand All @@ -83,7 +83,7 @@ def generate_app(
app.include_router(generate_portal_page_router(engine_manifest.name))

app = configure_openapi_schema(
app, engine_manifest.supported_features.manage_library
app, engine_manifest.supported_features.manage_library.value
)

return app
122 changes: 119 additions & 3 deletions voicevox_engine/app/routers/engine_info.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
"""エンジンの情報機能を提供する API Router"""

import json
from base64 import b64encode
from typing import Self

from fastapi import APIRouter, HTTPException
Expand All @@ -9,7 +11,11 @@
from voicevox_engine import __version__
from voicevox_engine.core.core_adapter import DeviceSupport
from voicevox_engine.core.core_initializer import CoreManager
from voicevox_engine.engine_manifest import EngineManifest
from voicevox_engine.engine_manifest import (
BrandName,
EngineManifestInternal,
EngineName,
)


class SupportedDevicesInfo(BaseModel):
Expand All @@ -31,8 +37,118 @@ def generate_from(cls, device_support: DeviceSupport) -> Self:
)


class UpdateInfo(BaseModel):
Copy link
Member

@Hiroshiba Hiroshiba Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

モデルはengine_manifest/model.pyとかで持つ方針だったけど、これはrouterで持っておく感じでしょうか?

物によってはinner側がmodelを見ないといけなかったりするので、どこに配置するか迷うことになりそうです。
あと複数routerから見るmodelはrouter内に置けないかもです。
他のmodelはどうなるのか考えて歩調合わせると良さそうですが、どうなりそうでしょう・・・?
(例えば全部router側に置くとか考えられてたり?)

Copy link
Contributor Author

@tarepan tarepan Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

全部router側に置くとか考えられてたり

👍️
その方針です。以下詳細。

(どこかで方針を説明したつもりだったんですが、してなかったかもです🙇)
(追記: A と B をひっくり返している箇所があったので修正)

API 用モデルと内部用構造体を分離するケース(#1249)では、以下の 2 種類のモデル/構造体が出現します:

  • A. ENGINE 内部向けの構造体(例: EngineManifestInternal
  • B. API 返り値用に一部削除・データ読み込みしたもの(例: EngineManifest

現在の xx/model.py モジュールには A/B 兼用の構造体が定義されています(各モジュールの docstring に明記済み)。

API は外側のレイヤーでありそのコードは app/ モジュールに分離されています。ゆえに B は本来 app/ 下で定義されるのがアーキテクチャとして正しいです。B の側面をもつ現在の model.pyapp/ 下にないのは「内側のレイヤーが参照する A/B 兼用モデル」を外側のレイヤーには置けないからです。

A と B が分かれた場合、上の制約が無くなるため外側レイヤーに属する B を app/ 内へ素直に配置できるようになります。これにより、API の処理とモデルが同モジュール内で書かれて見通しが圧倒的に改善します。

ゆえに、AとBを分離するリファクタリングでは B(API 用モデル)を全部 router 側に置く方針です。

Copy link
Member

@Hiroshiba Hiroshiba Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あれ、なるほどです。(途中からAとBが逆になってるかも)

方針はわかるのですが、考慮がいくつか漏れてるかもです。

  • 複数のrouterから必要で、内部から一部変えたものも必要なものはどこに配置するか
  • 内部に一部変えたものが必要なものはModelをmodel.pyではなくrouter側に書く、となると、物によってmodel.pyにあったりrouter側にあったりするけど分かりづらくないか
    • 内部の実装によってmodelの定義場所が変わる、ということになる

全部model.pyに書く感じかなと思ってました。
強い主張はないけど、↑の問題は解決しときたいな、くらいの気持ちです。
パッと思いついただけなので他にも問題あるかも。

Copy link
Contributor Author

@tarepan tarepan Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

複数のrouterから必要で、内部から一部変えたものも必要なものはどこに配置するか

機能別に router 分割しているため、ほぼ発生しない見込みです。
現在だと AudioQuerytts_pipelinemorphing に跨って利用されていますが、よくよく考えるとこれらは別 router であるべきか怪しい気もします(どちらも本質的に音声合成 / AudioQuerywave の処理をしてる)。

内部の実装によってmodelの定義場所が変わる

👍️
妥当な指摘です。
「全部分かりづらい場所に統一して置いてある」のと「一部が分かりづらい場所に置いてある」のどちらが良いか、という問題だと理解しています。
後者を取る場合、A と B の分割が進めば進むほど「殆どの API モデルは直感的に配置され、一部例外的な兼用モデルのみ xx/model.py にある」状態へ近づいていきます。こうなれば「全部分かりづらい場所に統一して置いてある」よりベターだと考えます。
もし 1 から ENGINE を再実装する人がいたら「API モデルを router に素直に書く。兼用しないと変換コードが煩雑で面倒なモデルがあった場合、妥協案として xx/model.py へ兼用モデルを移す」が自然な方向だとも感じます。
これらを考慮して、定義場所が分散するのは甘んじて受け入れるのが良いと考えます。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちょっと考えてみましたが、一旦返信から・・・!

複数のrouterから必要で、内部から一部変えたものも必要なものはどこに配置するか

機能別に router 分割しているため、ほぼ発生しない見込みです。

まあ今はそうかもですが・・・という気持ちです。
よほど簡素なAPI routerじゃないとmodelと1:1対応しなそうで、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ごとに全部分けることもできなくはない気もしつつ、でもこっちのほうが良さそうな気がする、みたいな微妙な直感です。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

先を見た設計にしても良い

👍️
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/ 内へ素直に配置 を実現できます。

Copy link
Member

@Hiroshiba Hiroshiba Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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レイヤーはこれ?)、あるいは共有するのをなくすか(大変そう)かなと。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

意図はModelの分離でした。Object/model.pyにしてたのを戻して、model/Object.pyにする的な。

Model というのは「内部レイヤーを構成する domain、及びその中心クラス」という意味合いではなく、「外部用 BaseModel および 内外兼用 BaseModel」というニュアンスという感じでしょうか?

気にしてる点とずれてる

気にしている点は一致していて、何を良しとするか意見が以下のように分かれている、と思われます。

  • @Hiroshiba さんの意見: 外部用modelが書かれた場所がいろんなレイヤーに散らばるややこしさを許容するか(これはやめたい)
  • @tarepan の意見: Model/Controller/Presentation 的レイヤー分けを志向する のであれば hoge のように配置するのが筋で fuga (AB兼用) に関して 定義場所が分散するのは甘んじて受け入れるのが良い

ただ Model/Controller/Presentation 的レイヤー分けを志向する の解釈が「Model」の指す意味合いで随分変わるので、意見の分かれ云々の前に一旦 ↑ のニュアンス確認をさせて頂けると🙏

Copy link
Member

@Hiroshiba Hiroshiba Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Model というのは「内部レイヤーを構成する domain、及びその中心クラス」という意味合いではなく、「外部用 BaseModel および 内外兼用 BaseModel」というニュアンスという感じでしょうか?

「外に露出するデータ構造」が近そうです!(ちゃんと整理できてなくて、自分の中でもまだずれてるかもです)
内部用のデータ構造も同じだった場合は兼用になったり、 @tarepan さんの仰る中心クラスになったりするかもです。(雰囲気ですが)

"""
エンジンのアップデート情報
"""

version: str = Field(title="エンジンのバージョン名")
descriptions: list[str] = Field(title="アップデートの詳細についての説明")
contributors: list[str] | SkipJsonSchema[None] = Field(
default=None, title="貢献者名"
)


class LicenseInfo(BaseModel):
"""
依存ライブラリのライセンス情報
"""

name: str = Field(title="依存ライブラリ名")
version: str | SkipJsonSchema[None] = Field(
default=None, title="依存ライブラリのバージョン"
)
license: str | SkipJsonSchema[None] = Field(
default=None, title="依存ライブラリのライセンス名"
)
text: str = Field(title="依存ライブラリのライセンス本文")


class SupportedFeatures(BaseModel):
"""
エンジンが持つ機能の一覧
"""

adjust_mora_pitch: bool = Field(title="モーラごとの音高の調整")
adjust_phoneme_length: bool = Field(title="音素ごとの長さの調整")
adjust_speed_scale: bool = Field(title="全体の話速の調整")
adjust_pitch_scale: bool = Field(title="全体の音高の調整")
adjust_intonation_scale: bool = Field(title="全体の抑揚の調整")
adjust_volume_scale: bool = Field(title="全体の音量の調整")
interrogative_upspeak: bool = Field(title="疑問文の自動調整")
synthesis_morphing: bool = Field(
title="2種類のスタイルでモーフィングした音声を合成"
)
sing: bool | SkipJsonSchema[None] = Field(default=None, title="歌唱音声合成")
manage_library: bool | SkipJsonSchema[None] = Field(
default=None, title="音声ライブラリのインストール・アンインストール"
)


class EngineManifest(BaseModel):
"""
エンジン自体に関する情報
"""

manifest_version: str = Field(title="マニフェストのバージョン")
name: EngineName = Field(title="エンジン名")
brand_name: BrandName = Field(title="ブランド名")
uuid: str = Field(title="エンジンのUUID")
url: str = Field(title="エンジンのURL")
icon: str = Field(title="エンジンのアイコンをBASE64エンコードしたもの")
default_sampling_rate: int = Field(title="デフォルトのサンプリング周波数")
frame_rate: float = Field(title="エンジンのフレームレート")
terms_of_service: str = Field(title="エンジンの利用規約")
update_infos: list[UpdateInfo] = Field(title="エンジンのアップデート情報")
dependency_licenses: list[LicenseInfo] = Field(title="依存関係のライセンス情報")
supported_vvlib_manifest_version: str | SkipJsonSchema[None] = Field(
default=None, title="エンジンが対応するvvlibのバージョン"
)
supported_features: SupportedFeatures = Field(title="エンジンが持つ機能")


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"],
Comment on lines +110 to +121
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ちょっとどっちが良いかわかっておらず、議論コメントです)

Internalの時点でファイル読み込みを終わらせておく手もありそうですが・・・・・・どっちのが良いでしょうか・・・・。
なんとなく、遅延読み込みな今の実装が良さそうな雰囲気はあります。
メリデメ考えられてたら聞きたいです!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internalの時点でファイル読み込みを終わらせておく手もありそう

👍️
ご指摘の通り 2 つの実装があり得ます。以下のトレードオフがあります:

「app 立ち上げ時に一度読む」
ENGINE 立ち上げが遅くなるが、GET /engine_manifest レスポンスは速くなる。

GET /engine_manifest 要求時に都度読む」
ENGINE 立ち上げが速くなり、引き換えに GET /engine_manifest レスポンスが毎回遅くなる(ただし GET /engine_manifest は 1 回しか呼ばれないのが普通の利用方法と思われる)。

ENGINE はスタートアップ速度に心配がある状態なので、後者の遅延読み込み方式が好ましい、つまりこのリファクタリングの方式が好ましいと考えています。

url=manifest["url"],
default_sampling_rate=manifest["default_sampling_rate"],
frame_rate=manifest["frame_rate"],
icon=b64encode((root_dir / manifest["icon"]).read_bytes()).decode("utf-8"),
tarepan marked this conversation as resolved.
Show resolved Hide resolved
terms_of_service=(root_dir / manifest["terms_of_service"]).read_text("utf-8"),
update_infos=[
UpdateInfo(**update_info)
for update_info in json.loads(
(root_dir / manifest["update_infos"]).read_text("utf-8")
)
],
# supported_vvlib_manifest_versionを持たないengine_manifestのために
# キーが存在しない場合はNoneを返すgetを使う
supported_vvlib_manifest_version=manifest.get(
"supported_vvlib_manifest_version"
),
dependency_licenses=[
LicenseInfo(**license_info)
for license_info in json.loads(
(root_dir / manifest["dependency_licenses"]).read_text("utf-8")
)
],
supported_features={
key: item["value"] for key, item in manifest["supported_features"].items()
},
)


def generate_engine_info_router(
core_manager: CoreManager, engine_manifest_data: EngineManifest
core_manager: CoreManager, engine_manifest_data: EngineManifestInternal
) -> APIRouter:
"""エンジン情報 API Router を生成する"""
router = APIRouter(tags=["その他"])
Expand Down Expand Up @@ -60,6 +176,6 @@ def supported_devices(
@router.get("/engine_manifest")
async def engine_manifest() -> EngineManifest:
"""エンジンマニフェストを取得します。"""
return engine_manifest_data
return generate_engine_manifest(engine_manifest_data)

return router
Loading