-
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
[project-vvm-async-api] get_supported_devices_json
をfallibleに
#502
[project-vvm-async-api] get_supported_devices_json
をfallibleに
#502
Conversation
PR作っておいてなんですが、onnxruntimeのコードを見るとデータ自体はconstexprとして埋まっているみたいなので、 v1.14.1: https://github.com/microsoft/onnxruntime/blob/v1.14.1/onnxruntime/core/providers/get_execution_providers.cc#L172-L184 これがCIで動いているなら実質的にinfallibleなものとみなせるのでは? という気がしてきました。 voicevox_core/crates/voicevox_core/src/devices.rs Lines 43 to 48 in 14aa242
|
つまり「onnxruntimeの現段階での実装上」、事前のユニットテストさえあれば 将来的に実行時にエラーが発生すると考えるならこのPRのようにするべきだし、考えないなら |
infallibleだと考えた場合ですが、build.rsのレイヤーでonnxruntime-rsを呼ぶことで #503 で定数化する( |
見ました! 調査ありがとうございます!! get/freeで良いのかなと思いました! |
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!!
SupportedDevices::get_supported_devices()
が2回呼ばれているのがちょっと不自然に感じましたが、compatible_engine.rsは互換性を持たせ続ける必要があるのでこの形がいいのかなと思いました!!
たぶん問題ないと思うので、テスト通ったらマージしようかなと思います! |
マージします!! 議論ありがとうございました!!! |
内容
voicevox_get_supported_devices_json
をfallibleにします。#280 (comment)
関連 Issue
その他
多分"get_"という命名が不適切になる気がします。