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

support iOS #19

Merged
merged 22 commits into from
Apr 15, 2023
Merged

support iOS #19

merged 22 commits into from
Apr 15, 2023

Conversation

HyodaKazuaki
Copy link

@HyodaKazuaki HyodaKazuaki commented Apr 3, 2023

iOSのアーキテクチャに対応します。

サポートするRustターゲット

  • aarch64-apple-ios
    • iOSの実機
  • aarch64-apple-ios-sim
    • Apple Silicon MacでのiOSシミュレータ
  • x86_64-apple-ios
    • Intel MacでのiOSシミュレータ

関連Issue

VOICEVOX/voicevox_core#459

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.

あ!! ちょっと認識違いが起こっていることに気づきました 🙇‍♂️

onnxruntime.dylibのビルドは、onnxruntime-builder側で行うのを想像していました。。
VOICEVOX/voicevox_core#459 (comment)

↑のコメントの続きで-rsの方を示されてることに気づきませんでした。。申し訳ない。。
VOICEVOX/voicevox_core#459 (comment)

onnxruntime-builderではbuild.yml内にてGithub Actionsでビルドを行っています。
既存コードとしてラズパイなど用のarmhfのビルドコードがあります。
https://github.com/VOICEVOX/onnxruntime-builder/blob/main/.github/workflows/build.yml

とはいえ、まあonnxruntime-rs側でビルドを行うのもありかもです。
将来的にonnxruntime-rsはonnxruntime純正のものを使う計画もあること、たぶんRustでビルドを書くよりshell scriptで書くほうがスリムなこともあり、メンテナ的にはbuilder側でビルドできると嬉しいのが本音です。

Github Actionsの練習がてらまあ良いか〜と思ってくださるのであればbuilderに移行を、あるいはとにかくiOSで動かせるか最速で試したい感じであればこのまま続行が良いのかなと思ったのですが・・・いかがでしょうか 🙇

.github/actions/auto_gen_bind_pr/action.yaml Outdated Show resolved Hide resolved
@HyodaKazuaki
Copy link
Author

あ!! ちょっと認識違いが起こっていることに気づきました 🙇‍♂️

onnxruntime.dylibのビルドは、onnxruntime-builder側で行うのを想像していました。。 VOICEVOX/voicevox_core#459 (comment)

↑のコメントの続きで-rsの方を示されてることに気づきませんでした。。申し訳ない。。 VOICEVOX/voicevox_core#459 (comment)

onnxruntime-builderではbuild.yml内にてGithub Actionsでビルドを行っています。 既存コードとしてラズパイなど用のarmhfのビルドコードがあります。 https://github.com/VOICEVOX/onnxruntime-builder/blob/main/.github/workflows/build.yml

とはいえ、まあonnxruntime-rs側でビルドを行うのもありかもです。 将来的にonnxruntime-rsはonnxruntime純正のものを使う計画もあること、たぶんRustでビルドを書くよりshell scriptで書くほうがスリムなこともあり、メンテナ的にはbuilder側でビルドできると嬉しいのが本音です。

Github Actionsの練習がてらまあ良いか〜と思ってくださるのであればbuilderに移行を、あるいはとにかくiOSで動かせるか最速で試したい感じであればこのまま続行が良いのかなと思ったのですが・・・いかがでしょうか 🙇

そうだったんですね...!私も勘違いしておりました、失礼しました。
ちなみに、armhfのビルドをこちらでビルドしているとのことですが、特にarmhfのビルドはonnxruntime-rs側で使っていないという認識で間違いないでしょうか?

ひとまずonnxruntime-builderへiOS向けビルドの移行を行います。
その後、移行PRを出してからこちらのPRも変更したいと思います。

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 6, 2023

特にarmhfのビルドはonnxruntime-rs側で使っていない

そうでした、使っていないんでした。。ちなみにRust移行のときに外れたんですが、まだタスクが残ってる感じでした。
VOICEVOX/voicevox_core#213 (comment)


