-
Notifications
You must be signed in to change notification settings - Fork 17
feat: onnxruntime 1.23.0に対応 #90
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
base: main
Are you sure you want to change the base?
feat: onnxruntime 1.23.0に対応 #90
Conversation
[skip ci]
[skip ci]
[skip ci]
[skip ci]
[skip ci]
[skip ci]
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.
そういえば1.23.0が出るまではまだマージできませんね。まぁ仮置きとして。
あとこのPR、実は前の1.18のIssueで出てきたブランチの流用である関係で、一緒にCoreMLも作られてるんですよね。作らない理由もあんまりないので作っておいても良さそう?
|
LinuxのCUDAが今現在落ちてますが、それ以外は通っている & LinuxのCUDA版も動くと言って良いはずなのでとりあえずDraft解除。まだ1.23.0がリリースされていないのでマージはできませんが、まぁ一旦Ready for Reviewということで... 追記:通りました、https://github.com/sevenc-nanashi/onnxruntime-builder/actions/runs/16519003091 |
|
1.23.0がリリースされたのでタイトルを更新しました。 |
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.
Pull Request Overview
This PR updates ONNX Runtime support from version 1.17.3 to 1.23.0. The changes include removal of version-specific patches, build configuration updates, and enhanced CI workflow with new platform targets and device support.
- Updates default ONNX Runtime version from 1.17.3 to 1.23.0
- Removes obsolete patches for 1.17.3 (Windows, strip, and Android ARM64 build fixes)
- Expands CI build matrix with WebGPU and CoreML support across multiple platforms
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| 1_17_3_windows.patch | Removes Windows-specific mutex header patch no longer needed |
| 1_17_3_strip.patch | Removes binary stripping patch for build artifacts |
| 1_17_3_android_arm64_build.patch | Removes Android ARM64 cross-compilation fix |
| .github/workflows/build.yml | Updates version, adds new build targets, and refactors CI configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@qryxip もしお時間あればレビューぜひ・・・!! 🙏 📝 Copilot君結構活躍してくれた?っぽいので再割当て |
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| os=${{ matrix.spec_os }} | ||
| arch=${{ matrix.spec_arch }} | ||
| for arg in "${build_opts[@]}"; do | ||
| case "$arg" in | ||
| CMAKE_SYSTEM_NAME=Windows) os=Windows ;; | ||
| CMAKE_SYSTEM_NAME=Linux) os=Linux ;; | ||
| CMAKE_SYSTEM_NAME=Darwin) os=macOS ;; | ||
| --android) os=Android ;; | ||
| --ios) os=iOS ;; | ||
| CMAKE_SYSTEM_PROCESSOR=x86_64 | CMAKE_OSX_ARCHITECTURES=x86_64 | x86_64) arch=x86_64 ;; | ||
| --x86) arch=x86 ;; | ||
| CMAKE_OSX_ARCHITECTURES=arm64 | --arm64 | arm64 | arm64-v8a) arch=AArch64 ;; | ||
| --arm) arch=ARMv7 ;; | ||
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.
ここら辺のCMAKE_SYSTEM_NAMEみたいなやつですが、記憶が正しければこれらのいくつかはVVORTのために設定したような気がします。うろおぼえですが。
誤動作を起こさないのであれば無くていいと思います。まあ検証するのも面倒なので、一度消し飛ばしてVVORTのビルドが壊れたら足すというやりかたでもよいと思います。
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.
ここはビルド仕様の生成なのでvvortの誤作動云々とは関係ないはず?
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.
ぼかして書いたのが良くなかったですが、CorrosionのFindRust.cmakeについてですね。CMAKE_SYSTEM_NAMEみたいなものを突っ込まないとAndroidとかiOSだと、推定に失敗してた記憶です。まあちょっと記憶があやふやではあるんですが。
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.
ここはビルド仕様の生成
というのはそうで、matrix定義側の方にコメントすべきでした。
| ONNXRUNTIME_VERSION: | ||
| |- # workflow_dispatchでのバージョン名が入る。無指定なら適当なバージョン | ||
| ${{ inputs.version || '1.17.3' }} | ||
| ${{ inputs.version || '1.23.0' }} |
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.
そういえば3日前にv1.23.1が出たみたいです。
| os=${{ matrix.spec_os }} | ||
| arch=${{ matrix.spec_arch }} |
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.
ここの変数名は少し時間をかけて考えてもいいかもしれません。他に良いのが無ければspec_{os,arch}でいいとは思いますが。
CC: @Hiroshiba
spec_base: { os: "…", arch: "…" }みたいなのを用意してjqの式で+でマージ、という手もあるかもしれません。
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.
spec_osやspec_archはそもそも何書けば良いのかわからないなーと感じました。
・・・ビルドターゲットとするOSやarchのわかりやすい名称・・・?
であればtarget_osとかが分かりやすそう感。
spec_baseもまあtarget_specだとかtarget_infoだとかにしても良いかも。
まあ何に変えるかは置いといて、名前はちょっとややこしそうな気がしました!
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.
今思い付いたのですが、spec_table_key_base: { os: "…", arch: "…" }はどうでしょうか?これにdevicesを足せばUNIQUE KEYになるっってことで、spec tableのkeyのbase(?)ってことで…。
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.
そもそもspec系は仕様テーブルの生成でしか使われていないはずなので、target_osとかでもよさそう。
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.
ビルド自体に使う文字列なんじゃないかという疑問を持たれないかという懸念はありますが、まあ変数の利用箇所なんてコード検索(Vimならカーソル位置からvey/<C-r>"<C-j>)でもわかるでしょうし、認知不可を著しく上げるといったことは無さそうかも。
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.
そういえばこの話はどうしましょうか?spec_{os,arch}のまま?
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.
Co-authored-by: Ryo Yamashita <[email protected]>
内容
タイトル通りです。
関連 Issue
スクリーンショット・動画など
(なし)
その他
1.23が出るまではマージしないでください。