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

プリセットファイルのパスを起動引数・環境変数で変更できるようにする #711

Merged
merged 19 commits into from
Oct 9, 2023

Conversation

aoirint
Copy link
Member

@aoirint aoirint commented Jun 24, 2023

内容

を実装します(エンジン側のプリセット機能に関する変更です。エディタではまだエンジン側プリセットAPIの呼び出しは未実装だと思います)。

実装方法には、generate_apppreset_pathを渡す方法、PresetManagerを渡す方法があると思いますが、SettingLoaderに合わせて後者にしました。

環境変数 VV_PRESET_FILE の命名は、MySQL DockerイメージのMYSQL_ROOT_PASSWORD_FILEや、Wordpress DockerイメージのWORDPRESS_DB_PASSWORD_FILEを参考にしています(パスを指定する場合、末尾が_FILE)。

未解決の課題

個別にIssueを作った方がいいですが、レビューに関係しそうなので気づいた点を書くだけ書いておきます(レビューに関係しなさそうなら、いったんスルーしてもらって大丈夫です)。

  • make_docs.pyでMockPresetManagerを使うようにしたい
    • ドキュメントを生成するためにプリセットファイルはいらない(はず)ので、実際のプリセットファイルのパスを指定する必要はない
    • このPRでは、暫定的にengine_root() / presets.yamlを使っている
      • USER_SETTING_PATHでは--voicevox_dirを参照しないが、プリセット(preset_path)の場合は参照するため、動作を維持するなら定数化できない
    • PresetManagerのMockまたはStubを作成して使うようにしたい
    • SettingLoaderも同じく、MockSettingLoaderを作成して使うようにしたい
  • 起動時にプリセットファイルが存在すること、破損していないことを検証したい
    • 現在の挙動
      • おそらくエンジンが起動しないケースを減らすため、プリセットファイルが存在しない場合でもAPIが起動するようになっている
      • この場合、プリセット系のAPIを呼び出すとHTTP 422が返る
    • APIの起動は維持しつつ、バリデーションをして、APIを呼び出す前にエンジンを直接使うユーザが気づけるようにしたい
    • または、検証に失敗した場合、エラー終了させる引数を追加したい
  • 設定ファイルsetting.ymlとプリセットファイルpresets.yamlでファイル名の表記揺れを修正したい(単数複数形?・拡張子)
    • YAMLファイルの拡張子はどちらかに統一したい
    • 実行時リネームはコンテナ実行時にマウントしたファイルが消失する可能性があるため避けたい
  • generate_appからroot_dir引数やengine_root()呼び出しを削除したい
    • generate_approot_dirOptional[Path]になっているのは、make_docs.pyからgenerate_appを使用するため?
    • API内のいろいろなところでroot_direngine_root()が使われているので、make_docs.pyが実際には使用しないファイルに依存している
    • SettingLoaderや(このPRの変更による)PresetManagerのように、Mockを差し込める形で(ファイルシステムに依存しない形で)generate_appを呼び出せるように変更したい

これらの課題に関連するIssue

関連 Issue

スクリーンショット・動画など

その他

@github-actions
Copy link

