cors policyにallを指定してdockerコンテナを起動したが、/setting画面では「現在値:localapps」となっている

[tomoish]

不具合の内容

cors policy modeをallとしてdockerコンテナを起動したが、/setting画面に行くと、CORS Policy Modeの欄に「現在値:localapps」と表示される。

実際には、cors policyはallとなっており、表示のみが誤っている。

再現手順

1. voicevox engineの起動

docker pull voicevox/voicevox_engine:cpu-ubuntu20.04-latest
docker run --rm -p '127.0.0.1:50021:50021' voicevox/voicevox_engine:cpu-ubuntu20.04-latest gosu user /opt/python/bin/python3 ./run.py --voicelib_dir /opt/voicevox_core/ --runtime_dir /opt/onnxruntime/lib --host 0.0.0.0 --cors_policy_mode all

2. http://0.0.0.0:50021/setting にアクセスすると、--cors_policy_mode allで起動したのに現在値がlocalappsとなっている

期待動作

引数--cors_policy_mode allをつけて起動すると、/setting 画面において、「現在値: all」と表示される。

解決方法

run.py内のsetting_get関数において、settings = setting_loader.load_setting_file()のように、表示されるcorsなどの設定値は設定ファイルから取得している。
しかし、起動時に呼ばれるgenerate_appでは最初に指定した設定値を設定ファイルに書き込まないので、表示が異なっている。
そのため、generate_appにおいて設定値をファイルに保存すれば解決すると考える。

VOICEVOXのバージョン

latest master branch

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

• Windows
• macOS
• Linux ubuntu20.04

その他

私はこのバグ解決に取り組みたいと考えています。

[sabonerune]

起動時の引数で保存した設定を変えてしまうというのはあまりよくない気がします。

根本的な問題は設定値には現在適応されている設定値と設定ファイルに書き込まれている設定値の二種類があるにも関わらず現在値:という表示1つしかないので

• 現在値:がどちらを示しているのか分からない
• 現在適応されている設定値をAPIから知る手段がない

という点な気がします。

(後者は必要かどうか分からないが)

[tomoish]

@sabonerune さん、コメントありがとうございます。

現状、run.pyの1530 - 1542行目において、

    setting_loader = SettingLoader(args.setting_file)

    settings = setting_loader.load_setting_file()

    cors_policy_mode: CorsPolicyMode | None = args.cors_policy_mode
    if cors_policy_mode is None:
        cors_policy_mode = settings.cors_policy_mode

    allow_origin = None
    if args.allow_origin is not None:
        allow_origin = args.allow_origin
    elif settings.allow_origin is not None:
        allow_origin = settings.allow_origin.split(" ")

となっており、もし設定ファイルを指定したとしても設定値は引数で指定したものが優先されます。これにより、仰る通り現在適応されている設定値と設定ファイルに書き込まれている設定値(もし存在しない場合は設定ファイル読み込み時にデフォルトを返す)が共存する状態になっています。
引数による設定が優先されるため、設定ファイルが存在したとしても、設定値を設定ファイルに上書きしても問題ないと考えました。

[Hiroshiba]

issue作成ありがとうございます！！
ここにUX上の問題があることを把握できていませんでした、とても助かります！！

仰るとおり、引数指定すると保存された設定ファイルよりも優先されます。
ただ、引数指定したもので設定ファイルを上書きしたいかどうかは、ユーザー次第だと感じています。例えば、一時的にそのパラメータ値にしたいだけかもしれません。

また仮に引数の値で設定ファイルを上書きした場合、実際には引数が優先されるのに、設定UIでパラメータを変更できたように見えてしまう別の問題が生じそうです。

解決策の方針は２つ考えられると思います。

1. 引数や環境変数など、設定ファイルより強い指定があった場合、設定UI上でそうとわかるように表示し、変更不可にする
   • 例えば「引数で指定されています」などと案内しつつUIをdisableにするなど
   • 「引数が無指定である」ことがわかるようにする改変が必要で、そこそこ大変
2. 設定UIでは設定ファイルの保存値を変える今の仕様のままにし、引数や環境変数など強い指定があっても表示に反映されていないことを明示する
   • 簡単だけど、どう案内すれば良いか不明

また意見うかがえると！！

[tomoish]

@Hiroshibaさん、コメントと解決策のご提案ありがとうございます。

