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

perf(docker): reduce prebuilt docker image size #2922

Conversation

Sharrrrk
Copy link
Contributor

@Sharrrrk Sharrrrk commented Oct 6, 2022

Signed-off-by: Shark Liu shark.liu@autocore.ai

Description

Solve issue #2840

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@kenji-miyake
Copy link
Contributor

kenji-miyake commented Oct 6, 2022

@Sharrrrk Thank you so much for this improvement!
Would it be possible for you to split install_devel-related changes into a separate PR because it's feat?
Like feat(setup): add an option to install dev packages.

@Sharrrrk
Copy link
Contributor Author

Sharrrrk commented Oct 6, 2022

@Sharrrrk Thank you so much for this improvement! Would it be possible for you to split install_devel-related changes into a separate PR because it's feat? Like feat(setup): add an option to install dev packages.

Sure, I'll create a separate PR.

@Sharrrrk
Copy link
Contributor Author

Sharrrrk commented Oct 6, 2022

@kenji-miyake Would it be possible that you could do a test of this script before merging. Thanks.

@kenji-miyake
Copy link
Contributor

@Sharrrrk Yes, I'll trigger GitHub Actions.

amd64.env Outdated Show resolved Hide resolved
@doganulus
Copy link

Can we test this PR? What's the status?

@kenji-miyake
Copy link
Contributor

Can we test this PR?

@doganulus What do you mean by the word "test"?
To check the workflow, we can trigger it manually.
To try out the generated image, you can build it locally, or you can fork and try this PR.

What's the status?

I'm waiting for the review comments to be applied.
And I'm planning to check this in more detail, but it's difficult for me to spare time for a while. I'm sorry for that, but please wait for a bit more.

@doganulus
Copy link

doganulus commented Oct 29, 2022

By testing, I mean using the resulting image, for example, with the AWSIM simulator or the scenario simulator. I think a runtime image as in this PR would be an improvement for these use cases. Thank you for the explanation!

@xmfcx
Copy link
Contributor

xmfcx commented Dec 13, 2022

Following the discussion here: autowarefoundation/autoware-documentation#270

Is it possible to keep the prebuilt image as it is, and add this runtime/release image separately?

@kenji-miyake
Copy link
Contributor

Is it possible to keep the prebuilt image as it is, and add this runtime/release image separately?

@xmfcx Yes, it's possible. But could you explain some reasons/use cases for that, if possible?
Since Autoware changes every day, the prebuilt artifacts cannot be reused generally.

@xmfcx
Copy link
Contributor

xmfcx commented Dec 16, 2022

Is it possible to keep the prebuilt image as it is, and add this runtime/release image separately?

@xmfcx Yes, it's possible. But could you explain some reasons/use cases for that, if possible? Since Autoware changes every day, the prebuilt artifacts cannot be reused generally.

Ok, since @cyn-liu agreed to adapt the training video to take this into account in autowarefoundation/autoware-documentation#270 (comment)

We don't need to include build artifacts in the new prebuilt image. I take back my request.

@liuXinGangChina
Copy link

Hi, @kenji-miyake .
Can we merge this pr?

@kenji-miyake
Copy link
Contributor

@liuXinGangChina cc @Sharrrrk As we moved to Humble, there are conflicts. Could you fix them?
image

Also, I didn't notice that there were changes after my last review. I'm sorry for that, but I'd appreciate it if you re-request a review by clicking the button.
image
image

So, now do you mean that you've addressed my comments and it's ready to merge?
In that case, I'm sorry for my late work, but I'll manage to make time for a review this week.

@Sharrrrk
Copy link
Contributor Author

Sharrrrk commented Feb 1, 2023

Hi, @kenji-miyake , the conflicts is about cuda_base_image, as it has been set to ubuntu:22.04 on main branch, but nvidia/cuda dit not has docker image match the cuda:11.6 + ubuntu22.04. Thank you so much if you could give some advice.

<<<<<<< 2840-reduce-prebuilt-docker-image-size
base_image=ubuntu:20.04
cuda_base_image=nvidia/cuda:11.6.2-runtime-ubuntu20.04

base_image=ubuntu:22.04
cuda_base_image=ubuntu:22.04

