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: /presets.yaml デフォルトファイルの廃止 #876

Closed
3 tasks done
tarepan opened this issue Dec 15, 2023 · 12 comments
Closed
3 tasks done

Refactor: /presets.yaml デフォルトファイルの廃止 #876

tarepan opened this issue Dec 15, 2023 · 12 comments
Assignees
Labels
機能向上 状態:実装 実装をおこなっている状態

Comments

@tarepan
Copy link
Contributor

tarepan commented Dec 15, 2023

内容

要望: /presets.yaml デフォルトファイルの廃止によるリファクタリング

現在の VOICEVOX ENGINE はAudioQueryプリセット機能を有する。
これを司る PresetManager では preset_path 引数を介してプリセットファイルを指定できる。
PresetManager は「デフォルト値・デフォルト保存先の指定」をこの preset_path 引数により実現している。
具体的なデフォルト値・保存先は /presets.yaml である。

この /presets.yaml はファイルであり、ビルドプロセスの中でファイルコピー等を必要とする。
デフォルト値 DI や保存先変更の観点からファイル方式は妥当な一方、ある程度のメンテコストも生じさせている。

ここで VOICEVOX ENGINE のプリセットデフォルトに関する要求を考えると、以下の2つが挙げられる:

  • デフォルト値は empty でよい。ただし指定があれば変更可能(DI)
  • presets.yaml をデフォルト保存先とする。ただし指定があれば変更可能

ここで、この2点であれば「presets.yaml の事後生成」でも達成できるとわかる。
すなわち preset_path="presets.yaml" かつ not Path("presets.yaml").exists() のとき、empty な presets.yaml をPythonで生成すればデフォルト /presets.yaml ファイル無しに上記の要件を満たせる。
これによりデフォルトファイルのメンテコストを削減できる。

このような背景から、 presets.yaml デフォルトファイルの廃止によるリファクタリングを提案します。

Pros 良くなる点

  • ビルドプロセス簡易化によるメンテコスト削減
  • PresetManager とデフォルト値の近接配置による見通し改善

Cons 悪くなる点

  • 無し

実現方法

  • preset_path="presets.yaml" and not Path("presets.yaml").exists() 時の empty YAML ファイル生成
  • /presets.yaml の削除
  • ビルドプロセスの簡略化

VOICEVOXのバージョン

0.14.10

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux
@github-actions github-actions bot added OS 依存:linux Linux に依存した現象 OS 依存:mac macOS に依存した現象 OS 依存:win Windows に依存した現象 labels Dec 15, 2023
@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 15, 2023

ファイルがある場合は(マルチエンジン作成などで)デフォルトのプリセットを作る時にAPIを叩かなくて良い、というメリットがなくはないですが、issue内に書かれている通りファイルがないことによるメリットがそこそこ大きいので、ユーザー辞書と同様にAPIでのみ作成可能にするという形にするのが良いのかなと思いました!!

機能を作ってくださった @takana-v さんのコメントもいただけると心強いです・・・!

@tarepan
Copy link
Contributor Author

tarepan commented Dec 15, 2023

デフォルトのプリセットを作る時にAPIを叩かなくて良い

なるほど、テンプレートファイルとしての機能ですね。

APIでのみ作成可能にするという形にするのが良いのかな

👍
直書きの是非(手書きミス誘発、validationするならAPIで作っても変わらない、など)も均衡していると思うので、ファイル無しメリットを優先して「APIへ集約」に賛成です。

@takana-v
Copy link
Member

昔はAPIが無く、presets.yamlファイルを直接編集することでのみプリセットを編集することができました。
この実装に依存している外部ソフトウェアがあるかもしれないので、変更のアナウンスは必要かと思います。
(恐らく無いとは思いますが...)

@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 17, 2023

変更のアナウンスは必要かと思います。
(恐らく無いとは思いますが...)

おそらく困るサードパーティアプリが1つあるかないかくらいかなと思っています!
エンジンリリース時のリリースノートに書いて案内します!

@Hiroshiba
Copy link
Member

あ。デフォルトファイルがあるのは、マルチエンジン対応のエンジンを作る時に少し便利かなと思いました!
こう、キャラクターごとにおすすめのプリセットをエンジン開発者側が用意できる、みたいな。
そう考えると、辞書のデフォルトファイル(default.csv)と同じ扱いにするほうがあっているかもです。
(なので消さない方がいいかも・・・?)