設定ファイルを引数で変えたくないという需要があること、理解いたしました。

ご提案につきまして、以下のことを考えました。

1. 実行時引数の強い指定を判定する方法に関して
   1-1.
   設定ファイルに、この設定が反映されているかどうかの項目を追加する。(しかし、これはsettingに関する大部分で変更を要する。)
   1-2.
   引数で設定が指定されている場合、既存の設定ファイルとは異なるファイル(以下、引数ファイルと呼称)を作成することで、引数が指定されたかどうかを判定する。(エンジン再起動時に以前の引数ファイルは一度削除してから再度作成することで1実行につき1つの引数ファイルを参照できる)
   これは1-1と異なり、引数ファイルの読み込み書き込みに関するSettingLoaderクラスの変更で十分だと思われる。
   引数ファイルが存在する場合、/settingのページにおいて引数ファイルを用いて現在の設定を表示する。また、「ここで変更できるのは設定ファイルであり、実際の設定は実行時引数(--cors_policy_mode, --allow_origin)が優先される」という注意書きを追加する。
2. /settingページにおいて、「--cors_policy_mode, --allow_originを指定した時は、このページで変更できません」と案内する。また、run.py --helpにおいて、--cors_policy_mode, --allow_originが設定ファイルよりも優先されることを案内する。以上の2点の案内が必要だと考えます。

現状、--cors_policy_mode, --allow_originの引数が設定ファイルよりも優先されることが明記されておりませんので、上記2の後半は必要であると考えます。

また、1-2のように、引数ファイルを作成することで@saboneruneが指摘されていた、
「
現在値:がどちらを示しているのか分からない
現在適応されている設定値をAPIから知る手段がない
」
を解決できそうだと思います。

ご意見よろしくお願いします。

[Hiroshiba]

ご意見よろしくお願いします。

承知しました！

1-2.
引数で設定が指定されている場合、既存の設定ファイルとは異なるファイル(以下、引数ファイルと呼称)を作成することで、引数が指定されたかどうかを判定する。(エンジン再起動時に以前の引数ファイルは一度削除してから再度作成することで1実行につき1つの引数ファイルを参照できる)

一度ファイルに書き出すと、そのファイルがプログラムの外から勝手に変えられてしまう可能性を考慮しないといけないので、いろんな可能性を考慮する必要があるという短所が目立ってしまうと思います。
また結局SettingLoaderクラスの書き換えが必要なので、大変さは1-1と同じくらいかそれ以上に感じます。
あと引数ファイルが存在する場合、/settingのページにおいて引数ファイルを用いて現在の設定を表示する。また、「ここで変更できるのは設定ファイルであり、実際の設定は実行時引数(--cors_policy_mode, --allow_origin)が優先される」という注意書きを追加する。というのは、1-1でも同じ手が使えるなと思いました（というより2の項目に近い）。
上述３つの理由から、やるならファイルを経由しない方式が良さそうかなと感じました！

2
/settingページにおいて、「--cors_policy_mode, --allow_originを指定した時は、このページで変更できません」と案内する。また、run.py --helpにおいて、--cors_policy_mode, --allow_originが設定ファイルよりも優先されることを案内する。以上の2点の案内が必要だと考えます。

ヘルプに追加するの、良いですね！！

1系は大変なので、とりあえず2をやるのが良さそうだと思うのですが、いかがでしょうか？

[tomoish]

@Hiroshibaさん、ご意見ありがとうございます。

ファイルに書き出すことの欠点、理解しました。

仰る通り、2の案内系の方が取り組みやすいと感じておりますので、これから行いたいと思います。
その後、1に関して引数による設定とファイルによる設定を判別できるような実装を考えようと思います。

[Hiroshiba]

@tomoish おお、ぜひぜひ！！
不明な点あればなんでもお聞きいただければ！！

[tomoish]

@Hiroshibaさん、設定が実行時引数で指定されているかどうかによって/settingのUI画面を変更するという課題についてです。
ファイルを介さず行う場合を考えておりまして、

1. generate_appに新しくcors_policy_mode, allow_originのいずれかが実行時引数で設定されているかどうかを表す引数を作る
2. @app.get("/setting")でこの新しい変数によって、設定ファイルより強い指定があるかどうかでui.htmlを変更する
   というの考えておりました。

このような方針に関して、ご意見を伺えると幸いです。
よろしくお願いします。

