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

Merge v2.0.0-rc.4, keeping ONNX Runtime v1.17 #7

Merged
merged 50 commits into from
Aug 17, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Aug 15, 2024

内容

関連 Issue

スクリーンショット・動画など

その他

decahedron1 and others added 30 commits April 27, 2024 17:46
still need constant inputs and functions for getting I/O lengths & names from indexes
Co-authored-by: Florian Kasischke <[email protected]>
aka The Cleanening, part 2

- Add clearer documentation and examples for more things.
- Rework string tensors by introducing `PrimitiveTensorElementType` for primitive (i.e. f32) types, and again re-implementing `IntoTensorElementType` for `String`. This allows string tensors to be used via `Tensor<String>` instead of exclusively via `DynTensor`. Additionally, string tensors no longer require an `Allocator` to be created (which didn't make sense, since string data in Rust can only ever be stored on the CPU anyway). This also now applies to `Map`s, since their data also needed to be on the CPU anyway. (`Sequence`s are currently unaffected because I think a custom allocator could be useful for them?)
- Rework the `IoBinding` interface, and add an example clarifying the intended usage of it (ref pykeio#209). Thanks to AAce from the pyke Discord for pointing out the mutability issue in the old interface, which should be addressed now.
- Refactor `OperatorDomain::add` from the slightly-nicer-looking-but-more-confusing `fn<T>(t: T)` to just `fn<T>()` to further enforce the fact that `Operator`s are zero-sized.
- Maps can now have `String` keys.
- Remove some unused errors.
This whole thing where usize != size_t specifically on aarch64 is so bad.
@qryxip qryxip force-pushed the merge-v2.0.0-rc.4 branch 2 times, most recently from f532a7a to b5350a7 Compare August 16, 2024 14:55
@qryxip qryxip changed the title Merge v2.0.0-rc.4 Merge v2.0.0-rc.4, keeping ONNX Runtime v1.17 Aug 16, 2024
@qryxip qryxip marked this pull request as ready for review August 16, 2024 14:56
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.

ONNX Runtime v1.17(というよりONNX v1.15)を使い続けなければならない事情が出てきたため、17のAPIを使い続けるようにしました。

このPRで行った変更に対する補足をレビューコメントとして残します。あとPRのdiffとしては出ませんが、次のONNX Runtime C APIの利用を削りました。

run: cargo clippy -p voicevox-ort --all-targets --workspace --features fetch-models
run: cargo clippy -p voicevox-ort --all-targets --features fetch-models
Copy link
Member Author

Choose a reason for hiding this comment

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

ort-sys/src/lib.rsをv2.0.0-rc.2時点に留めていることで pykeio#202 で追加されたtraining機能のコンパイルが通らなくなっているため、examples/trainingをClippyの対象から外す。

APIの辻褄合わせはできるだろうが、我々が使わないであろうtraining機能を維持するよりは捨てる方向にした方がよいと思った。

Copy link
Member

Choose a reason for hiding this comment

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

賛成です!

乖離しすぎるとそれはそれで追従が大変になってくるので、もしそうだったら再考してもいいかも。

Comment on lines -348 to +395
"cu12" // ビルド環境に何がインストールされていようが、常にCUDA 12を使う
"cu12+cudnn8" // ビルド環境に何がインストールされていようが、常にCUDA 12とcuDNN 8を使う
Copy link
Member Author

Choose a reason for hiding this comment

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

cuDNN 9の方を使うことはないので、もしかしたら識別子はcu12のままでもよいかも?

Copy link
Member

Choose a reason for hiding this comment

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

ちゃんとわかってないけど、元のから差分が少ない形に保っておくと追従しやすいかも?
元のにcudnnに関しての記載がないなら、記載ない方が揃いそう。
でも把握しきれるならどちらでも・・・!!

Copy link
Member Author

Choose a reason for hiding this comment

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

正直pykeio側とのdiff的には変わらないので、v2.0.0-rc.4の方の記述に合わせるといった見方もできると思います。なので+cudnn8でも…?

Comment on lines -42 to +49
pub trait ExecutionProvider {
pub trait ExecutionProvider: Sync + Send {
Copy link
Member Author

Choose a reason for hiding this comment

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

v2.0.0-rc.4ではExecutionProviderDispatchがenumからArc<dyn ExecutionProvider>のラッパー構造体に変わったため、Environment!Synd!Sinkになってしまっている。pykeio/ortとしては特に困ることはないだろうが、我々としては困る。

最小の変更でなんとかするにはExecutionProvider自体を: Sync + Sendにするのがベストと判断。

Comment on lines -340 to +392
"rocm"
} else {
"none"
};
let _ = designator; // 上記のものを無視する
let designator = if cfg!(feature = "directml") {
feature_set.push("rocm");
}
let feature_set = if !feature_set.is_empty() { feature_set.join(",") } else { "none".to_owned() };
let _ = feature_set; // 上記のものを無視する
let feature_set = if cfg!(feature = "directml") {
Copy link
Member Author

Choose a reason for hiding this comment

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

変数名が変更されている (designatorfeature_set)

Comment on lines -297 to +303
let mut env = EnvironmentBuilder::default().with_name(env!("CARGO_PKG_NAME"));
let mut env = EnvironmentBuilder::new().with_name(env!("CARGO_PKG_NAME"));
Copy link
Member Author

Choose a reason for hiding this comment

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

pykeio@c64b8eaで行われた変更に追従。

@qryxip qryxip requested a review from Hiroshiba August 16, 2024 15:40
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!!

1箇所変更が生じるかもしれないコメントをしたので、そちらだけお待ちします!

📝 squashをff mergeにしてからマージ

@Hiroshiba
Copy link
Member

マージします!

@Hiroshiba Hiroshiba merged commit 8627833 into VOICEVOX:main Aug 17, 2024
7 checks passed
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.

6 participants