main
cuda_version=11.6
cudnn_version=8.4.1.50-1+cuda11.6
tensorrt_version=8.4.2-1+cuda11.6

@kenji-miyake
Copy link
Contributor

@Sharrrrk I'm sorry to be late.
We need to upgrade the CUDA version to 11.8 or 12.0 to use a CUDA image for the base image. However, since there is no arm64 (sbsa) version provided yet, we can't upgrade the version.
See also #3084 (comment).

So, until then, I think using ubuntu:22.04 for the base image is okay.

@Sharrrrk
Copy link
Contributor Author

Sharrrrk commented Feb 8, 2023

@Sharrrrk I'm sorry to be late. We need to upgrade the CUDA version to 11.8 or 12.0 to use a CUDA image for the base image. However, since there is no arm64 (sbsa) version provided yet, we can't upgrade the version. See also #3084 (comment).

So, until then, I think using ubuntu:22.04 for the base image is okay.

Do you mean we should set both base_image and cuda_base_image to ubuntu:22.04 for now?

@kenji-miyake
Copy link
Contributor

@Sharrrrk Yes, exactly. It will be resolved in #3084, but arm64 TensorRT packages aren't released yet. We have to wait for that.

Copy link
Contributor

@kenji-miyake kenji-miyake left a comment

Choose a reason for hiding this comment

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

@Sharrrrk Thank you for updating the PR, and I'm sorry for my late review.
Added additional comments, could you confirm them, please?

docker/build.sh Outdated Show resolved Hide resolved
docker/autoware-universe/Dockerfile Show resolved Hide resolved
docker/autoware-universe/Dockerfile Show resolved Hide resolved
docker/autoware-universe/Dockerfile Outdated Show resolved Hide resolved
@Sharrrrk Sharrrrk force-pushed the 2840-reduce-prebuilt-docker-image-size branch 4 times, most recently from aa3a80d to 91edbca Compare February 20, 2023 08:53
@Sharrrrk Sharrrrk force-pushed the 2840-reduce-prebuilt-docker-image-size branch 3 times, most recently from aa6c3e3 to 5be9c70 Compare February 21, 2023 03:10
@Sharrrrk Sharrrrk force-pushed the 2840-reduce-prebuilt-docker-image-size branch from 5be9c70 to 619c851 Compare February 22, 2023 05:40
setup-dev-env.sh Outdated Show resolved Hide resolved
@Sharrrrk Sharrrrk force-pushed the 2840-reduce-prebuilt-docker-image-size branch from 619c851 to 7700c90 Compare February 28, 2023 03:31
setup-dev-env.sh Outdated Show resolved Hide resolved
setup-dev-env.sh Outdated Show resolved Hide resolved
Shark Liu and others added 10 commits March 2, 2023 15:25
Signed-off-by: Shark Liu <shark.liu@autocore.ai>
Signed-off-by: Shark Liu <shark.liu@autocore.ai>
Signed-off-by: Shark Liu <shark.liu@autocore.ai>
Signed-off-by: Shark Liu <shark.liu@autocore.ai>
Signed-off-by: Shark Liu <shark.liu@autocore.ai>
Signed-off-by: Shark Liu <shark.liu@autocore.ai>
Signed-off-by: Shark Liu <shark.liu@autocore.ai>
Signed-off-by: Shark Liu <shark.liu@autocore.ai>
… ros2_dev_tools

Signed-off-by: Shark Liu <shark.liu@autocore.ai>
@Sharrrrk Sharrrrk force-pushed the 2840-reduce-prebuilt-docker-image-size branch 2 times, most recently from 9ab67f4 to 542fadf Compare March 2, 2023 07:42
Signed-off-by: Shark Liu <shark.liu@autocore.ai>
@Sharrrrk Sharrrrk force-pushed the 2840-reduce-prebuilt-docker-image-size branch from 542fadf to 151eb76 Compare March 2, 2023 07:50
Copy link
Contributor

@kenji-miyake kenji-miyake left a comment

Choose a reason for hiding this comment

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

@Sharrrrk I believe everything is okay. Thank you for your work and patience.

@kenji-miyake kenji-miyake merged commit 3555383 into autowarefoundation:main Mar 2, 2023
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.

None yet

6 participants