VOICEVOX / voicevox_engine

無料で使える中品質なテキスト読み上げソフトウェア、VOICEVOXの音声合成エンジン
https://voicevox.hiroshiba.jp/
Other
1.29k stars 195 forks source link

refactor: 内部向け構造体を `BaseModel` から `dataclass` へ変更する #1405

Open tarepan opened 3 months ago

tarepan commented 3 months ago

内容

概要: 内部向け構造体を BaseModel から dataclass へ変更してリファクタリング

VOICEVOX ENGINE は API 用 BaseModel とは別に、ENGINE 内部で引き回すための内部向け構造体を定義している。 これらは BaseModel を継承している場合が多い。これは pydantic v1 時代から BaseModel 経由で提供されてきたバリデーション機能を使うためだったと思われる。
しかし pydantic は独特の仕様がかなり多く、単なる構造体としては dataclass で十分である。そして pydantic v2 より dataclass に対して pydantic の TypeAdapter でバリデーションが可能になった。これを利用すれば pydantic の利用を限局したうえで安全な dataclass インスタンスを ENGINE 内部で取り回せる。

このような背景から、内部向け構造体を BaseModel から dataclass へ変更するリファクタリングを提案します。

Pros 良くなる点

Cons 悪くなる点

無し

実現方法

VOICEVOXのバージョン

0.19.0

その他

実例として EngineManifestJson および PartOfSpeechDetail へ適用する PR を作成しました(#1394, #1395)。Go/NoGo 判断の一助にお使いください。

tarepan commented 3 months ago

(#1394 より転記)

単なる構造体用途であれば dataclass が適しているが、

そう・・・な気がしていますが、より機能を持ったBaseModelを使うのも悪くない気もしないでもないです。 BaseModelよりdataclassのが良いのは理由とか思いつかれてたりしますか? 👀

@Hiroshiba
「pydantic の仕様を知らなくても良い」ことが dataclass の方が良い理由です。
pydantic には独特の振る舞いが多数あります。例えば以下のコードが正常に実行され(てしまい)ます。

class SurprisingModel(BaseModel):
  version: str = Field("話者のバージョン")

x = SurprisingModel()

パッと見ると Field はフィールドの説明/docstring ぽく見えます(私もずっとこれをスキーマ用 docstring だと思っていました)。しかしこれはデフォルト引数です。実際、x に入っているインスタンスは version=="話者のバージョン" になっています。

ゆえに内部用構造体に触りたい新規コントリビュータは次のいずれかのパターンに嵌ると考えられます:

どちらのパターンも ENGINE OSS のコントリビュータを増やす観点で好ましくないと考えます。
このリスクに対して BaseModel を使う理由は「将来便利機能を使う…かもしれない」であり、デメリットが大幅に勝ると考えています。

Hiroshiba commented 3 months ago

たしかにです!! ただのデータ構造体ではない仕様があって危なそう&労力が掛かりそうな印象です。

dataclassのが良さそうですね!!

tarepan commented 3 months ago

dataclassのが良さそう

👍️
着手します。