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

スタイルID(style_id)のことを話者ID(speaker_id)としているコードを全部置き換える #741

Merged
merged 45 commits into from
Oct 22, 2023

Conversation

weweweok
Copy link
Contributor

@weweweok weweweok commented Sep 6, 2023

内容

同上

関連 Issue

ref #589

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

その他

分割してこのページにコミットします。(変更しないほうがいい箇所が見つけられなかったため)

変更予定ファイル一覧(style_idに変更しているファイルをチェック)

  • run.py
  • test_mock_synthesis_engine.py
  • test_synthesis_engine_base.py
  • test_synthesis_engine.py
  • cancellable_engine.py
  • morphing.py
  • mock.py
  • core_wrapper.py
  • synthesis_engine_base.py
  • synthesis_engine.py

@weweweok weweweok requested a review from a team as a code owner September 6, 2023 03:09
@weweweok weweweok requested review from y-chan and removed request for a team September 6, 2023 03:09
@Hiroshiba
Copy link
Member

Hiroshiba commented Sep 6, 2023

あ!
issueの方にちょこっと書いているのですが、互換性を維持しておきたいので、style_id/speaker_idともにOptionalにして受け取るのはどうでしょうか?

@weweweok
Copy link
Contributor Author

weweweok commented Sep 6, 2023

例えば以下の内容を

 def synthesis(
        self,
        query: AudioQuery,
        style_id: int,
        enable_interrogative_upspeak: bool = True,
    ) -> np.ndarray:
    def synthesis(
        self,
        query: AudioQuery,
        style_id: Optional[int,str], (または、style_id: int | str) <<変更予想箇所
        enable_interrogative_upspeak: bool = True,
    ) -> np.ndarray:

に変更するということでしょうか?
互換性に関して、よく理解していませんでした。m(_ _)m

@Hiroshiba
Copy link
Member

Hiroshiba commented Sep 6, 2023

あ、ちょっと惜しいです!
やりたいことはこんな感じです

  • 今まで通りspeaker_id引数が渡された場合でも動くけどワーニングが出る
  • 新しくstyle_id引数が渡された場合も動く
  • 両方指定された、あるいは両方指定されてない場合はエラー

こんな感じを想像しています(一部省略してます)

def synthesis(
	speaker_id: Optional[int],
	style_id: Optional[int],
):
	if "speaker_idとstyle_idが両方存在しない" or "speaker_idとstyle_idが両方存在しちゃう":
		error
	if speaker_id is not None:
		warning.warn("style_idに変わったのでそちらを使ってください的な文面")
		style_id = speaker_id
		speaker_id = None

非推奨パラメータにしてあげると更に良いかもです。
https://fastapi.tiangolo.com/ja/tutorial/query-params-str-validations/#_11

@Hiroshiba Hiroshiba requested review from Hiroshiba and removed request for y-chan October 9, 2023 15:30
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.

コメントにも書いたのですが、speaker_idではなくspeakerになっている箇所がかなりあることに気づきました 🙇
あと、互換性を持たせるべき箇所はWebAPIの箇所で、実際に互換性が持たせられているのが内部で使っているクラスになっていそうです。
この2点を加味して、頂いたプレイリストをどうしていただくべきかをちょっと迷っています。

方針は3つありそうです。

一つがWebAPIは書き換えずに内部で使っている変数を全て正しい方の表記に書き換えること。これは今のプルリクエストのうち、クラスに互換性にを持たせているところを元に戻しつつ、ソースコード全体で本来はstyle_idなのにspeakerとなっている部分も変更すれば完了だと思います。

2つ目の方法が↑プラスでWebAPIも書き換えて、互換性も持たせる方針です。

3つ目がとりあえずspeaker_idとなっていたものだけstyle_idに変える方法です。これは1の方法からspeakerとなっているものは一旦無視する方向になりそうです。

一番混乱がないのは2の方法ですが、一番何度が高いのも2だと思います。

run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
run.py Outdated
Comment on lines 337 to 339
accent_phrases = engine.replace_mora_data(
accent_phrases=accent_phrases, speaker_id=speaker
accent_phrases=accent_phrases, style_id=speaker
)
Copy link
Member

Choose a reason for hiding this comment

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

このプルリクエストを見て気づいたのですが、speaker_idではなくspeakerになっている箇所がかなりありますね!!
見逃していました、申し訳ない。。

