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

CancellableEngineが必要な引数だけを受け取るようにする #762

Merged
merged 24 commits into from
Oct 16, 2023

Conversation

aoirint
Copy link
Member

@aoirint aoirint commented Oct 9, 2023

内容

CancellableEngineは、run.pyで定義されたargparse.ArgumentParserparse_args()したargparse.Namespaceを初期化時に受け取っていました。

argparse.Namespaceにはメンバ情報・型情報がなく、起動引数(args)が変更されてもCancellableEngine側では気づけない状態だったため、CancellableEngineが必要な引数だけを受け取るようにしました。

Cancellable Engineを有効化するコマンド例(Windows)

poetry run python run.py --voicevox_dir "$HOME/AppData/Local/Programs/VOICEVOX" --enable_cancellable_synthesis --init_processes 2

Cancellable Synthesisを実行するコマンド例

echo -n "こんにちは、音声合成の世界へようこそ" >text.txt

curl -s \
    -X POST \
    "127.0.0.1:50021/audio_query?speaker=1"\
    --get --data-urlencode [email protected] \
    > query.json

curl -s \
    -H "Content-Type: application/json" \
    -X POST \
    -d @query.json \
    -o audio.wav \
    "127.0.0.1:50021/cancellable_synthesis?speaker=1"

関連 Issue

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

その他

CancellableEngine内でのenable_cancellable_synthesisのチェックを削除

テストを作りにくくなるため削除しました。

リファクタリング: run.py: root_dirの定義コード変更

main関数におけるroot_dir変数は、args.voicevox_dirをもとに定義されます。
make_synthesis_engines()CancellableEngine()args.voicevox_dirを渡すため、main関数にargs.voicevox_dirに型ヒントをつけたvoicevox_dir変数を追加しました。

root_dir変数の定義でこれに対応するコードはいくつか考えられますが、voicevox_dir変数を参照しつつ型を読み取りやすくなるように変更しました。

現状維持

args.voicevox_dirから2回別々に読み取るコードになり、起動引数の変更に対応しにくい。

root_dir: Path | None = args.voicevox_dir
if root_dir is None:
    root_dir = engine_root()

voicevox_dir変数を参照 + 三項演算子

直前に https://github.com/VOICEVOX/voicevox_engine/pull/711/files#r1349680805 でリファクタリングとしてifブロックに統一しており、手戻りになる。
voicevox_dir変数を参照すればAny型が関与しないため、三項演算子でもいいかなとは思います。

root_dir = voicevox_dir if voicevox_dir is not None engine_root()
# または
root_dir: Path = voicevox_dir if voicevox_dir is not None engine_root()

(PR作成時点で採用)voicevox_dir変数を参照 + ifブロック

変更量が少なく済む。

root_dir = voicevox_dir
if root_dir is None:
    root_dir = engine_root()

# または

root_dir: Path | None = voicevox_dir
if root_dir is None:
    root_dir = engine_root()

voicevox_dir変数を参照 + ifブロック + root_dirを初期化せずに型ヒントだけ付ける

root_dirnot Noneなことはこちらの方が伝わりやすそうですが長くなるので、第2候補くらいで悩む。

root_dir: Path
if voicevox_dir is not None:
    root_dir = voicevox_dir
else:
    root_dir = engine_root()

リファクタリング: 型ヒントの更新

変更した部分の周りの型ヒントを新しい記法に移行しました。

型ヒントmultiprocessing.connection.Connection_ConnectionBaseに変更

multiprocessing.Pipeの返すtupleの中身の型(tuple[T, T]T)に対する型ヒントを変更しました。

tupleの中身の型は、Linux/macOSではmultiprocessing.connection.Connectionですが、Windowsではmultiprocessing.connection.PipeConnectionになっています(Python 3.11の該当ソースコード@2bad6e7)。

ConnectionとPipeConnectionはいずれも互いを継承しておらず、Windowsでは型が不一致(mypyの型エラー)になります。
また、Linux/macOSではPipeConnection型が定義されない(Python 3.11の該当ソースコード@2bad6e7)ため、ユニオン型(Connection | PipeConnection)にすることもできません(自前でうまく分岐を書けばできるかもですが)。

これらの型は共通の基底クラス_ConnectionBaseを持つため、頭にアンダースコアがついてはいますが、型エラーを回避するため、型ヒントをmultiprocessing.connection._ConnectionBaseに変更しました。

頭にアンダースコア1つがついた名前は、Pythonの仕様としてアクセス禁止が定義されてはいなさそうですが、慣習として非publicなものとされていて、今後Pythonのマイナーバージョンアップで実装が変わる可能性がある点に注意が必要です。
アンダースコアが付いていない型が今後追加されることを期待して、import asで名前を変えています。

https://docs.python.org/ja/3/tutorial/classes.html#private-variables

アンダースコアで始まる名前 (例えば _spam) は、 (関数であれメソッドであれデータメンバであれ) 非 public なAPIとして扱います。これらは、予告なく変更されるかもしれない実装の詳細として扱われるべきです。

https://peps.python.org/pep-0008/#descriptive-naming-styles

_single_leading_underscore: weak “internal use” indicator. E.g. from M import * does not import objects whose names start with an underscore.

args.init_processesの取り出し

if enable_cancellable_synthesis:の中でargsから取り出してもいいかもですが、enable_cancellable_synthesisの値に関わらずnot Noneなことが伝わるので、ifブロックの外で取り出しています。

argsに型付けする部分はparse_args()のすぐ下に全部移動させて、make_synthesis_engines()SettingLoader()のようなクラスの初期化はその下か別の関数に移動させるリファクタリングを別PRでしてもいいかなとちょっと思いました)

@aoirint aoirint requested a review from a team as a code owner October 9, 2023 07:08
@aoirint aoirint requested review from Hiroshiba and removed request for a team October 9, 2023 07:08
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 453 305 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 91 71 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 2204 684 coverage-69%

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.

いろんなリファクタリングが含まれていますが、ほとんど全部賛成寄りです!
1点だけどっちの方が良いかわからなかったのでコメントしてみました。

voicevox_engine/cancellable_engine.py Outdated Show resolved Hide resolved
voicevox_engine/cancellable_engine.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なのですが、ペンディング状態になってるのだけ解決方向だけ決めちゃえると助かります!

voicevox_engine/cancellable_engine.py Show resolved Hide resolved
voicevox_engine/cancellable_engine.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!!!

また1段階コードが綺麗になったと思います、リファクタリング本当にありがとうございます!!!
もしまたよければ是非・・・!!

@Hiroshiba Hiroshiba merged commit 758b670 into VOICEVOX:master Oct 16, 2023
3 checks passed
@aoirint aoirint deleted the patch-cancellable_engine_type_hint branch October 16, 2023 16:32
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