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.

Sign up for GitHub

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

Jump to bottom

#298 CPU版でGPUモードへの切り替えを不可能に #1140

Merged
merged 15 commits into from
Jan 25, 2023
Merged

#298 CPU版でGPUモードへの切り替えを不可能に #1140

merged 15 commits into from
Jan 25, 2023

Conversation

tunamaguro
Copy link
Contributor

@tunamaguro tunamaguro commented Jan 23, 2023 •
edited
Loading

内容

cudaかDirectMLが利用できないときに、GPUモードへの切り替えをできないようにしました

関連 Issue

ref #298

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

  • cudaかDirectMLを利用できるとき(GPUがないので手動で状態を変えています)
    image

  • cudaかDirectMLを利用できないとき
    image

その他

  • GPUがないためCPU環境のみでの検証しています。GPUを持っているどなたか確認していただければ...
  • Vue初心者なのでとんでもないコードを書いているかもしれません。お伝えいただければ修正いたします。

@tunamaguro tunamaguro requested a review from a team as a code owner January 23, 2023 11:58
@tunamaguro tunamaguro requested review from y-chan and removed request for a team January 23, 2023 11:58
@tunamaguro tunamaguro changed the title #298 CPU版で #298 CPU版でGPUモードへの切り替えを不可能に Jan 23, 2023
@tunamaguro tunamaguro mentioned this pull request Jan 23, 2023
Closed
y-chan
y-chan reviewed Jan 23, 2023
View reviewed changes
Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

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

PRありがとうございます！
機能的な面は問題なさそうでした！
今後のメンテナンス的な面から見て、色々コメントしたので、そちらの改善をしていただけると...!:pray:

src/components/SettingDialog.vue Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
src/store/ui.ts Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Jan 23, 2023
View reviewed changes
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.

@y-chan さんレビューありです！！
僕からもちょっと１点だけ！

@tunamaguro
Copy link
Contributor Author

tunamaguro commented Jan 23, 2023 •
edited
Loading

ご指摘いただいた、メッセージの修正とメソッドの細分化等を行いました。

修正後のメッセージは以下のとおりです。
image

お時間がありましたら、レビューをお願い致します。

y-chan
y-chan approved these changes Jan 23, 2023
View reviewed changes
Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

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

ほぼLGTMです！
対応ありがとうございます！

一点だけ、suggestionをしましたので、ご確認いただけると...!

src/components/SettingDialog.vue Outdated Show resolved Hide resolved
@tunamaguro @y-chan
Update src/components/SettingDialog.vue
ac5dcd4 
不要なsetを削除

Co-authored-by: Yuto Ashida <y-chan@y-chan.dev>
Hiroshiba
Hiroshiba reviewed Jan 23, 2023
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ちょっと設計レベルでクール（？）な提案があります！
どっちにせよできることに変わりはないので、気が向いたら修正いただければという感じです！

提案１つ目がstateの重複の解消です。

今２種類のstateengineCanUseGPUとallEngineCanUseGPUがあります。
allの方はengineCanUseGPUから計算できそうです。
これらの情報を同期しないといけず、将来片方をsetしたときにもう片方をsetし忘れるといったことが気軽に起こるかもと感じます！
いわゆる「唯一の情報源」を目指したいかもです。

Vuexにはその機構があって、allEngineCanUseGPUをGETTERにすることで簡単に実現できます。
ただこうするとSET_ALL_ENGINE_CAN_USE_GPU辺りの役割が微妙になるかもです。これは↓の提案にも続きます！

もう１点の提案がengineCanUseGPUすら持たず、エンジンのsupportedDevicesを同期してstateに持たせるだけにする形です。
実はsupportedDevicesさえあればengineCanUseGPUも計算可能なはずで、エンジンの持つsupportedDevicesだけを「唯一の情報源」とできそうです。

ということで提案する形はこんな感じです！

  • FETCH_AND_SET_ENGINE_SUPPORTED_DEVICES
  • SET_ENGINE_SUPPORTED_DEVICES
    • supportedDevicesを保存するだけのmutation
  • ENGINE_CAN_USE_GPU
    • エンジンがGPU使えるか返すだけのgetter
  • ALL_ENGINE_CAN_USE_GPU
    • ↑のALL版

あとはFETCH_AND_SET_ENGINE_SUPPORTED_DEVICESをPOST_ENGINE_START辺りに置いておけば完璧だと思います！

まあでも最初の通りできることは変わっていないので、そのままでも問題ではないと思います！
VuexやReactの思想に合わせた形なので、もし気に入ったら実装にチャレンジしてみていただければ･･･！

src/components/SettingDialog.vue Outdated Show resolved Hide resolved
@tunamaguro @Hiroshiba
Update src/components/SettingDialog.vue
80158d6 
メッセージの文言を修正

Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
@tunamaguro
Copy link
Contributor Author

すごい丁寧なご提案ありがとうございます!!
確かに、そうしたほうがデータの流れも１方向になって直感的になりますし、更新漏れもなくなっていい事ずくめですね!

実装してみたいと思いますが、ちょっともう寝るので明日作業します!

@tunamaguro
Copy link
Contributor Author

tunamaguro commented Jan 24, 2023 •
edited
Loading

前回の改善案でありましたようにGPUを使えるかどうかの判定部分をgetterに移しました。

気持ち的にはENGINE_CAN_USE_GPUの1つ前にENGINE_SUPPORTED_DEVICESが
あっても良いのかなと思いましたが、かなり冗長になると感じたのでやめました。

お時間がありましたら、レビューをお願い致します。

@tunamaguro tunamaguro requested a review from Hiroshiba January 24, 2023 12:15
Hiroshiba
Hiroshiba reviewed Jan 25, 2023
View reviewed changes
src/store/engine.ts Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba approved these changes Jan 25, 2023
View reviewed changes
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！！

Vueっぽい書き方になって良い感じなのかなと感じました！！

辞書全角に引き続きありがとうございます！
VOICEVOXはまだかなり細々とした課題を抱えているので、またよかったらPRやコメントお待ちしています！！

@tunamaguro @Hiroshiba
fix:エラーを握りつぶさないように修正
bedea4f 
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
@tunamaguro
Copy link
Contributor Author

お二人共レビューありがとうございました!!

@Hiroshiba Hiroshiba merged commit 66d7cb5 into VOICEVOX:main Jan 25, 2023
@y-chan y-chan mentioned this pull request Feb 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y-chan y-chan y-chan approved these changes

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tunamaguro @Hiroshiba @y-chan