すみません、いったんわかりやすいようにdraftにさせて頂こうと思います!

build on HyodaKazuaki/onnxruntime-builder to confirm working generate-bindings
@HyodaKazuaki
Copy link
Author

@Hiroshiba
https://github.com/VOICEVOX/onnxruntime-builder/pull/14でマージしていただいた命名ルールに対応しました。
ただ、現在はこのPRのブランチは1.13.1のものになっており、VOICEVOX/onnxruntime-builderにはまだiOS向けビルドがありません。
そのため、最新のコミットでgenerate-bindingsしようとしても失敗します。
一応、私の方でビルドしたものを使うように仕向けたコミットをee0f161に入れましたので、こちらで動作確認はできます。

このPRはこの後どのように進めればいいでしょうか?

@PickledChair
Copy link
Member

PickledChair commented Apr 10, 2023

ただ、現在はこのPRのブランチは1.13.1のものになっており、VOICEVOX/onnxruntime-builderにはまだiOS向けビルドがありません。
そのため、最新のコミットでgenerate-bindingsしようとしても失敗します。

横から失礼します。その通りですね!

コアのリポジトリの方の議論では同時にコアを onnxruntime v1.14.1 に対応させるという流れになっていたと思うので、まず先にその作業をすることになるのかなと思いました。 onnxruntime v1.14.0 を使うことになりました VOICEVOX/onnxruntime-builder#15

