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

project-vvm-async-api #497

Merged
merged 20 commits into from
Jul 22, 2023
Merged

project-vvm-async-api #497

merged 20 commits into from
Jul 22, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented May 22, 2023

内容

project-vvm-async-apiをmainにマージします。

project-vvm-async-apiは以下の変更からなる予定です。

  1. APIの更新
    1. Python API(及び今後追加されるかもしれない他言語API)のasync化 (実装はtokioベース)
    2. VVMファイルの読み込みという形でモデルを読む
    3. その他諸々の破壊的変更
  2. /crates/voicevox_coreのモジュール構造のリファクタ

タスクリスト

タスクリストは随時更新します。

タスクリスト (オプショナル)

  • 以前のAPIをdeprecatedなものとして残す
    • C API
    • Python API

関連 Issue

Closes #274.
Resolves #276.
Resolves #280.

その他

Co-authored-by: Ryo Yamashita <[email protected]>
Co-authored-by: Nanashi. <[email protected]>
qryxip added 3 commits June 3, 2023 01:43
* `voicevox_get_version()` → `voicevox_version`

* indocを外す

* コメントを更新

* テスト`global_info`を追加

* `make_default_`系を定数に

* Minor refactor

* diffを減らすためにquoteをダウングレード

* Minor refactor

* Minor refactor

* `SupportedDevices`の中身も確認する

* Minor refactor

* `version!()` → `VERSION`

* `Default`のimplはマクロ側に寄せる

