追加: デフォルトプリセット機能を削除しプリセット保存先を指定する機能へ変更

[tarepan]

内容

presets.yaml を削除するリファクタリングを提案します。

機能上の変化はありません。ビルド・GET /presets・POST /add_preset の正常機能を確認済みです。

関連 Issue

step1 of #876
resolve #1243 (final step 🎉)

[Hiroshiba]

あっどうしようかな･･･

プリセットのサンプルの書き方を示す目的も兼ねていました。
が、そもそも最初からサンプルが入ってるのも良くない気がしてきました。

すみません、ちょっと提案です 🙇
presets.yamlをなくし、かつサンプルをなくす方針はどうでしょうか･･･？

代わりにドキュメントでサンプルを案内するか、あるいはデータ構造はこちらですとPreset用のモデルを案内するか･･･

サンプルプリセットが入ったyamlがなくなることは別に良いのですが、それがコード内にのみ現れるのは意図と異なるので、一旦コメントお返しします 🙇

[tarepan]

かつサンプルをなくす方針

👍️
削除します。

代わりにドキュメントでサンプルを案内するか、あるいはデータ構造はこちらですとPreset用のモデルを案内するか

前提として、POST /add_preset でプリセットが追加可能となりました。
ゆえに他者に配布する（マルチエンジン実装者が作成する）プリセットファイルは手書きでなく API 経由で作成したうえで配布されるのが好ましいと考えます（手書きはバグのもと）。
よってサンプルは完全削除・追加の案内無しで良いと考えます。

---

@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re-review よろしくお願いします。

[Hiroshiba]

前提として、POST /add_preset でプリセットが追加可能となりました。
ゆえに他者に配布する（マルチエンジン実装者が作成する）プリセットファイルは手書きでなく API 経由で作成したうえで配布されるのが好ましいと考えます（手書きはバグのもと）。
よってサンプルは完全削除・追加の案内無しで良いと考えます。

なるほどです。
まあマルチエンジン開発者とかはそうでもないので案内しても良さそうな気がしますが、一旦別に良い気がしました！

[tarepan]

@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re²-review よろしくお願いします。

[tarepan]

@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re³-review よろしくお願いします。

[Hiroshiba]

ゴールとは違う形の中間状態になってる事に気づきました！

やってることは「presets.yamlを削除する」ですが、変わっていることをまとめると

• presets.yamlからデフォルトファイルとしての役割をなくす
• でも引数等で与えたものはデフォルトファイルとしての役割が残ってる

という状態になり、これは最終ゴールの形と違いそうです。
ゴールはたぶん、引数で与えたものもデフォルトファイルとしての役割をなくす形。

おそらくそこまで進めても行数はほぼ変わらないので、そこまで進めちゃうのはどうでしょう。
「どんな場合でもプリセット保存先にファイルがなかったらemptyで作る」にすればOKだと思います。
むしろ変更行数は減りそう？（今+50 −28）

ここまで最初のレビューで気づければよかったです･･･すみません。。

[tarepan]

最終ゴールの形と違いそう

最終ゴールの形は #876 で議論されており、まだ形の同意が取れていないと認識しています。
ただ議論の途中でも「ひとまず機能変更無しで presets.yaml は消せるね = リファクタリングできるね」との同意が得られたため、本 PR を step 1 として出しています。

引数で与えたものもデフォルトファイルとしての役割をなくす形 は破壊的変更であり、いくつか懸念点もあります。
本 PR による中間状態は（全体最適が崩れることなく）単純にリファクタリングが進んで見通しが改善しているため、一旦 review & merge する方向が良いと考えます。

[Hiroshiba]

あ、リファクタリングではないと思ってます。
デフォルト値をプリセットファイルに書けるという仕様がなくなったので、すでに破壊的に変更してるはずです。

変更量が少ないなら、一気に変更できた方がメンテナーとしてはだいぶ安心感が違います。
一気に変更だと「コードの良さ・正しさ・仕様変更確認」を１回ずつだけど、中間状態が入ると「コードの良さ２回・正しさ２回・中間状態を把握・仕様変更確認（中間状態がちゃんと解かれてるかも確認）」になる感じです。

コードの変更量が多い場合はコンフリクトが発生しやすくなるのと、流石にレビューが難しいので、中間状態で一旦マージするのにメリットがあると思います。
今回の場合はむしろ一気に変更した方が変更量少ないし、すでに仕様変更なので２回仕様変更になってしまうしで、一気のがかなり楽そうです。

[tarepan]

リファクタリングではないと思ってます

👍️
認識を理解しました。

---

最終形（本 PR で実装予定）についての認識合わせをさせてください。

ゴールはたぶん、引数で与えたものもデフォルトファイルとしての役割をなくす形。

プリセット設定（--preset_file CLI 引数）は「プリセット保存先の指定」という意味になる、で認識あっているでしょうか？

[Hiroshiba]

プリセット設定（--preset_file CLI 引数）は「プリセット保存先の指定」という意味になる、で認識あっているでしょうか？

あ、そういう認識です！
たぶん既存のsetting_fileと同じ挙動になるんじゃないかなーと思ってます。

[tarepan]

@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re⁴-review よろしくお願いします。

[Hiroshiba]

LGTM！！

実装ありがとうございます！！

･･･あ、もしかして消えたのはpresets.yamlファイルであって、デフォルトプリセット機能は消えてない･･･？
ファイルパスが偶然以前と同じなので。
なのでPRタイトルが気になったけど、まあそういう方針にしていきましょうということでデフォルトプリセットファイルを消しました、という感じで良さそう！
（後でいろいろ忘れそうなのでメモコメント）

---

@takana-v 共有です 🙏

一旦、デフォルトのプリセットをpresets.yamlに書ける機能をサポートしない方針になった感じです。
気になる点あればコメントいただけると･･･！！

[takana-v]

メモ：Windowsで全てのユーザー向けにインストールした場合、preset_pathに対する書き込み権限が無く、FastAPIが立ち上がる前にエンジンが落ちる気がします

• REST API : post /add_preset が Internal Server Errorを返す #1478

[Hiroshiba]

たしかに！！！

プリセット先はインストール先と関係ないローカルディレクトリにした方が良さそうですね･･･！！

[Hiroshiba]

遅くなりました、マージしたいと思います！

📝 プリセット保存先はデフォルトでエンジンディレクトリの直下のまま。