以下のような段階を踏むと良さそうかと思いました。もし間違いや考慮不足・説明不足などありましたらご指摘いただければ幸いです。

  • 別 PR で onnxruntime-rs をバージョン 1.14.1 1.14.0 に対応させ、先にマージ(参考: onnxruntimeを1.13.1に上げる #14
  • voicevox_core をバージョン 1.14.1 1.14.0 対応の onnxruntime-rs を使うように変更し、テストが通ることを確認する
    • 製品版もビルドしてプレリリースを行ない、ローカルで実際に使用できることを確認できるとなお良いかも
  • onnxruntime-builder の 1.14.1 のリリースをやり直して、iOS 向けバイナリを含むようにする onnxruntime-builder の 1.14.0 のリリースを作る
  • 本 PR に master ブランチをマージして作業の続きを行ない、完了させる
    • この作業中、 onnxruntimeを1.13.1に上げる #14 でもそうだったのですが、onnxruntime-sys crate の build.rs を変更するコミットを push すると、フォーク先で GitHub Actions を通して generate-bindings が行われ、自動で PR が作られるので、この作業ブランチにそれをマージすることで binding を追加できるはず
    • 更新:上記は permission の設定を行わないと失敗するようでした

@PickledChair
Copy link
Member

PickledChair commented Apr 10, 2023

最初の

別 PR で onnxruntime-rs をバージョン 1.14.1 に対応

について、試しに作業してみたのですが、onnxruntime-android の v1.14.1 バイナリが存在しない(公開されていない)ために、Android 向けの generate-bindings が失敗しますね……(実は iOS 向け v1.14.1 バイナリも CocoaPods では公開されていない。v1.14.0 向けには Android 版も iOS 版もバイナリが配布されている)。

@Hiroshiba これどうしましょうか……。Android 向けに v1.14.1 バイナリが公開される計画があるのかどうかがわかりません。

@PickledChair
Copy link
Member

PickledChair commented Apr 10, 2023

とりあえず思いつく代替案は、v1.14.1 ではなく v1.14.0 向けのバインディングを作ることですね

その場合、onnxruntime-builder の方も 1.14.1 をリリースし直すのではなく、1.14.0 のリリースを新たに作ることになります。

@Hiroshiba
Copy link
Member

Android

なるほどです。他のonnxruntime使いの人も困りそうなので、然るべき場所で報告してあげると良さそうかなと思いました!
こちらもAndroid版をビルドする手もありそうですが、全体的に1.14.0にするのが手っ取り早そうに思いました。

onnxruntime-builderがバージョン指定できる形になってなかったので、そこの改修が必要そうでした。

@Hiroshiba
Copy link
Member

このPRはこの後どのように進めればいいでしょうか?

@PickledChair さんが作ってくださった手順を進めつつ、もうbuilder側で1.14.1なり1.14.0なりを作成して、onnxruntime-rs側のiOS対応も進められると良いのかなと思いました!
とりあえずbuilder側で1.14.0作成と1.14.1の再作成しようと思います。ちょっとお時間頂いちゃうかもです。

@PickledChair
Copy link
Member

とりあえず onnxruntime-builder の方に Issue を建てておきました! VOICEVOX/onnxruntime-builder#15

@HyodaKazuaki
Copy link
Author

Android

なるほどです。他のonnxruntime使いの人も困りそうなので、然るべき場所で報告してあげると良さそうかなと思いました! こちらもAndroid版をビルドする手もありそうですが、全体的に1.14.0にするのが手っ取り早そうに思いました。

onnxruntime-builderがバージョン指定できる形になってなかったので、そこの改修が必要そうでした。

報告されていました。
microsoft/onnxruntime#15449

Release Noteにも記載されていましたが、1.14.1はPypiとNugetのみでリリースされたようです。
主にWindowsとPython向けのfixが行われたことが理由のようですね。

@Hiroshiba
Copy link
Member

おーーーー!! なるほどです。
こういうことがあるならターゲットごとにバージョン変更しても良いかも・・・?いやでもややこしくなりそうですね・・・。
とりあえず1.14.1版があると嬉しいことをissueにコメントしました。

こちらでの進行はとりあえず1.14.0で進めちゃって、Android版がリリースされたりしたらバージョン上げを検討するのが良いかもと思いました!
1.14.0はちょっと実行が遅い環境もある?っぽいので、正式リリースまでにはバージョン上げたいかもです。(忘れてそう)

@HyodaKazuaki
Copy link
Author

HyodaKazuaki commented Apr 13, 2023

onnxruntime-rsに1.14.0のonnxruntimeを使う変更 #20 がマージされたので、このPRにも取り込みました。

@HyodaKazuaki HyodaKazuaki marked this pull request as ready for review April 13, 2023 13:59
@HyodaKazuaki HyodaKazuaki marked this pull request as draft April 13, 2023 14:09
@HyodaKazuaki HyodaKazuaki marked this pull request as ready for review April 13, 2023 14:24
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です!!

onnxruntime-sys/build.rs Outdated Show resolved Hide resolved
onnxruntime-sys/build.rs Outdated Show resolved Hide resolved
onnxruntime-sys/build.rs Outdated Show resolved Hide resolved
Copy link
Member

@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.

試してませんがLGTM

@@ -416,6 +426,7 @@ impl OnnxPrebuiltArchive for Os {
Os::Linux => Cow::from("linux"),
Os::MacOs => Cow::from("osx"),
Os::Android => Cow::from("android"),
Os::IOs => Cow::from("ios"),
Copy link
Member

Choose a reason for hiding this comment

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

Rustの命名規則だとIosの方がいいんですが、2行上でnbigaouette氏(オリジナルのonnxruntime-rsの作者)がMacOsと書いちゃっているので統一感のためにここは致し方なし...?

In UpperCamelCase, acronyms and contractions of compound words count as one word

Copy link
Member

Choose a reason for hiding this comment

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

個人的には仕方ないかなと思いました・・・!
本当はMacosが良いんだろうなとも思います。が、まあ直しちゃうとコンフリクトが出てしまうので・・・ 😇

@qryxip qryxip requested a review from PickledChair April 14, 2023 15:16
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!!

@Hiroshiba Hiroshiba merged commit c322c6a into VOICEVOX:master Apr 15, 2023
@HyodaKazuaki HyodaKazuaki deleted the feature/support-aarch64-apple-ios branch April 15, 2023 03:22
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.

4 participants