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

pydantic系の処理を厳密にする #995

Open
Hiroshiba opened this issue Jan 8, 2024 · 8 comments
Open

pydantic系の処理を厳密にする #995

Hiroshiba opened this issue Jan 8, 2024 · 8 comments
Labels
機能向上 状態:必要性議論 必要性を議論している状態 要議論 実行する前に議論が必要そうなもの 非アクティブ

Comments

@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 8, 2024

内容

型を便利に扱えるpydanticをよく使っていますが、デフォルトの状態だと結構扱いが特殊なことに気づきました。
デフォルトの状態をより良くすることで、ミスを防ぎやすくなると思われます。

このissueはpydanticをより使いやすくするための手順をメモしたものです。

Pros 良くなる点

よりコードが書きやすくなる。(というよりミスしなくなる)

実現方法

  1. pydanticをv2にする
    • v1から挙動がいくつか変わって分かりやすくなってる
  2. いろいろ良い感じの共通BaseModelを作って、全てから依存する
    • 例えばデフォルトでは再代入時に型検査をしてくれないので、validate_assignmentをTrueにする
    • 他にもstrictなど便利そうなものは設定しても良さそう
    • 配置するならmodel/base.pyとか?
  3. 開発モードと製品モードを切り替えて、製品モードの時はより軽い状態にする
    • 2で設定したものはおそらくある程度処理が重くなってしまうので、製品モードの時はもっと検査を緩めて軽くすると良さそう

その他

(本当は1個1個やりたいのですが、ちょっと他のことやらないといけないので、とりあえずメモしてみました。どなたか進めていただけると。。。)

@Hiroshiba Hiroshiba added 機能向上 初心者歓迎タスク 初心者にも優しい簡単めなタスク labels Jan 8, 2024
@tarepan
Copy link
Contributor

tarepan commented Jan 9, 2024

ref #262 (comment)

@tarepan
Copy link
Contributor

tarepan commented Jun 28, 2024

pydantic に関連する様々な検討と実装がおこなわれ、issue の課題は大方解決しました。

@Hiroshiba
本 issue は resolve につき close 可能そうです。

@tarepan tarepan removed the 状態:実装者募集 実装者を募集している状態 label Jun 28, 2024
@Hiroshiba
Copy link
Member Author

まだ実現方法の2と3が未達かもです!

結構実装も変わったので、そもそもやる・やらないを議論しても良いかも。
個人的には、再代入時に型エラーが出ないのはいつかanyを代入したときとかに問題になりそうだな〜とちょっとだけ思いました。
strictもミスに気づけるのが早くなりそう…?

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

本 Issue は直近 30 日間で活動がありません。今後の方針について VOICEVOX チームによる再検討がおこなわれる予定です。

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Jul 29, 2024

pydanticデフォルトのBaseModelを使う形だと若干不安ではあるものの、大多数において問題は無いとは思われます。
若干不安な理由は、デフォルトのだと再代入のときに型検査をしてくれないなど、意図しない挙動がちらほらあるためです。

とはいえissueにするほどなのかどうかが判断むずかしいです。
少なくとも今のissueの形だと、細分化されたタスクのうち最初のが終わっている少しわかりにくい状態なので、小分けにしてissueを作ったほうが良いかもです。

@tarepan さん的にどうでしょう・・・?

@sabonerune
Copy link
Contributor

試していないのでもしかしたら違うかもしれませんがstrictを有効化した場合APIからの入力データの検証が厳しくなるかもしれません。
現状のエンジンで試したところJSONの数値を文字列で渡すと自動的に数値に変換して動作するという挙動がありました。

もしstrictを常時有効化する場合、少なくともアナウンスが必要な変更になってしまうかもしれません。
またstrictを開発時とリリースに動作を切り替えるのはPydanticのValidationの挙動を変えてしまうのでリリース時のみ発生するバグを生む原因になるかもしれません。

現状でstrictを有効化するのはVOICEVOX内部で扱うデータのみにするべきかもしれません。

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Jul 29, 2024

あ〜なるほどです!! たしかに破壊的変更になってしまうかもしれませんね・・・。
最初からstrictであればそちらのほうが良いけど、わざわざ変えるほどかというと・・・。

現状でstrictを有効化するのはVOICEVOX内部で扱うデータのみにするべきかもしれません。

strict等に頼りたい理由はコーディングミスを減らすという意図だったのですが、そういえば内部で扱うデータはpydanticではなくdataclassを使うようになってきているので効果は薄いかもしれません。

うーん、となるとstrictやついでにvalidate_assignmentもまあなくても良い・・・・かなぁ。
何かしらのミスが起こったら再考という形でも良いのかなと思えてきました!

Copy link

本 Issue は直近 30 日間で活動がありません。今後の方針について VOICEVOX チームによる再検討がおこなわれる予定です。

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