Skip to content

metasとsupported_devicesの改善を行った #274

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
Closed
wants to merge 8 commits into from
Closed

metasとsupported_devicesの改善を行った #274

wants to merge 8 commits into from

Conversation

qwerty2501
Copy link
Contributor

内容

  • voicevox_coreのmetasとsupported_devicesに対するCString依存を取り除いた
  • python APIの場合、metasとsupported_devicesを関数から取得するようにした

その他

pyo3はちょっと自信がないので念入りに見ていただけると助かります

@qwerty2501
Copy link
Contributor Author

qwerty2501 commented Sep 13, 2022 •
edited
Loading

#276 より、staticに取得できたほうが良さそうだったのでstaticmethodにしました

@qryxip
Copy link
Member

qryxip commented Sep 13, 2022

関数化はともかくとして、PyO3実装においてclass VoicevoxCoreは現状「initializeからfinalizeまでの間にできる操作を提供するオブジェクト」という位置付けにあるという認識です(なぜならコンストラクタでinitializeされ、デストラクタでfinalizeされるため)。その位置付けをそのままにしておくのであれば、それらの操作との関連性が薄いmetasとsupported_devicesは場所も別の所にした方がよいのではないかと思いました。

@qwerty2501
Copy link
Contributor Author

@qryxip @Hiroshiba クラス設計については概念的なもので言語を超えて統一させるべきだと思うのでもし変える必要があるのであればRust側も変えたほうが良さそうですね

@qwerty2501 qwerty2501 mentioned this pull request Sep 13, 2022
Closed
@PickledChair
Copy link
Member

@qryxip

PyO3実装においてclass VoicevoxCoreは現状「initializeからfinalizeまでの間にできる操作を提供するオブジェクト」という位置付けにあるという認識です
それらの操作との関連性が薄いmetasとsupported_devicesは場所も別の所にした方がよいのではないかと思いました。

個人的な感覚になってしまいますが、metas も supported_devices も決して関連性が薄いとは言えないような気がしています。どのような話者が使えるのか (metas) や、GPU サポートがあるのかどうか (supported_devices) は initialize から finalize の間に行う操作に関連する情報としてライブラリユーザーにとって興味のある情報だと思います。この２つを VoicevoxCore クラスと関連付けさせるのも決して意味のない選択ではないと考えました。

@qwerty2501
Copy link
Contributor Author

#276 でもクラス設計に関する議論がされているのでまずクラス設計専用issue立ててそこで議論したほうがいいかもしれません

@Hiroshiba
Copy link
Member

@qwerty2501 さんの仰る通り、最初にクラス設計を考えるのが良いのかなと思いました！

@qwerty2501 qwerty2501 mentioned this pull request Sep 14, 2022
Closed
@qwerty2501
Copy link
Contributor Author

issue作成したのでそちらで議論しましょう #280

@qwerty2501
Copy link
Contributor Author

@Hiroshiba そういえば配信でinitializeは何回でもできるみたいなことを発言されていたようですが今のpython apiだとnewされる時に同時にinitializeされapiとしてinitializeが存在しないようですがこれって良くないでしょうか？

@qryxip
Copy link
Member

qryxip commented Sep 14, 2022

再initializeについてはVoicevoxCore.restart(self, ...)みたいな形の再initialize用のメソッドを足せばいいかなと考えてました。

@Hiroshiba
Copy link
Member

おっとなるほどです！

こちらで詳細に議論されそうなので、こちらでお答えします！

@qwerty2501
Copy link
Contributor Author

このPRはこのままreview進めて良さそうですね

@qwerty2501
Copy link
Contributor Author

metasをメンバ関数にしました

@qwerty2501
Copy link
Contributor Author

メンバ関数にしましたがこの問題がありそうなので見ていただけると
#280 (comment)

qryxip
qryxip reviewed Sep 17, 2022
View reviewed changes
@qwerty2501 @qryxip
Update crates/voicevox_core_python_api/python/voicevox_core/_rust.pyi
fc10379 
Co-authored-by: Ryo Yamashita <qryxip@gmail.com>
@qryxip
Copy link
Member

qryxip commented Sep 23, 2022

そういえばexample/pyo3が更新されてないと思います。

@qwerty2501
Copy link
Contributor Author

クラス設計が決まらないと進められそうにないのでここも一旦凍結ですかね。draftにします

@qwerty2501 qwerty2501 marked this pull request as draft September 24, 2022 01:56
@qryxip qryxip mentioned this pull request May 5, 2023
Merged
@qryxip qryxip mentioned this pull request May 22, 2023
Merged
67 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qryxip qryxip qryxip left review comments

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

Successfully merging this pull request may close these issues.
4 participants
@qwerty2501 @qryxip @PickledChair @Hiroshiba