refactor: NumPy のレガシー関数を置き換え・ ruff NPY を導入
#1644
Conversation
|
と同様、一旦さきに静的解析を頑張る理由を整理できると嬉しいです･･･！ |
Hiroshiba
force-pushed
the
master
branch
3 times, most recently
from
e2f0df9 to
5a6877e
Compare
tarepan
changed the title
NPY を導入
tarepan
changed the title
NPY を導入NPY を導入
| # Inputs | ||
| query = _gen_query(volumeScale=2, outputSamplingRate=48000, outputStereo=True) | ||
| raw_wave = np.random.rand(240).astype(np.float32) | ||
| raw_wave = np.random.default_rng().random(240).astype(np.float32) |
There was a problem hiding this comment.
（ただのコメントです）
こーーーーーのルールは結構おせっかいというか、思想強めな気がしますねぇ！！
deprecatedの根拠としてる部分はここですが、
https://numpy.org/doc/stable/reference/random/legacy.html#legacy-random-generation
おそらく「より良いランダムを冪等に得るにはこっちが良い」くらいの気持ちな気がしました。
「numpyのコードを正しく書こう」というより、「ランダムを正しく得よう」というモチベに立つと、たしかに非推奨になる。
でも僕達が使ってるrandomの場所はそこそこな乱数であれば良いので、むしろnp.random.randのが直感的な気がしますね･･･。
実際この関数のドキュメント見に行ってもどこにも非推奨だと書いてないし･･･。
https://numpy.org/doc/stable/reference/random/generated/numpy.random.rand.html
まあ僕達はnumpyの中の、数値ベクトルを一気に計算できるとこだけしか使ってないのでレアケース寄りなんですが･･･。
（numpyをよく使う自分的には結構ありがたいけど、VOICEVOX ENGINE的にはいまのとこ結構ありがた迷惑）
NPYルールはもう詳しくなるコストかけちゃったので、せっかくなので導入で良いと思いました！
NPY002以外は有用そうだし。
これ級のおせっかいルール･･･今回の例でもうちょっと言語化すると、そのドメインには有用だけどこっちのドメインでは手続きが増えるだけのものが、まあ結構あるだろうな～と感じました。
使ってるのが２箇所だから良いけど、これがいたるところに出てきたらたぶんデメリットのが大きい気がしました。
自分が把握しているだけで良いならむしろ適用されたほうが嬉しいんですが、新規コミッターが出くわしたらなんで？ってなりそうだよなぁと。
将来適当で良いランダム使う箇所が増えたらignoreすることになるかも。
あ。エラーではなく、自動修正されるとかならまぁ別に良いかもです。
There was a problem hiding this comment.
NumPy docs より:
This class should only be used if it is essential to have randoms that are identical to what would have been produced by previous versions of NumPy.
「This class should only be used」との記述からすると、deprecated ではないが特殊用途（どこでも使うのは非推奨）、が自然な解釈だと私は理解しました。
こっちのドメインでは手続きが増えるだけのもの
現時点では一理あると感じます。機能的に十分で、かつ、とても書き慣れた記法です。
ただ、新コードは既存コードの慣習を引きずりやすいため、「良い乱数がほしいな、あ、近くに書いてある np.random.rand 使えばいいか」で危険な乱数が導入される将来のリスクがあると考えます。
一方で、NPY002 導入によるコード増加量は default_rng(). という14文字だけです。
この14文字で「直感性が凄い落ちた」と感じるのは、私含めた、NumPy 1 のレガシーに慣れたプログラマーのみだと思います。NumPy 公式が述べている「This class should only be used」に沿ってコーディングしてきた NumPy 2 ネイティブは「乱数ってそうやって作るものでしょ？」と感じそうです。
今回に関しては、新記法にするのがトータルプラスだと考えます。
（似た議論が他でも今後ありそうなので）
ルールは結構おせっかいというか、思想強めな気が
ひとつの基準として、「公式が言っている推奨法をルール化したもの」「linter 作者の思想をルール化したもの」の分けるのがよさそうと今回のを見て感じました。
- 前者であれば基本従う
- 後者が多いルールは導入 NoGo 寄り（議論コストに見合わない可能性がそれなり）
とすれば、だいぶ議論コストを下げられそうです。
後者に関しては、PR 作成者が責任を持って導入メリットを述べた場合に限って議論すれば良さそうです。
There was a problem hiding this comment.
危険な乱数が導入される将来のリスクがあると考えます
この小さな差が致命的になるような機能が追加されることはあまり思いつかないですねー。
役立つのは統計を取るときとか、数億個くらいの乱数を高頻度に作るときとか、論文書くときくらいかなと。
NPY002 導入によるコード増加量は default_rng(). という14文字だけです。
文字数ではなく、どちらかというと「開発者が意味わからないエラーに遭遇してコミットを辞めたくなる」とかを懸念しています。
この辺ですかね。↓
これ単体なら大丈夫だと思いますが、ルールを導入しまくってこういうのが10個20個ある状況になるとだいぶしんどいと思います。
（僕はいっぱいあるうちの１つを適用する場合、こういう最終的にどうなりうるかを想像しながら導入すべきか考えてます。実は無駄なのかも。）
今回に関しては、新記法にするのがトータルプラスだと考えます。
今回は他のルールが優秀そうなので導入でOKだと思います。
「公式が言っている推奨法をルール化したもの」「linter 作者の思想をルール化したもの」
思想が強いという表現がちょっと良くなかったかもです。
今回のが微妙な理由は違ってて、ルールは目的のために導入するけど、その目的が自分たちにとって問題ではなく、かつルールによってデメリットが生じてる状況だと思います。
つまりまあ、どっちであるかはあまり関心がないかなぁと。
それでメリットがどれだけあり、デメリットがどれだけないかだと思います。
（僕はpython公式がl使うなと言ってるのも、フォントが変わった今これに従うのはデメリットのが大きいと感じます）
今後まだたぶんルール追加のすると思うので、今の個人的な感想をメモしておきます：
- 考え方があってないルールはたぶん今後結構ある
- 細かいデメリットよりメリットの量を考えると最終的な考えに近いところから始められる
- 恩恵がなくてデメリットだけあると感じるものは全てオフにする、みたいな指針もありかもしれない
There was a problem hiding this comment.
危険な乱数が導入される将来のリスクがあると考えます
これに関して念のため指摘しておきます。
そもそもこのモジュールはセキュリティや暗号化のためには設計されていないと書かれています。
https://numpy.org/doc/stable/reference/random/index.html
Warning
The pseudo-random number generators implemented in this module are designed for statistical modeling and simulation. They are not suitable for security or cryptographic purposes. See the secrets module from the standard library for such use cases.
そのためLegacy Generatorをdefault_rng置き換えてもセキュリティ用途に使っている限り危険であると思います。
|
たぶん大丈夫だと思うのでマージします！！ @sabonerune さんもありがとうございました！！ |
3 participants
（#1681 での議論が収束したため、レビュー可能になります。全体を大きく書き換えました。）
内容
NumPy のレガシー関数を置き換えて ruff
NPYを適用するリファクタリングを提案します。VOICEVOX ENGINE は数年間にわたり運用されてきたため、外部ライブラリのレガシーな関数等が残っている。
具体的には
np.random.randがレガシーであり、置き換えが必要である。また NumPy は大規模なバージョン移行が近年あったため、古い numpy に慣れたコントリビューターが手癖でレガシー関数を新たに使うリスクがある。
これは ruff
NPYルールで予防できる。#1646 の基準に従って整理すると、
NPYルール導入には現時点で以下の利点がある：np.random.randを置き換えまた将来的にも（上記に述べたように）バグ埋め込みの危険性を下げる。
NPYは 4 つのルールのみからなり、基本的に deprecated と lagacy のチェックであり、デメリットは無い。このような背景から、NumPy のレガシー関数を置き換えて ruff
NPYを適用するリファクタリングを提案します。関連 Issue
無し