ONNX Runtimeとモデルのシグネチャを隔離する

[qryxip]

内容

ONNX Runtimeとモデルのシグネチャ(?)の存在を、別のモジュールに分離します。

以下の改革の前準備です。

#553 (comment)

関連 Issue

#545

その他

[Hiroshiba]

なかなか巨大になりそうですね…！！
お待ちしてます！！

[qryxip]

大体考えて組んだので、draftを外します。

モジュールinferの登場人物は主に3つに分けられます。これらをもとに抽象化を組んでいます。
(ただinfer.rsは115行しかないといっても、中身の圧が強いのでファイル分割すべきかもしれません)

• trait InferenceRuntime (e.g. Onnxruntime)
  • trait Session
  • trait RunBuilder
  • trait InputScalar (e.g. i64, f32)
• trait Signature
  • trait Output (e.g. (Vec<f32>,))
  • enum SignatureKind { PredictDuration, PredictIntonation, Decode })
    • struct PredictDuration: Signature<Kind = SignatureKind>
    • struct PredictIntonation: Signature<Kind = SignatureKind>
    • struct Decode: Signature<Kind = SignatureKind>
• struct SessionSet<K (= SignatureKind), R: InferenceRuntime>
  • struct SessionOptions

補足としては:

• onnxruntime-rsを触る部分はまとめてcrate::infer::runtimes::onnxruntimeに隔離しました。
• model_fileモジュール (復号部分), SessionOptions, SessionSet, InferenceModelsなどはinferモジュールに移しました。
  • status::SessionSetとvoice_model::InferenceModelsはenum-mapクレートを用いてEnumMap<SignatureKind, …>のような形で表現しています。
• <R: InferenceRuntime>の型引数はSynthesisEngineのあたりで止めています。またSignatureKind (= PredicutDuration | …)もstatusモジュールに直接登場させています。
• 将来の変更に対する耐性としては、次のように考えています。
  • ONNX Runtime以外のランタイム(e.g. TFLite)を導入するときは、crates::infer::runtimesに新しいランタイムを増設します。
    • featureを導入し、コンパイル時にONNX Runtimeと切り替えられるようにします。
    • ただVVMの仕様も含め、パブリックAPI部分の変更はそもそもどのみち避けられないかと思います。
  • モデルのシグネチャや数を変更するときは、crate::infer::signatureを変更します。
    • ([f32],)以外の出力にするときは、infer/runtimes/onnxruntime.rsのimpl Output<Onnxruntime> for (Vec<f32>,)の部分を拡張します。
    • predict部分のモデルとdecodeのモデルを別々に分けて読み込みたい、という需要が将来発生した場合、enum SignatureKindの存在を二つに分割してそれぞれについてEnumMapを管理します。
    • 3-モデルの組と4-モデルの組を同時に扱いたいとなったときもenum SignatureKindを複数作って抽象化します。

[Hiroshiba]

見渡しました！
input/outputの加工用関数の非対称性が気になりましたが、全体的に良い感じなのかなと思いました！

[sevenc-nanashi]

ヒホさんと同意見ですー。

[qryxip]

色々再構成しました。
e0f29c6
cb1db34
c3e08dd
e4b91ab
8584d27
4795309
a5dbbdd
525f4b1

主な変更点として:

• trait InferenceRuntimeを継承する次の3つのトレイトを導入し、これらに役割を持たせました。
  • trait SupportsInferenceInputTensor<I> (Iの例: Array1<i64>)
  • trait SupportsInferenceInputSignature<I> (Iの例: PredictDurationInput)
  • trait SupportsInferenceOutput<O> (Oの例: (Vec<f32>,))
• trait SignatureをInferenceSignatureとInferenceInputSignatureに分離しました。またtrait InputScalarとtrait Outputは、上記のInferenceRuntimeのサブトレイトに吸収される形で消えました。 (ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment))
• RunBuilderは(基本的に)自分で関数を持たなくなったので、RunContextとしました。(ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment))
• RunContextを除きprefixにInferenceを付けるようにしました。(ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment))

[Hiroshiba]

