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

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

Open
tarepan opened this issue Jun 18, 2024 · 3 comments
Assignees
Labels
機能向上 状態:実装 実装をおこなっている状態

Comments

@tarepan
Copy link
Contributor

tarepan commented Jun 18, 2024

内容

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

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

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

Pros 良くなる点

  • pydantic に依存する範囲の限局

Cons 悪くなる点

無し

実現方法

  • 内部向け構造体を dataclass ベースへ変更
  • バリデーションを TypeAdapter ベースに変更

VOICEVOXのバージョン

0.19.0

その他

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

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

tarepan commented Jun 18, 2024

#1394 より転記)

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

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

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

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

x = SurprisingModel()

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

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

  • 疑問を持たず dataclass ぽく扱って、どこかのタイミングで pydantic の罠にハマってデバッグに難儀する
  • 触る前に pydantic の仕様チェックに膨大な労力を使う

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

@Hiroshiba
Copy link
Member

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

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

@tarepan tarepan added 状態:実装 実装をおこなっている状態 and removed 要議論 実行する前に議論が必要そうなもの 状態:必要性議論 必要性を議論している状態 labels Jun 18, 2024
@tarepan tarepan self-assigned this Jun 18, 2024
@tarepan
Copy link
Contributor Author

tarepan commented Jun 18, 2024

dataclassのが良さそう

👍️
着手します。

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