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

model.pyの型からFastAPI依存だったものを取り除く #262

Closed
qwerty2501 opened this issue Jan 2, 2022 · 6 comments
Closed

model.pyの型からFastAPI依存だったものを取り除く #262

qwerty2501 opened this issue Jan 2, 2022 · 6 comments
Labels

Comments

@qwerty2501
Copy link
Contributor

qwerty2501 commented Jan 2, 2022

内容

https://github.com/VOICEVOX/voicevox_engine/pull/256/files#diff-fe4f36a9a39ec418bb204a851a10f75ec6270144207f94bdfb91e458c5613f0dR11-R12
ここに書いてある内容の修正を行う

  • dataclass化
  • BaseModel,Fieldの除外
@tarepan
Copy link
Contributor

tarepan commented Dec 10, 2023

本 issue は pydantic 依存の解消が目的です。

FIXME: このファイルの各型からpydantic由来の機能を削除し、dataclassにする。

一方、現在の voicevox_enginepydantic の機能を活用したコードが複数箇所あります。
一例:

try:
_presets = parse_obj_as(List[Preset], obj)
except ValidationError:
raise PresetError("プリセットの設定ファイルにミスがあります")

@Hiroshiba
このような背景から次の質問があります:

Q: voicevox engine は pydantic に依存し機能を活用する方針であるか否か?

YES (活用) の場合、本 issue が立った時期とは方針が変更されたため、本 issue は not planned で close 可能です。
NO (削除) の場合、voicevox engine のネストしたかなり深い場所まで依存しているので、計画立てて排除していかないと大きめの技術的負債になりそうです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 10, 2023

@tarepan コメントありがとうございます!!

一言で言うとpydanticに依存しているのはFastAPI側内で使っているためなので、このリポジトリ内での依存は外せないと思います。
リポジトリ全体でpydantic依存を外すのはできませんが、voicevox_engineディレクトリ内でpydantic依存を外すのはありだと思います!

ちょっとこちらのissueに関してかなりややこしくなっているので整理してみました!


そもそもissueの中身に書いていたコードは現在存在していません。ビルド後の実行時エラーが出てリバートされたためです。

このissueの意図はたぶん、WebAPI(FastAPI)用に使われているPydanticのModelと、voicevox_engineディレクトリ内で使うModelを完全に分離することだと思います。
外向けに用意しているAPIは変更が難しいのですが、それをvoicevox_engineディレクトリ内でも使ってしまっているので関心が分離できておらず、そこの解消を目指したものだったと思います。
(この辺りのfrom_engine to_engineで相互変換する形かなと)
https://github.com/VOICEVOX/voicevox_engine/pull/256/files#diff-107d694ff39d1712269187b1e4e5514f59894eaa7e719ed463d0a7c29216ddd6R171-R190

@tarepan
Copy link
Contributor

tarepan commented Dec 10, 2023

issueの意図はたぶん、WebAPI(FastAPI)用に使われているPydanticのModelと、voicevox_engineディレクトリ内で使うModelを完全に分離すること

ぼんやり「そうかな~?」と思っていたのが明確になりました、ありがとうございます!

解消を目指したものだったと思います。(この辺りのfrom_engine``to_engineで相互変換する形かな

具体的コードまでありがとうございます。まさにドメイン変換ですね。


voicevox_engine ディレクトリ内でpydantic依存を外すのはありだと思います!

こちらについて、以下のどの意図/方針が近いでしょうか?

voicevox_engine パッケージ(≠ run.py)内において:

A. 今後の新規コードで pydantic 固有機能は原則利用しない
B. pydantic 固有機能は使っても良いし、逆に今ある依存を消してもいい(≒ 特に規約無し)
C. pydantic 固有機能を活用してコード簡略化するのを推奨

というのも、pydantic が便利(すぎる)機能を多く提供しているため、voicevox_engine パッケージ内の型表現・キャスト・バリデーション等で pydantic が既に使われている現状があります。

VvlibManifest.validate(vvlib_manifest)

def cost2priority(context_id: int, cost: conint(ge=-32768, le=32767)) -> int:

_presets = parse_obj_as(List[Preset], obj)

つまり現状は方針Cへ自然と寄りつつあるということです。

今の VOICEVOX ENGINE が方針 B or C なら特に問題は起きないと思います。
しかし(Hihoさんの解説で明確化したように)この issue は「方針Aを取ろう」というものです。
今後の方針もAな場合、ほっておくとこのままC化が進行していくと考えられるため、規約整備など計画立てて排除していかないと大きめの技術的負債になりそうです。
ゆえに現在/今後の方針が知りたい。
というのが質問の意図詳細になります。

今後の方針はA/B/Cのどれに近いでしょうか?

@Hiroshiba
Copy link
Member

@tarepan 確かにです!! 個人的になんとなくpydanticは排除する方針で良いと感じていましたが、別にpydantic依存することは悪いことじゃないなと思いました。

目的を整理すると、一番やりたいことは「ドメイン変換」かなと。
であればpydanticを使っても使わなくても良いのかなと。
なのでとりあえずAの方針ではなく、BかCの方針が良さそうに思いました!
別に推奨することでもないとは思うので、どっちかって言うとBかなと・・・!

であればこのissueは閉じる方針で良さそうですが、ちょっとこの方針で良いか自信が無いので、もし何かあればご意見いただけると!!

@tarepan
Copy link
Contributor

tarepan commented Dec 12, 2023

B. pydantic 固有機能は使っても良いし、逆に今ある依存を消してもいい(≒ 特に規約無し)
...
どっちかって言うとBかなと

👍

何かあればご意見いただけると

  • pydantic は後方互換性をたまに壊す(例: v1→v2移行)ので、いずれ移行コストが発生しそう
  • voicevox_engine は入れ子クラスをもつので pydantic のインスタンス化関連ユーティリティは超効く

がパッと思いつくところです。
review 時に pydantic が出てきたらちょっと気をつける、くらいが良い塩梅に感じます。

このissueは閉じる方針で良さそう

👍
同意です。

@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 13, 2023

たしかに移行コストは発生しうるかもですね!
仰るとおり、ちょっと気をつけるくらいの気持ちで臨もうかなと思います。

ということでcloseします!! ありがとうございました!!

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

3 participants