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

1.14.1のORTを対象にし、$ORT_RUST_STRATEGYのデフォルトをdownload #2

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Mar 25, 2023

Description

microsoft#12606で追加されたRustバインディングに、まずは以下の変更を加えます。

  • mainブランチのlibonnxruntimeを対象にしているので、1.14.1に固定する
  • $ORT_RUST_STRATEGYのデフォルトがcompileになっているので、旧onnxruntime-rsと同じdownloadに戻す
    "TODO"とありますし、これを見るにこのリポジトリの外では動かないと思います。
    let mut config = cmake::Config::new("../../cmake");

Motivation and Context

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.

長らく気づかずすみません!

mainブランチのlibonnxruntimeを対象にしているので、1.14.1に固定する

これあれですね、最新のmainを取ってきちゃってるからここが1.15.0になってるんですね!
こちらのVOICEVOX側のonnxruntimeはreleaseされたものをforkしてくる運用が普通なのかなと思いました。
なので1.14.1のcommit hashまでreset --hardしてforce pushするとややこしさが軽減できそうかも・・・?
方針これで問題なさそうだったらforce pushしようかなと思います!
(念頭から漏れてました、すみません 🙇 )

@qryxip
Copy link
Member Author

qryxip commented Mar 28, 2023

microsoft/onnxruntimeではmainブランチとリリース用ブランチが分離されており、タグv1.14.1にはRustバインディングが入っていません。またRustバインディングが未完成であることから、仮に今v1.14.2が出るとしてもまだ入らないと思います。
またmainブランチにmicrosoft#12606がマージされた時点で、このRustバインディングは1.15用に作られていたように見えました (本PRでコメントアウト入れているのは、1.15用のを1.14用のに戻す変更です)。

なのですみません、「1.14.1のcommit hashまでreset --hardして」の意味が掴めませんでした。

@qryxip
Copy link
Member Author

qryxip commented Mar 28, 2023

なのでこういう運用がいいのかなと思っています。
#1 (comment)

rust/onnxruntime-sys/build.rs Outdated Show resolved Hide resolved
rust/onnxruntime-sys/build.rs Show resolved Hide resolved
@qryxip
Copy link
Member Author

qryxip commented Mar 28, 2023

これを見るにこのリポジトリの外では動かないと思います。

let mut config = cmake::Config::new("../../cmake");

これ考えたら嘘ですね。dependencyをgitで指定したらGitリポジトリを丸ごと持って来るので、../../cmakeも含まれるはず。ただcompileにしてしまうとシングルスレッド設定でソースからonnxruntime.dllのビルドを始めるので、ビルド時間がとんでもないことになりますが...

@Hiroshiba
Copy link
Member

Hiroshiba commented Mar 28, 2023

なのですみません、「1.14.1のcommit hashまでreset --hardして」の意味が掴めませんでした。

ああ! すみません、かなり意図を把握できていませんでした!!

おそらく本家onnxruntimeにRustバインディングが入るのは1.15ですよね。
VOICEVOX側の方針として、コアが本家Rustバインディングを正式に使い始めるのはonnxruntime 1.15が出てからが良いのかなと思いました。
というのも正式リリースまで待ちたいのと、たぶんコアの0.15アプデより早く1.15が来そうなのでのんびり進めても大丈夫かなという感じです。

ということで、ORT_VERSIONinclude_str!("../../VERSION_NUMBER");のままでも良いのかなと思いました。
(戻し忘れが怖いので。。)

@qryxip
Copy link
Member Author

qryxip commented Mar 29, 2023

その場合../../VERSION_NUMBERを1.14.1にしないと、実際に1.15が出るまで手元で試しに動かしてみることも困難になるかと思います (どこからもダウンロード不可なので)。

@Hiroshiba
Copy link
Member

Hiroshiba commented Mar 29, 2023

あ~~~ なるほどです。なんでonnxruntime側がbuildがデフォルト挙動になっているのかわかりました。。

んじゃ VERSION_NUMBER ファイルを1.14.1にしておきますか!!
どちらにせよ修正忘れるかもですが、まあ最悪でも本家が1.15.1にアプデするときにコンフリクトして気づける気がします!

@qryxip
Copy link
Member Author

qryxip commented Apr 6, 2023

VERSION_NUMBERを使うようにしたらバージョン1.14.1\nを探し求めに行ってしまった... 😇

なんか$ORT_VERSION (onnxruntime本体のCMakeでも使われる)を読むようにするPRが出てるし、それでなんとかするという手も?
microsoft#14884
環境変数ならVOICEVOX/voicevox_core#223のときとかのように.cargo/config.tomlに設定してしまえば、VOICEVOX/voicevox_project#24を成し遂げるまではとりあえずなんとかなるんじゃないかと思っています。

@qryxip
Copy link
Member Author

qryxip commented Apr 6, 2023

あと$ORT_RUST_STRATEGYですが、compileがデフォルトのまま正式リリースされることはなさそうです。流石に。
microsoft#14935 (comment)

@qryxip
Copy link
Member Author

qryxip commented Apr 6, 2023

VERSION_NUMBERを使うようにしたらバージョン1.14.1**\n**を探し求めに行ってしまった... 😇

あ、単に0.15.0\n0.14.1という変更をすればいい?

@Hiroshiba
Copy link
Member

VERSION_NUMBERに関しては環境変数が一番いいなと思いました!!
とりあえずこちらのPRをマージして、ORT_VERSIONのとこを戻すというissueを建てておくとかはどうでしょう?

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.

2 participants