search

autowarefoundation / autoware

Autoware - the world's leading open-source software project for autonomous driving
https://www.autoware.org/
Apache License 2.0
8.58k stars 2.88k forks source link

feat(docker): provide modular images for openadkit planning simulator visualizer #4673

Open oguzkaganozt opened 2 months ago

oguzkaganozt commented 2 months ago

Description

Provide modular planning, simulator and visualizer containers to enable easier development and deployment of Autoware functionalities with distinct needs.

Related links

https://github.com/autowarefoundation/autoware/issues/4538

Tests performed

docker build action: https://github.com/autowarefoundation/autoware/actions/runs/9254434490 New docker images are thoroughly tested in a fresh environment.

Notes for reviewers

Interface changes

N/A as it will only introduce new modular container images.

Effects on system behavior

N/A to bare-metal setups, but it will provide new modular deployment scheme.

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.

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

youtalk commented 1 month ago

@oguzkaganozt It is good PR but quite a big changes to review it.

To streamline the review process, please minimize the amount of revisions per PR and break it up into multiple PRs. In addition, for both maintenance and build efficiency, a single Dockerfile should be used with multi-stage build instead of creating multiple Dockerfiles. ref https://github.com/orgs/autowarefoundation/discussions/4661

oguzkaganozt commented 1 month ago

@oguzkaganozt It is good PR but quite a big changes to review it.

To streamline the review process, please minimize the amount of revisions per PR and break it up into multiple PRs. In addition, for both maintenance and build efficiency, a single Dockerfile should be used with multi-stage build instead of creating multiple Dockerfiles. ref https://github.com/orgs/autowarefoundation/discussions/4661

Thank you for the review @youtalk. You're right on big PR changes but since this PR includes many tightly-related changes I though that it will be more easier to review in all-in-one PR.

For the multi-stage build we are already utilizing it in the monolithic images of Autoware devel and runtime. And these new modules also utilizing the base image from the monolithic images (which includes ros, rmw and some basic dependencies) but we still need to keep module dockerfiles separate because ideally in the near future we will only copy related nodes into specific dockerfile, which means reducing the size of the modules even better. (At this time, this PR copying all of the install folder into all modules which is not ideal).

mitsudome-r commented 1 month ago

@oguzkaganozt Could you make modification to the documentation accordingly as well? I think the path for run scripts and build scripts must be changed on this page.

youtalk commented 1 month ago

I think the renaming refactoring and multi-stage build PRs should be done separately. If we do the renaming first, the diff would be smaller.

oguzkaganozt commented 1 month ago

@oguzkaganozt Could you make modification to the documentation accordingly as well? I think the path for run scripts and build scripts must be changed on this page.

@mitsudome-r we don't need to update any documentation for this PR anymore, as right now it is only adding openadkit directory which only contains modular images.

oguzkaganozt commented 1 month ago

I think the renaming refactoring and multi-stage build PRs should be done separately. If we do the renaming first, the diff would be smaller.

I know the changes look like a lot, but as I said this work is tightly coupled and includes the all work that previously demonstrated at the last CES. Creating separate PRs would be ideal but the time timeline is tight for these features to make users to replicate the last demo.

oguzkaganozt commented 1 month ago

I tried to review it, but the diff is too large to write review comments easily. Although it is divided into multiple images, it looks very wasteful because the same vcs import and rosdep install are repeated in various Dockerfiles. Also, the Dockerfiles other than the simulator are almost the same in content.

@youtalk at this stage of development my main focus was to just introduce these 3 different modules with demonstration of planning scenario. You're right that there are not much of diff between these dockerfiles but in the near future after defining the perception-localization container there will be.

Refining of each module's dockerfiles will be done on top of that, also we can leverage src-imported layer in the next PR of modular images. But at the time being intention was to upstream planning-control, simulator and visualizer containers and enable users to run planning scenarios with modular container setting with the help of this docker-compose file.

oguzkaganozt commented 1 month ago

Removed the upload artifacts section from the workflow because of the disk space after resolving the issue we can re-enable them. @youtalk @mitsudome-r

youtalk commented 1 month ago

https://github.com/autowarefoundation/autoware/pull/4771 reduces the image size. Please merge the latest main branch and check the CI again.

youtalk commented 1 month ago

Removed the upload artifacts section from the workflow because of the disk space after resolving the issue we can re-enable them. @youtalk @mitsudome-r

@oguzkaganozt You were saying the opposite https://github.com/autowarefoundation/autoware/pull/4738#issuecomment-2124790959. So I think we can't remove the Upload Artifact.

Uploading of images is necessary because you can test out the produced images without pushing them into the registry.

oguzkaganozt commented 1 month ago

Removed the upload artifacts section from the workflow because of the disk space after resolving the issue we can re-enable them. @youtalk @mitsudome-r

@oguzkaganozt You were saying the opposite #4738 (comment). So I think we can't remove the Upload Artifact.

Uploading of images is necessary because you can test out the produced images without pushing them into the registry.

@youtalk Since it's stepping on our feet with various features I think we can re-enable them after we solve the disk space issue for now. I just don't want to slow down our development process for now, but eventually, we will need them soon.

oguzkaganozt commented 1 month ago

@mitsudome-r @youtalk Here is the build-action -> https://github.com/autowarefoundation/autoware/actions/runs/9254434490

youtalk commented 1 month ago

I think @oguzkaganozt is continue committing. So I changed this PR to the draft. If completed, Please press Ready for review.

youtalk commented 1 month ago

This PR includes too many changes and will take a lot of time to merge, so I have split out a subset PR for only renaming. https://github.com/autowarefoundation/autoware/pull/4785

oguzkaganozt commented 1 month ago

This PR includes too many changes and will take a lot of time to merge, so I have split out a subset PR for only renaming. #4785

Thanks @youtalk i really didn't have the time to do this as i am busy with demo prep. Now that I have merged main into this PR and the diff should be more human-readable :) @mitsudome-r

youtalk commented 1 month ago

I have split out a subset PR for only disabling Upload Artifacts. https://github.com/autowarefoundation/autoware/pull/4789

youtalk commented 1 month ago

@oguzkaganozt I understand that you need to use this for a demonstration, but as I've pointed out several times, this PR includes too many changes that should be separated. Therefore, the PR can never be made merge-ready.