-
Notifications
You must be signed in to change notification settings - Fork 118
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
補足:
@@ -313,85 +264,202 @@ pub(crate) mod tokio { | |||
/// 音声モデル。 | |||
/// | |||
/// VVMファイルと対応する。 | |||
#[derive(Clone)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VoiceModel
からClone
を外しているのは、現段階での設計がすっきりするという理由もあるのだが #829 をやるためでもある。
(ファイルディスクリプタを専有して排他ロックを掛けることになるため)
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()) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
別ファイルに移動。ただしちょっと実装を変更してある(attrs
も取得するようにした)
There was a problem hiding this comment.
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
のように設計しようと考えています。
There was a problem hiding this 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あまりわかってない僕がわからなくても問題ない箇所もあるとは思うので、一旦コメントまでということで・・・!
|
ccbda99
to
e84fed7
Compare
|
27265d8
to
18991c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
@sevenc-nanashi さんのコメントがあるかもと思ったのでマージしてないです。
特になければマージで・・・!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
大丈夫だと思います。
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)
内容
voice_model.rsの大半を書き直し、 #746 と #828 を行います。また、*.{onnx,bin}について
VoiceModel::from_path
の時点で存在確認を入れるようにします。関連 Issue
Resolves #746.
Resolves #828.
その他