* `voicevox_create_supported_devices_json`のシグネチャを直す
`$OUT_DIR`を使うものをtest_utilクレートに移動
qryxip and others added 3 commits June 12, 2023 22:02
* Docs: 事例紹介にvoicevoxcore.goを追加 (#498)

Docs: 事例紹介のvoicevoxcore.goを追加

* 間違った`char*`の解放を明示的に拒否する (#500)

* Cargo.tomlをフォーマットする (#504)

* Update rust toolchain 1.70.0 (#506)

Co-authored-by: PickledChair <[email protected]>
Co-authored-by: Ryo Yamashita <[email protected]>

* Rust APIのbuild.rsを抹消する (#508)

* Python APIでは`panic=unwind`にする (#505)

* Python APIでは`panic=unwind`にする

* C APIは`-C panic=abort`でビルドする

* READMEのスペースが足りてなかった (#511)

* windows-latestでなぜかdownload_testが落ちるのを改修 (#517)

* windows-latestでなぜかdownload_testが落ちるのを修正

* a

* cbindgenに`fn.args=vertical`を設定し、`__declspec`の後の空行を消す (#518)

* `__declspec(dllimport)`と関数定義の隙間の空行を消す

* `fn.args="vertical"`を設定

* cbindgenの`include_version`を有効化する (#519)

* cbindgenの`include_version`を有効化する

* jobをリネーム

* 誤字

---------

Co-authored-by: Kota Amasaka <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: PickledChair <[email protected]>
Co-authored-by: Hiroshiba <[email protected]>
@Hiroshiba
Copy link
Member

いろいろな判断や考えを先行してお伝えすると捗るかなと思ったのでお伝えします🙏

async/await周りを精査する

過去の事例を考えると「DirectMLだと動かない」みたいなことはありえる気がします。
とりあえずCPU環境で動けばOKで、動かない環境が出てきたときはドキュメントに書く感じで良いかなと!

[議論] 文字列の_free系のAPIについて

特定のポインタだけ弾くという形で良さそう!

[議論] voicevox_synthesizer_audio_query → voicevox_synthesizer_create_audio_query

ぜひ🙏
同じ系統だなとユーザーが思うものが同じ命名規則に従っていることが大事かなと!

[議論] Open JTalkは"text analyzer"のように抽象化した方がいいのではないか? (名前の破壊的変更をやるなら今がベスト)

以前と言ってること違うかもなのですが、できればそのままがOpenJTalkが良いかなと思います。
ユーザーが「TextAnalyzerにはOpenJTalkを入れる」という非直感的なコードを書かないといけなくなり、複雑度が上がってしまうためです。
辞書ファイルが密結合なのと、interfaceが1個増えちゃうのと、汎用的に書こうとするとドキュメントがちょっとややこしくなってしまうので。
あとOpenJTalk以上に優れたOSSが出てきたり、多言語対応するとなると置き換わるかもなのですが、そんなポンポン出てくるものではないし、そもそも他のAPIも変わりそうなので・・・。

5段階中4くらいの不要感がありますが、5段階中5の必要感がある方がいらっしゃったら実装すべきかを議論したいです!

[議論] ロガーの初期化はオプションなりで明示的にする?

最低限、ログレベルは切り替えられればなんでも良いかなと思っています。
それ以上は実装行数が増えすぎない(100行未満くらい)なら、いろいろ制御できたほうがかっこいいとは思います。

cbindgenがextern constへのコメントを吐いてくれないので、調査して対応を考える

こちらは前コメントしたかもなのですが、そのままにしてcbindgenにissue作るだけで良いかなと・・・。

bindgenが荒ぶってアイテムの順序が変わるので、sort_by = trueにしてアルファベット順にするのはどうか?

良いと思います! もしかしたら微妙かもとも思っているので、一度見てみたいかもです。


他にもなにか聞きたいことなどあればなんでも・・・!!

@qryxip
Copy link
Member Author

qryxip commented Jul 2, 2023

C APIで&mut VoicevoxSynthesizer (VoicevoxSynthesizer*)を引数に取っている関数があるが、これを&VoicevoxSynthesizer (const VoicevoxSynthesizer*)にする

こちらですが3つの選択肢が見えてきました。

  • 全体をMutexで包むと、以下の4つが排他となる
    1. 一つのモデルの存在確認(contains)
    2. 一つのモデルの実行(get)
    3. 一つのモデルの追加(insert)
    4. 一つのモデルの削除(remove)
  • 全体をRwLockで包むと、以下の3つが排他となる
    1. 複数のモデルの存在確認(contains), 実行(get)
    2. 一つのモデルの追加(insert)
    3. 一つのモデルの削除(remove)
  • flurryを用いると、以下の2つが排他となる
    1. 複数のモデルの存在確認(contains), モデルの実行(get), モデルの追加(insert)
    2. 複数のモデルの削除(remove) (操作自体はノンブロッキングで、実際の削除がi.が収まるまで遅延される)

Mutexは「asyncなAPI」としての意味が無くなるので選択肢から外すとして、個人的にはflurryのようなデータ構造を使うのがいいと思っています。

(ただ今の実装は結構ステートフルな設計になっているので、そこのmutを剥がすのを設計的にどうしようかと思っています。剥がすことはできてもdiffは凄いことになりそう)

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 2, 2023

その3つだと2番めの

全体をRwLockで包むと、以下の3つが排他となる

が良さそうに感じました! async周りで満たしたい要件はこんな感じかなと・・・!

  • 1つのSessionは同時実行不可能
  • Sessionが違う場合は同時実行可能
  • それ以外はスレッドセーフならなんでもOK(Sessionの実行に比べると一瞬で完了するので)
  • 実装は簡単なほど良い

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 8, 2023

@y-chan
vvm-async周りのこのプルリクエストをmainにマージすることを考えています。

残る大きなタスクはドキュメントと効率的なasync化で、まだリリースするには早いのですが、ちょっとmainから離れている期間が長くなっていることと、辞書やprojects用のAPIの追加を行うのに支障が若干出始めているので一旦マージしたほうが良いのかなと・・・!

マージ後のmainの取り扱いは、Rust化のとき同様に工事中にすることを考えてます。
readmeで工事中を案内し、仮にキャラクター追加やhotfixがあった場合はrelease-0.14から派生させたbracnhにコミットする運用です。
(エディタ・エンジン側が0.15化した場合、こちらもrelease-0.14から派生してrelease-0.15を作るべきかはちょっと迷っています。まあ作らなくて良さそう?)

やることとしては

  • README.mdの一番上に工事中であるということとどこを見ればいいかの案内を追加
  • タスクリストを移動させるためのissueを作成
  • みんなでプルリクエストを全体的に確認して大きな問題があるかどうかを一応確かめる(オプション)

かなと思っています。
そもそもマージすべきか、マージするとして何か課題があるかなど、どうでしょう・・・?

@y-chan
Copy link
Member

y-chan commented Jul 9, 2023

サッとPRを眺めてみたり、関連PR/Issueを眺めてみたりしましたが、挙げられている点以外特に課題感はないかなと思いました...!
現時点でマージした方がこの先いろいろ進めやすくなるかなと思います!

エンジン/エディタ側のアップデートをする時は、こちらも合わせてバージョンを上げた方が、気持ち的にはいい気はしますが、特に機能変化がないのにアップデートするのは明らかに変ですし、現時点で既に各モジュールでバージョンが合ってない・合わせる必要がないので、vvm-asyncの完成と、エディタ周りのアップデートサイクルがかみ合わない感じであれば、0.14.xのままでいいと思います。

@Hiroshiba
Copy link
Member

@qryxip - [ ] VOICEVOX_RESULT_*_ERRORをVOICEVOX_*_ERRORにする?
という項目を追加しようかなと思います 🙏
(ちなみにどう思われますか 👀 )

@Hiroshiba
Copy link
Member

@y-chan ありがとうございます!!!

@qryxip ということで、このPRのdraftを開けていただければ!!!
僕と @qryxip さんと、可能なら @y-chan @PickledChair @sevenc-nanashi さんあたりで大雑把で良いので見ていければと思います・・・!!

ちなみにmainへのマージタスクリストはこちら。
#497 (comment)

@qryxip qryxip marked this pull request as ready for review July 10, 2023 23:59
@qryxip
Copy link
Member Author

qryxip commented Jul 11, 2023

- [ ] VOICEVOX_RESULT_*_ERRORをVOICEVOX_*_ERRORにする? という項目を追加しようかなと思います 🙏
(ちなみにどう思われますか 👀)

よいと思います。

@qryxip
Copy link
Member Author

qryxip commented Jul 11, 2023

いやよく考えたらenumは他にacceleration mode(VOICEVOX_ACCELERATION_MODE_…)とword type(#538VOICEVOX_USER_DICT_WORD_TYPE_…)もあるので、"RESULT"というprefixはあった方が統一感が保たれるという考えもできるかも...?

@Hiroshiba
Copy link
Member

あ、ほんとですね!! RESULTは付けたほうが良さそう。
- [ ] VoicevoxResultCodeのエラーでVOICEVOX_RESULT_*_ERRORになっていないものを直す
を足しておきます!

@PickledChair
Copy link
Member

@Hiroshiba

こんにちは、ずいぶんご無沙汰してしまいました。レスポンスがなくてすみませんでした……!

変更が多岐に渡るため( @qryxip これまでの作業ありがとうございます!)、細かいチェックなどはできていないのですが、全体的な方針は問題ないと感じており、main にマージしてしまっても良いのではないかと思っています。その後、問題が見つかり次第、適宜対処するのが良いのかなと思いました。何か特に今の段階でチェックすべきという部分があれば、教えていただけるとありがたいです……!

@qryxip
Copy link
Member Author

qryxip commented Jul 15, 2023

この2つですかね...? タスクリストはちゃんと作っているので、チェックと言えるのは特に無いかと思います。

  • README.mdの一番上に工事中であるということとどこを見ればいいかの案内を追加
  • タスクリストを移動させるためのissueを作成

#497 (comment)

@qryxip
Copy link
Member Author

qryxip commented Jul 22, 2023

タスクリストはとりあえず #545 を作って移しました。

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

ざっと全部確認させていただきました!!
またちょっとタスクは残っているかもしれませんが、本当にお疲れ様です!!!!
完成まで一緒に頑張れるととても嬉しいです!!!引き続きよろしくお願いします・・・!!!

crates/voicevox_core_c_api/src/c_impls.rs Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit c3f61e2 into main Jul 22, 2023
@Hiroshiba
Copy link
Member

ちゃんとstashなくマージできたので、project-vvm-async-apiブランチとbuffer用のブランチ消しちゃいますか!!
(念のために残しておくのもありだと思います)

@qryxip
Copy link
Member Author

qryxip commented Jul 23, 2023

#532 (comment)をやりましたし、消していいと思います。

(PRがまだ向いている状態でブランチを消そうとするとどうなるんだろう... 弾かれる?)

@Hiroshiba Hiroshiba deleted the project-vvm-async-api branch July 23, 2023 11:17
@Hiroshiba
Copy link
Member

とりあえず消去しました!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants