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

APIのstyle_idの型をStyleId型に #966

Merged
merged 9 commits into from
Jan 3, 2024

Conversation

Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Jan 2, 2024

内容

の続きです。
タイトルの通りで、ついでにSpeakerStyle.idやPreset.style_idもStyleId型にしています。

その他

よかったら @tarepan さんレビューいただけると・・・!

@Hiroshiba Hiroshiba requested a review from a team as a code owner January 2, 2024 13:12
@Hiroshiba Hiroshiba requested review from y-chan and removed request for a team January 2, 2024 13:12
@Hiroshiba Hiroshiba requested review from a team and removed request for y-chan and a team January 2, 2024 13:13
Copy link

github-actions bot commented Jan 2, 2024

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 508 326 coverage-36%
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/cancellable_engine.py 94 72 coverage-23%
voicevox_engine/core_adapter.py 51 17 coverage-67%
voicevox_engine/core_initializer.py 59 30 coverage-49%
voicevox_engine/core_wrapper.py 224 159 coverage-29%
voicevox_engine/dev/core/init.py 2 0 coverage-100%
voicevox_engine/dev/core/mock.py 36 8 coverage-78%
voicevox_engine/dev/tts_engine/init.py 2 0 coverage-100%
voicevox_engine/dev/tts_engine/mock.py 27 0 coverage-100%
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/library_manager.py 92 5 coverage-95%
voicevox_engine/metas/Metas.py 34 0 coverage-100%
voicevox_engine/metas/MetasStore.py 18 6 coverage-67%
voicevox_engine/metas/init.py 2 0 coverage-100%
voicevox_engine/model.py 162 9 coverage-94%
voicevox_engine/morphing.py 71 46 coverage-35%
voicevox_engine/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/preset/Preset.py 13 0 coverage-100%
voicevox_engine/preset/PresetError.py 2 0 coverage-100%
voicevox_engine/preset/PresetManager.py 80 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 17 0 coverage-100%
voicevox_engine/setting/init.py 3 0 coverage-100%
voicevox_engine/tts_pipeline/init.py 3 0 coverage-100%
voicevox_engine/tts_pipeline/acoustic_feature_extractor.py 26 0 coverage-100%
voicevox_engine/tts_pipeline/kana_converter.py 86 1 coverage-99%
voicevox_engine/tts_pipeline/mora_list.py 4 0 coverage-100%
voicevox_engine/tts_pipeline/text_analyzer.py 132 6 coverage-95%
voicevox_engine/tts_pipeline/tts_engine.py 188 8 coverage-96%
voicevox_engine/user_dict.py 145 12 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 13 0 coverage-100%
voicevox_engine/utility/path_utility.py 35 9 coverage-74%
voicevox_engine/utility/run_utility.py 10 7 coverage-30%
TOTAL 2259 724 coverage-68%

@tarepan
Copy link
Contributor

tarepan commented Jan 2, 2024

import 関連のエラーが検出されているようです。

=========================== short test summary info ============================
ERROR - ImportError: cannot import name 'StyleId' from partially initialized module 'voicevox_engine.model' (most likely due to a circular import) (/home/runner/work/voicevox_engine/voicevox_engine/voicevox_engine/model.py)
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.74s ===============================
Error: Process completed with exit code 2.

@tarepan
Copy link
Contributor

tarepan commented Jan 3, 2024

lint 関連のエラーが検出されているようです。

lint finished with error(s)
Error: Process completed with exit code 1.

@Hiroshiba
Copy link
Member Author

ありがとうございます!
(github上でコンフリクト解決したのでformetterがかかってなかったので直そうとしてたのですが、ちょっと環境整備してたら時間かかってコメントもらったことに気づき遅れました 🙇 )

Copy link
Contributor

@tarepan tarepan left a comment

Choose a reason for hiding this comment

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

API レベルの型変更によるキャスト不要化で、run.py 中心に見通しが改善しています👍

int 型 speaker

  • "/morphable_targets" における base_speakers
  • "/synthesis_morphing" における base_speaker / target_speaker

が 依然として int 型です。
後者に関しては synthesis_morphing_parameter 引数で(mypy が検出できていない)型エラーが起きています。
抜けであれば追加が、意図的なものであれば NOTE 追加が良さそうです。

型定義の配置

style_idmetas.Metas に置くなら、Moratts_pipeline 配下に置くべき?」という議論がありそうです。
型定義移動は import エラー解決のワークアラウンドでしょうか? それとも有るべき場所への移動という形でしょうか?
後者の場合、model モジュールに置くべき型と子モジュールに置くべき型の判断基準はありますか?(今後の良い判断基準になりそうです)

@Hiroshiba
Copy link
Member Author

"/morphable_targets" における base_speakers
"/synthesis_morphing" における base_speaker / target_speaker
が 依然として int 型です。

あっ 込み入った変更が必要になるので、別PRの↓での変更を計画していました 🙇

タイトルに偽りありになってしまうので、タイトルをAPIのstyle_id引数を〜というふうに変えつつ、FIXMEコメントを足そうと思います!

synthesis_morphing_parameterでエラーが検知されてないのは気づきませんでした。
lru_cacheを挟むと想定する型が変わっちゃうんですね・・・。なるほど・・・。

「style_id を metas.Metas に置くなら、Mora は tts_pipeline 配下に置くべき?」という議論がありそうです。
型定義移動は import エラー解決のワークアラウンドでしょうか? それとも有るべき場所への移動という形でしょうか?

あ!比較的ワークアラウンドという認識です!ここに移動しないと循環参照になってしまったので・・・。
ワークアラウンドなのでコメントで書き足すべきなのですが、どうにも忘れてしまいますね・・・・・・。足しておこうと思います。

後者の場合、model モジュールに置くべき型と子モジュールに置くべき型の判断基準はありますか?(今後の良い判断基準になりそうです)

metasがmodel内では無いことに今気づきました。
APIとして露出する型はmodel内にあるべきだと感じています。MoraないしPhonemeもとりあえずmodel内かな〜〜〜と!

@Hiroshiba Hiroshiba changed the title APIの型をStyleId型に APIのstyle_idの型をStyleId型に Jan 3, 2024
@Hiroshiba
Copy link
Member Author

変更しました 🙇  再度レビューいただけると・・・!! @tarepan

Copy link
Contributor

@tarepan tarepan left a comment

Choose a reason for hiding this comment

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

LGTM

APIレベルの変更で見通しを改善し、将来の変更点をコメントで明示しています👍
型の配置基準も明確化され将来に繋がる PR です、good work!

@tarepan tarepan mentioned this pull request Jan 3, 2024
1 task
@Hiroshiba
Copy link
Member Author

レビューありがとうございます!マージします!!

@Hiroshiba Hiroshiba merged commit 938f8ae into VOICEVOX:master Jan 3, 2024
3 checks passed
@Hiroshiba Hiroshiba deleted the StyleId仕上げ branch January 3, 2024 11:00
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