github-actions bot commented Jun 24, 2023

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 444 297 coverage-33%
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/acoustic_feature_extractor.py 75 0 coverage-100%
voicevox_engine/cancellable_engine.py 85 66 coverage-22%
voicevox_engine/dev/core/init.py 2 0 coverage-100%
voicevox_engine/dev/core/mock.py 27 12 coverage-56%
voicevox_engine/dev/synthesis_engine/init.py 2 0 coverage-100%
voicevox_engine/dev/synthesis_engine/mock.py 36 2 coverage-94%
voicevox_engine/downloadable_library.py 93 5 coverage-95%
voicevox_engine/engine_manifest/EngineManifest.py 34 0 coverage-100%
voicevox_engine/engine_manifest/EngineManifestLoader.py 12 0 coverage-100%
voicevox_engine/engine_manifest/init.py 3 0 coverage-100%
voicevox_engine/full_context_label.py 162 3 coverage-98%
voicevox_engine/kana_parser.py 86 1 coverage-99%
voicevox_engine/metas/Metas.py 33 0 coverage-100%
voicevox_engine/metas/MetasStore.py 29 12 coverage-59%
voicevox_engine/metas/init.py 2 0 coverage-100%
voicevox_engine/model.py 160 9 coverage-94%
voicevox_engine/mora_list.py 4 0 coverage-100%
voicevox_engine/morphing.py 70 46 coverage-34%
voicevox_engine/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/preset/Preset.py 12 0 coverage-100%
voicevox_engine/preset/PresetError.py 2 0 coverage-100%
voicevox_engine/preset/PresetManager.py 81 2 coverage-98%
voicevox_engine/preset/init.py 4 0 coverage-100%
voicevox_engine/setting/Setting.py 11 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 19 0 coverage-100%
voicevox_engine/setting/init.py 3 0 coverage-100%
voicevox_engine/synthesis_engine/init.py 5 0 coverage-100%
voicevox_engine/synthesis_engine/core_wrapper.py 201 146 coverage-27%
voicevox_engine/synthesis_engine/make_synthesis_engines.py 59 30 coverage-49%
voicevox_engine/synthesis_engine/synthesis_engine.py 130 11 coverage-92%
voicevox_engine/synthesis_engine/synthesis_engine_base.py 67 9 coverage-87%
voicevox_engine/user_dict.py 144 11 coverage-92%
voicevox_engine/utility/init.py 5 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/core_version_utility.py 8 1 coverage-88%
voicevox_engine/utility/mutex_utility.py 10 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 8 coverage-69%
TOTAL 2189 671 coverage-69%

run.py Show resolved Hide resolved
@Hiroshiba
Copy link
Member

@aoirint 気になっている点のまとめありがとうございます、とても参考になります!!

make_docs.pyでMockPresetManagerを使うようにしたい

モックを挿せるようにするのは良い方針だと思います!!
もしコードの複雑度がだいぶ上がりそうなら再考したいかもです。

起動時にプリセットファイルが存在すること、破損していないことを検証したい

良いと思います!!
ちゃんとコード読めてないですが、PresetManagerのインスタンス化のタイミングあたりで確認してあげると良さそうかも。
これは仕様の破壊的変更になってしまうので、エラー終了させる引数を追加は必須になっちゃいそうです。

SettingMangerも同じ仕組みにできると良いかもですね!

設定ファイルsetting.ymlとプリセットファイルpresets.yamlでファイル名の表記揺れを修正したい

なるほどです。
ひとかたまりの設定をsettingとするなら単数が正しく、プリセットは複数保存されるのでpresetsのように複数形のが正しいかも?

・・・が、なんか統一されてないのは若干違和感ある気がしますね・・・。
とりあえず一理あるとして一旦置いといて、↓の破壊的変更時一覧に書き加えておき、変えるか再検討するのはどうでしょう。

generate_appからroot_dir引数やengine_root()呼び出しを削除したい

同意です!
engine_root()呼び出しはなんとかなるかもですが、root_dir引数はPresetとSettingの他にSpeakerInfosでも使われてることに留意が必要かもです。


これら、root_dirを引きはがすissueとして作っちゃって、 #501 を参照する形にすると良いかもと思いました!!
変更が100行くらいならいきなりPRでも全然問題ない気がします。PRを何回かに分ける or PR作りたいわけじゃない感じなら、1回issueにまとめておくと迷子になりにくいかも!

@aoirint aoirint marked this pull request as ready for review October 1, 2023 15:35
@aoirint aoirint requested a review from a team as a code owner October 1, 2023 15:35
@aoirint aoirint requested review from Hiroshiba and removed request for a team October 1, 2023 15:35
@aoirint
Copy link
Member Author

aoirint commented Oct 1, 2023

課題点について

確認ありがとうございます。いったんIssue化してみます。

このPRについて

SwaggerからプリセットのAPIを叩いて、起動引数・環境変数それぞれでファイルが切り替わることを確認したので、Ready for Reviewにします。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ほぼLGTMです!!
helpの部分だけかなと!

run.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
run.py Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

@Hiroshiba Hiroshiba merged commit a398d3b into VOICEVOX:master Oct 9, 2023
3 checks passed
@aoirint aoirint deleted the patch-preset-file-arg branch October 9, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

プリセットファイルのパスを起動引数および環境変数で変更できるようにしたい
2 participants