-
Notifications
You must be signed in to change notification settings - Fork 203
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
add: DockerHub で提供されているイメージに arm 版を追加 #639
Conversation
アーキテクチャごとに workflow の build_args を切り替えることができればもっとスマートだったんでしたが、、 |
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.
おーーーー docker buildにplatformsを指定でき、しかもそれを同じタグにpushできることを知りませんでした!!!
ただこうするとdockerfile内でonnxruntime_urlを作る必要があり、バイナリビルドしている build.yml 側とコードがずれてしまうことに気づきました。
コードを同じような形にするには、1つのjob内で2プラットフォームビルドするのを、2つのjobで1つずつビルドしてpushできる必要がありそうなのですが、可能かご存知でしょうか・・・?
まあでも問題ではないと思うので、頂いたPRの方針で良いかも。
@y-chan さん的にどうでしょう 👀
あ! すでに質問の部分は答えてくださってました、すみません! |
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.
現在のPR方針で問題ないと思います...!
特に問題も見当たらなさそうだったので、LGTMとしたいと思います!
コードを同じようにかつ 2つ job(arm64, amd64) をくっつけるという方針のパターンを一つ考えてみました。 検証 PR job が分かれていい感じになりますが、デメリットもあり dockerhub 側で 〇〇-arm64 〇〇-amd64 というイメージが置かれてしまうところです。(中間イメージ的なの) 前の方針で良いとかあれば全然 Revert します |
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.
おーー manifestってこういう機能用だったんですね!!
とても勉強になります!!!
頑健な一方、キャッシュを作る必要があるといった課題もあるのかなという感想です。
(まあもともとbuildcacheがありますが)
利点欠点並べてみます。
- 利点
- URLの差などに頑健
- Dockerfileがそのまま
- 欠点
- 中間的なdocker imageを作る必要がある
- コード変更量が多くなる
- アーキテクチャとURLに不整合が起こり得る
うーーーーーん!!
すごく難しい判断でどちらも素晴らしいのですが、利点が多そうな前回の方法が良いのかなと思いました・・・ 🙇♂️
せっかくPR頂いたのでいろいろコメントしていますが、revertをお願いしてもよろしいでしょうか。。
将来、今のPRの方針でいくこのになった場合の参考になるので、resetではなくrevertだととても嬉しいです…🙇♂️
ほんとに申し訳ないです🙇♂️
.github/workflows/build-docker.yml
Outdated
base_image: ubuntu:18.04 | ||
base_runtime_image: ubuntu:18.04 | ||
voicevox_core_asset_prefix: voicevox_core-linux-x64-cpu | ||
onnxruntime_url: https://github.com/microsoft/onnxruntime/releases/download/v1.14.1/onnxruntime-linux-aarch64-1.14.1.tgz |
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.
バージョン合わせておくと環境ごとの差異なくせて良いかも?
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.
完全に僕の転記ミスです🙇♂️
.github/workflows/build-docker.yml
Outdated
target: runtime-env | ||
base_image: ubuntu:18.04 | ||
base_runtime_image: ubuntu:18.04 | ||
voicevox_core_asset_prefix: voicevox_core-linux-x64-cpu |
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.
arm64かもです
.github/workflows/build-docker.yml
Outdated
target: runtime-env | ||
base_image: ubuntu:20.04 | ||
base_runtime_image: ubuntu:20.04 | ||
voicevox_core_asset_prefix: voicevox_core-linux-x64-cpu | ||
onnxruntime_url: https://github.com/microsoft/onnxruntime/releases/download/v1.13.1/onnxruntime-linux-x64-1.13.1.tgz | ||
platforms: linux/amd64 | ||
- tag: cpu | ||
tag_platform_prefix: arm64 |
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.
最後につけてるのでpostfixかも。
あと「プットフォームのタグのpostfix」とすると意味が通りやすいので、platform tag postfixにすると他と揃うかも…?(微妙かも……)
あとそういえばnvidia+arm64のペアが省かれていますが、これは基本になるdocker imageがないとかでしょうか 👀 |
arm 版 onnxruntime リリースが見当たらなかったためです、ドキュメントを確認してもはっきり動くと書いていませんでした。 |
This reverts commit b41ce05.
レビューありがとうございました!!ご指摘の通り、revert しました。
manifest に関してはテキストの情報(josn)をくっつけているだけっぽくなので特にキャッシュをしなくても速いといった感想です。(下記画像で37s) |
一応関連っぽいのでコメント #322 |
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!!
revertすみません、ありがとうございます🙇♂️
arm64とcudaが無いのはonnxruntime側でしたか!!
把握できてませんでした、なるほどです。
たしかにmanifest作成はたしかにめちゃめちゃ早そうですね!!
アーキテクチャごとにimageがpushされて、一時ファイルが残るため検索性が落ちちゃうのを気にしていました。
Actions側でimageをまとめてからpushするとかできそうですが、それならplatformで判定する今の形のが綺麗だよなーと…!
マージします! PRありがとうございました、もしよければまたお待ちしています!!
|
内容
関連 Issue
close #633
スクリーンショット・動画など
その他