[Hiroshiba]

なるほどです！
より強い指定が１つ以上ある場合は、設定の変更を全部できなくする、みたいな感じでしょうか？

将来もっと変更可能な設定が増えたときに、「この値は引数指定しつつ、この値は変更したい」となったときに少し不便かもです…！
メリットデメリットがある実装になるなと感じました！

[tomoish]

@Hiroshibaさん、返信ありがとうございます。
実行時引数による強い指定があったときは、/settingページで「ここで変更しても実行次引数の方が強い」ことをREADMEにも書いたように案内するというものをイメージしておりました。

具体的には、実行時引数があるときに/settingでは以下の挙動となることを考えています。

1. main関数内部でenumやdictを用いて設定ファイルにある設定が実行時引数で上書きされているかを保持する。(enumやdictで管理することで、cors_policy_mode, allow_originどちらかが指定されているのか判別でき、仰っていただいたような新しい設定引数が追加されても管理できると思います。)
2. generate_appの引数を追加して1.の内容を渡す。
3. /settingが呼ばれた時に、1.の引数から設定が上書きされているかを判定し、上書きされているものについては、「このページで設定を変更しても、実行時引数が優先されるため、同じコマンドで実行した場合はこの変更が反映されない」旨を表示する。

以上のようにすることで、設定ファイルの変更は許しつつも、実行時に適用されるかどうかを案内することができると考えております。
よろしくお願いいたします。

[tarepan]

@Hiroshiba
レビュアー側でキャッチボールが止まっています。
上記の実装方針に関して Go/NoGo 判断があると進行しやすそうです。

[Hiroshiba]

ちょっとかなり迷っています！
迷っているのは実装が複雑になるためです。
上がる複雑性について意見を聞けるとありがたいかもです。

現在run.pyの設定は、起動時引数で設定できるもの・環境変数でも設定できるもの・設定ページでも設定できるものが混じっています。
複数の方法で設定された場合、前者ほど優先的に使われます。
このあたりはなかなかややこしいのですが、一応現状はmain()関数内で完結しています。

提案は「設定ページでも設定できるもの」２種に対し、より優先度の高い指定方法があった場合に設定ページでそれを案内することです。
これはその２種をgenerate_app内でも特別扱いすることになり、コードの煩雑性が広がります。
この２種だけでなく、他全ての設定値をうまいこと共通化してから、その共通概念に対して追加実装するのであれば煩雑性は上がらないと思うのですが、その妥当な実装をうまく提案できずにいた感じです。

ちょっと実装に時間はかかってしまうかもなのですが、まず設定値をうまく構造化する部分から進められると良いのかも･･･？
環境変数での指定値、起動時引数での指定値、設定ページでの指定値を持てるようなgenericクラスを作る、とか･･･。

僕のリソースが出せないでいるので、もし可能であれば @tarepan さんに引き継いでいただけるととても助かるかもです 🙇

[tomoish]

@Hiroshibaさん、ご提案ありがとうございます。

提案は「設定ページでも設定できるもの」２種に対し、より優先度の高い指定方法があった場合に設定ページでそれを案内することです。

まず、ご提案いただいた内容について、設定ページで設定できるものより優先度の高い方法は実行時引数のみであります。
また、#1102 の解決により、サーバー起動時の設定を(この設定がファイルによるものか引数によるものかに関わらず)簡単に取得することができるようになりました。
そのため、例えば以下の画像のように、現在の適用値と/settingページでの設定が設定ファイルに行うものであるということを注意書きすることで、実装を複雑にせずユーザーに案内できる可能性があると考えております。

@tarepanさん含め、ご意見いただけるとありがたいです。

[tarepan]

本 issue で報告されたバグ「表示のみが誤っている」の修正として「現在の適用値と ... 注意書き」を追加する、という方針と認識しました。

@tomoish
この方針に同意です。
注意書きの位置も適切であり、これで実装 Go と考えます。
細かい文言 & 適用値位置は review 時に調整するとして、この形で PR を是非頂ければと思います。

[Hiroshiba]

@tarepan @tomoish
「引数aとbは⚪︎⚪︎という特徴があります」という案内は、その案内を更新し忘れるというリスクもあります。
そこ注意してメンテナンスできそうであればok、難しそうであれば文言の調整が必要かもです。

気になったのはそこだけです…！
方針は良さそうかなと！

[github-actions]

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