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

improve: rework VoiceModel #830

Merged
merged 18 commits into from
Sep 12, 2024
Merged

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Sep 7, 2024

内容

voice_model.rsの大半を書き直し、 #746#828 を行います。また、*.{onnx,bin}についてVoiceModel::from_pathの時点で存在確認を入れるようにします。

関連 Issue

Resolves #746.
Resolves #828.

その他

@qryxip qryxip requested a review from Hiroshiba September 7, 2024 20:44
@qryxip qryxip mentioned this pull request Sep 8, 2024
3 tasks
Copy link
Member Author

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

補足:

crates/voicevox_core/src/voice_model.rs Outdated Show resolved Hide resolved
@@ -313,85 +264,202 @@ pub(crate) mod tokio {
/// 音声モデル。
///
/// VVMファイルと対応する。
#[derive(Clone)]
Copy link
Member Author

Choose a reason for hiding this comment

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

VoiceModelからCloneを外しているのは、現段階での設計がすっきりするという理由もあるのだが #829 をやるためでもある。
(ファイルディスクリプタを専有して排他ロックを掛けることになるため)

Comment on lines -352 to -375
fn struct_fields(data: &Data) -> syn::Result<Vec<(&syn::Ident, &Type)>> {
let fields = match data {
Data::Struct(DataStruct {
fields: Fields::Named(fields),
..
}) => fields,
Data::Struct(DataStruct { fields, .. }) => {
return Err(syn::Error::new(fields.span(), "expect named fields"));
}
Data::Enum(DataEnum { enum_token, .. }) => {
return Err(syn::Error::new(enum_token.span(), "expected a struct"));
}
Data::Union(DataUnion { union_token, .. }) => {
return Err(syn::Error::new(union_token.span(), "expected a struct"));
}
};

Ok(fields
.named
.iter()
.map(|Field { ident, ty, .. }| (ident.as_ref().expect("should be named"), ty))
.collect())
}

Copy link
Member Author

Choose a reason for hiding this comment

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

別ファイルに移動。ただしちょっと実装を変更してある(attrsも取得するようにした)

crates/voicevox_core/src/asyncs.rs Show resolved Hide resolved
Copy link
Member Author

Choose a reason for hiding this comment

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

(WASM版でスレッドプールが使えなさそうという不測はありましたが、)#746 (comment)は本PRのVoiceModelのように設計しようと考えています。

crates/voicevox_core/src/asyncs.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/asyncs.rs Outdated Show resolved Hide resolved
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.

voice_modelのreadでオンメモリにする感じですよね!
なるほどでした!!!

いくつか僕視点でわからなかった点をコメントしてみました!
まあ別にRustあまりわかってない僕がわからなくても問題ない箇所もあるとは思うので、一旦コメントまでということで・・・!

@qryxip
Copy link
Member Author

qryxip commented Sep 10, 2024

@qryxip qryxip force-pushed the improve-rework-voice-model branch from ccbda99 to e84fed7 Compare September 10, 2024 14:59
@qryxip qryxip requested a review from Hiroshiba September 10, 2024 15:23
@qryxip
Copy link
Member Author

qryxip commented Sep 10, 2024

@qryxip qryxip force-pushed the improve-rework-voice-model branch from 27265d8 to 18991c1 Compare September 12, 2024 01:22
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!!

@sevenc-nanashi さんのコメントがあるかもと思ったのでマージしてないです。
特になければマージで・・・!

Copy link
Member

@sevenc-nanashi sevenc-nanashi left a comment

Choose a reason for hiding this comment

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

大丈夫だと思います。

@Hiroshiba Hiroshiba merged commit e07c795 into VOICEVOX:main Sep 12, 2024
35 checks passed
qryxip added a commit that referenced this pull request Sep 13, 2024
Tokioに依存したプログラムは、async-stdやsmolで使うことはできない。一方、
現在のVOICEVOX CORE Rust APIのTokio依存部分は
`tokio::task::spawn_blocking`のみである。そのため
`tokio::task::spawn_blocking`を、同等の機能を持つblockingクレートのも
のに置き換えることでRust APIの"脱Tokio"を行う。

https://docs.rs/crate/blocking

またこの"脱Tokio"に伴い、Rust APIの`voicevox_core::tokio`を
`voicevox_core::nonblocking`にリネームする。

Python APIではpyo3-asyncioが現在Tokio版かasync-std版しかない状態なので、
Tokioに依存した状態のままにしてある。またtest_utilやdownloaderではこれま
で通りreqwestに依存する。

また将来`Synthesizer`や`OpenJtalk`なども`trait Async`をベースにした設計
にすることを考えているが、本PRではTODOコメントを残すのみにしてある。

#830 (comment)
qryxip added a commit to qryxip/voicevox_core that referenced this pull request Sep 24, 2024
qryxip added a commit that referenced this pull request Sep 24, 2024
`UserDict`に対して以下の二つを行う。

1. #830 のように`Inner<A: Async>`で共通化する設計にする
2. async-fsを導入することでasync APIとしての質を向上させる
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.

VVMファイル全体をメモリに載せるのをやめる VoiceModelの実装を統合する
3 participants