ほぼLGTMです！！

コードの読み方がわかりました。onnxruntime.rsを読んで、signature.rsを読んでからinfer.rsを読むとわかりやすいですね！

ドキュメントを作っておかないと迷子になると思います。
と言っても大層なものは必要なくて、infer.rsのそれぞれのtraitやimplに1行ずつ目的を書いていくだけでだいぶ変わるかなと！

あと、実装レビューの前に設計レビューをすると良かったかもとか思いました。

今まではonnxruntime周りが煩雑だったがゆえに、変更が生じても適当に書き足せば良かったのですが、今回で理路整然となるので、追加の実装をしたいときにどこに足せば良いか分からなくなるだろうなと思いました。
project-sなどで関数を追加したり形を変えたりすることがちょっとありそうですが、その際は相談させていただけると･･･！！

[qryxip]

• 96a93e9: ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment)
• 59d8779: voicevox_core_macrosのリファクタ(モジュール分割)をしました。
  200行以上のコードのインデントのずれが生じるため、今やった方がよいかなと…
• d0dc56f: InferenceDomain::{INPUT,OUTPUT}_PARAM_INFOSを統合し、タプルのEnumMapにしました。
• c654cd1: ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment)
• 868d3f6: ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment)
• 0998793: ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment)

[qryxip]

であれば、もう一旦マージして、階層構造を小さくすることに取り組むのが良いのかなと思いました。 実装していく上でまた新しい課題が見つかったりもする可能性も全然あると思いますし。

構造の変更はもうこのプルリクエスト内ではやめて、名称の変更やドキュメントの追加だけにして、次に進むというのはどうでしょう･･･？ （1回のレビューに3時間かかっており。。。。）

はい。それがよいと思います。

階層構造ですが、今InferenceCoreでやっている「入出力のworkaround処理」をdomain.rs(旧signatures.rs)に移せれば、こんな感じにできるのではと思っています。

synthesizer.rs --+--> infer/status.rs -----> infer/runtimes/
                 +--> infer/domain.rs
                 |
                 +--> open_jtalk.rs

[Hiroshiba]

こんな感じにできるのではと思っています。

なるほどです、良さそうに思いました！！

なんとなくの直感ですが、statusがかなり多くの責務を担いそうだなと思いました。statusという名称が曖昧なので便利屋さんみたいになりがちかも。将来なんか名前変えられると良さそう！
あとSynthesizerが何を見れるべきかを考えていくとアーキテクチャ作りやすいのかなとか思いました。RuntimeやSessionは見れるべきではない、など？

[Hiroshiba]

ほぼLGTMです！！

Domainのとこだけ落としどころが見つかれば完成なのかなと思いました！！

[qryxip]

• 75fd7ac: https://github.com/VOICEVOX/voicevox_core/pull/675/files/81b5804037ea21fe0b3fe0ae934fe1d2a2548c74#r1392976600

[qryxip]

• ad222c9: ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment)

[Hiroshiba]

LGTM！！！！！！

お疲れ様でした！！！！
良い切り分けができたのではないかなと思います！！！

Interfaceの設計をレビューするということに慣れておらず、右往左往してしまってすみませんでした 🙇
（コードがある場合は、先に実装を見て書き心地を把握し、ドメイン用語の認識をそろえて、設計方針の認識を揃えていくのが良さそうだと思いました！）

一気に多層構造も切り崩していきたいですね･･･！！！

[qryxip]

• 7005c96: ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment)
• 9417992: Merge branch 'main' into HEAD
• 1655719: doc内の「voicevox_core内の」という表現を「Rust APIクレート内の」と置き換えました。
• a73f22c, f17919b: ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment)

[qryxip]

• 48bdb1b: ONNX Runtimeとモデルのシグネチャを隔離する #675 (comment)
• 9d7d001: 二重説明になっていた箇所を削除
• af828eb: 不要な修飾を削除
• b6b7975: マクロの中でのみ使っているArrayExtを、マクロの中に押し込める

[Hiroshiba]

修正確認しました！！マージします！！！