Comment on lines 112 to 115
def is_initialized_speaker_synthesis(
self, style_id: Optional[int], speaker_id: Optional[int] = None
) -> bool:
"""
Copy link
Member

Choose a reason for hiding this comment

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

このクラスで互換性を持たせていますが、互換性を持たせるべきはWebAPIなのでずれてるかもです!
ここは戻していただく形になるかなと!

@Hiroshiba
Copy link
Member

互換性に関してちょっと認識がずれていそうだったので解説をしてみます。

そもそも互換性を入れないといけない理由は、既存ユーザーが使っているAPIをそのまま使えるようにするためです。
引数の名称を変えてしまうと、変更前のAPIの引数名でこのエンジンを叩くともちろんエラーが出てしまうと思います。
そうならないように、古い引数名でも正しく動くようにする必要があるという感じです。

そのWebAPIはrun.pyに色々定義されているので、それらに互換性を持たせていくタスクになります。
分からないことがあったら何でも聞いてみてください!

@weweweok
Copy link
Contributor Author

weweweok commented Oct 11, 2023

id_checker関数の指摘も頂いたので、そちらも修正しました。下記が、audio_queryパラメータで動くように調整したものを検証したスクショです。いかがでしょうか?

style_idのみの使用
image

speakerのみの使用。
image

speakerとstyle_id両方なし
image
image

speakerとstyle_id両方あり
image
image

APIドキュメント
image

@Hiroshiba
Copy link
Member

@weweweok スクショの内容を見るに実装良い感じなのかなと思いました!!
そのスクショから2点気づきがありました!

  • 500エラーが出ている
    • これはサーバー側が悪い(エンジン側が悪い)という意味
    • どちらかというとパラメータが不正なので400を返すのが良さそう
  • メッセージはspeaker_idではなくspeakerがより正しそう

何にせよ今のプルリクエストにプッシュしていただければと思います!

@weweweok
Copy link
Contributor Author

レビューを反映しました。

ここのspeaker_idですが、注釈を見る限り、intではないのでそのままにしておいたのですが、変更したほうが良いですか?

def yukarin_s_mock(length: int, phoneme_list: numpy.ndarray, speaker_id: numpy.ndarray):
    result = []
    # mockとしての適当な処理、特に意味はない
    for i in range(length):
        result.append(round(float(phoneme_list[i] * 0.0625 + speaker_id), 2))
    return numpy.array(result)

@Hiroshiba
Copy link
Member

Hiroshiba commented Oct 17, 2023

ゴール近そうですね!!

ここのspeaker_idですが、注釈を見る限り、intではないのでそのままにしておいたのですが、変更したほうが良いですか?

あ、本当ですね!!!
もしよかったらそちらも変えていただければ!!
ちょっと全部見れてないから分からないのですが、たぶんnumpy.ndarray的な型になっているspeaker_idspeakerstyle_idが正しそうです。
(ちなみにnumpy.ndarrayは配列的な型で、中にint値が入ってると思います。)

@weweweok
Copy link
Contributor Author

weweweok commented Oct 21, 2023

レビューを反映しました。

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.

良い感じだと思います!!
あとはmasterブランチをマージしてコンフリクトを解消していただければ!!

@weweweok
Copy link
Contributor Author

weweweok commented Oct 22, 2023

 ローカル側の操作にまだ慣れておらず、変更内容をpushできなかったため、コンフリクトの解消をgithub上で行いました。
 masterブランチの関数とcore_version引数の型ヒントが、自分のコミットした記述になかったことで、コンフリクトが発生していました。
 競合部分は、解消できたようなのですが、以下のような部分は、競合ではないため、注釈が追加されていません。これも修正すべきだと思いますが、別のプルリクエストで修正したいです。いかがでしょうか?

    @app.post("/initialize_style_id", status_code=204, tags=["その他"])
    def initialize_style_id(
        style_id: int,
        skip_reinit: bool = Query(  # noqa: B008
            False, description="既に初期化済みのスタイルの再初期化をスキップするかどうか"
        ),
        core_version: Optional[str] = None,   <<<この部分
    ):

@Hiroshiba
Copy link
Member

なるほどです!!
このままマージするとcore_version: str | None = None,になってるところとcore_version: Optional[str] = None,になっているところ2箇所が現れちゃうので、このプルリクエスト内で解決しちゃった方が良いかなと思います!
2行くらいですし。

1回通しで確認してみますが、おそらくもうそこ以外はマージできる形になっていると思います!
なので問題なかったらちょっとこっち側で勝手に変更させていただきますね!

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!!!!!

テストが落ちているのでそれも含めてこちらで直しちゃおうと思います!
何箇所かさらに変更が必要な場所がありそうだったので、そちらはissueにまとめておこうと思います。
もしよかったらプルリクエストに挑戦していただければ!!!

@github-actions
Copy link

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 477 323 coverage-32%
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 2228 702 coverage-68%

@Hiroshiba
Copy link
Member

テストが通ったのでマージします!

@Hiroshiba
Copy link
Member

Hiroshiba commented Oct 22, 2023

@weweweok 結構右往左往してしまって申し訳ありませんでした 🙇
しっかりとこちらのレビューに対して変更してくださってとても助かりました、ありがとうございます!!

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