ちょっと話がずれますが。。
この辺りのエンジンごとのデフォルト値用のファイルは、ルート直下ではなくengine_manifest_assetへ移動する形が合っているのかなとか思いました。
ちょっとビジネスロジックにも絡んできてissueの内容と離れてしまいますが・・・。

@tarepan
Copy link
Contributor Author

tarepan commented Dec 18, 2023

デフォルトファイルがあるのは、マルチエンジン対応のエンジンを作る時に少し便利

👍
マルチエンジン開発者のユースケースが具体的に理解できました。確かに意義深そうです。

なので消さない方がいいかも・・・?

そして同時に、このユースケースであれば Python ファイルの方が扱いやすそうです。
デフォルト値作るためにはYAML文法知るべし、を避けられそうです(私もYAMLよくわからない)。
default_presets.py を作るイメージです。

デフォルト値用のファイルは、ルート直下ではなくengine_manifest_assetへ移動する形が合っている

👍
同意です。
別 issue として扱うのも同意です。

@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 19, 2023

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

そして同時に、このユースケースであれば Python ファイルの方が扱いやすそうです。
デフォルト値作るためにはYAML文法知るべし、を避けられそうです(私もYAMLよくわからない)。
default_presets.py を作るイメージです。

なるほどです! いろんなこと考えるとまあ文字列ファイルの方が良さそうかなと思いました!
(別言語で実装する場合、Pythonのバージョンが変わる場合、任意のコードが書けるのでセキュリティ上の懸念、など)

yamlが難しいというのは確かにと思いました。
今はプリセットは1個だけなので、コピペで作るときにリストの記法を調べないとダメになってますね。。
要素を2つにすればかなりマシになるかなと思いました!


@takana-v エンジンマニフェストにプリセットのデフォルト値ファイルを含めるの、設計的に何か懸念点とかありそうでしょうか・・・・・?(ちょっと不安)
辞書ファイルとかもこっちに含めちゃえるよな~と考えてます。

@takana-v
Copy link
Member

現状のエンジン側のプリセット機能がほぼ使われていないことを考えると、マルチエンジン対応のエンジン製作者にとっての利点も少ないのではと思います。
(せっかくプリセットのデフォルト値ファイルを用意しても使ってもらえないと思います。辞書は形態素解析を行う際にユーザーに使われますが...)
エンジンマニフェストにプリセットのデフォルト値ファイルを追加し、エディタ側から参照できるようになれば、便利だと思います。

また、プリセット周りは複雑であり、ユーザーが混乱することが考えられます。
(エンジンのプリセットを編集する導線が複数存在する・エディタとエンジンのプリセット機能は別である)

そのことを考えると、ルート直下のプリセットファイルを廃止し、setting.yml等と同様に、%localappdata%\voicevox-engine内に保存するようにしてもいいのかなと思いました。
(外部ソフトウェアの存在に留意する必要はあると思いますが...)


エンジンマニフェストの設計に関しては、あまり自信が無いので、 @aoirint さん辺りの意見を伺いたいです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 22, 2023

@takana-v ありがとうございます!!
確かに保存先がルート直下になってるのは僕も気になってました。破壊的変更になっちゃいますけど、一旦仕方ないということで変更しちゃった方が良さそうに感じました!
(まあissueの内容とは直接関係ないですが)

今のところの意見をまとめるとこんな感じですかね~

  • /presets.yamlのデフォルトファイルは廃止で良さそう
  • それはそれとして、engine_manifest内にpresetsのデフォルトファイルを作り直すのは良さそう
    • エディタ側から使ってもらえれば特に良い
    • マニフェスト周りはまだちょっと自信ない
  • それはそれとして、保存先ディレクトリを設定ファイルと同じとこにするのも良さそう

@tarepan tarepan added 状態:実装者募集 実装者を募集している状態 and removed OS 依存:mac macOS に依存した現象 OS 依存:linux Linux に依存した現象 OS 依存:win Windows に依存した現象 labels Mar 8, 2024
@tarepan
Copy link
Contributor Author

tarepan commented May 25, 2024

着手しました。

@Hiroshiba
Copy link
Member

がマージされたので、このissueは目標達成に思いました!
なのでクローズで良さそう!


少し話が変わりますが、presetsファイルのデフォルトの保存先ディレクトリがエンジンのディレクトリになっている関係で、特定条件で問題になることも分かりました。

これは

保存先ディレクトリを設定ファイルと同じとこにするのも良さそう

で解決できそうな気がしています。
ということでちょっとissue作ろうと思います!

@Hiroshiba
Copy link
Member

作りました!

ということでこちらのissueはクローズします、ありがとうございました!

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