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

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

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

oguzkaganozt
Copy link
Contributor

@oguzkaganozt oguzkaganozt commented Apr 30, 2024

Description

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

  • Move monilithic autoware images into autoware package and, openadkit images into autoware-openadk package
  • Provide planning, simulator, visualizer container images
  • Provide docker-compose file for running autoware with multiple images easily

Detailed information can be found at the below related issue.

Related links

#4538

Tests performed

docker build action: https://github.com/autowarefoundation/autoware/actions/runs/9177235488
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.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

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.
  • The PR is ready for merge.

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

@oguzkaganozt oguzkaganozt added component:system System design and integration. component:planning Route planning, decision-making, and navigation. DevOps Dojo: Build & Run type:ci Continuous Integration (CI) processes and testing. labels Apr 30, 2024
@oguzkaganozt oguzkaganozt self-assigned this Apr 30, 2024
@youtalk
Copy link
Member

youtalk commented May 7, 2024

@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
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Member

youtalk commented May 20, 2024

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 oguzkaganozt force-pushed the 4538-provide-modular-images-for-openadkit-planning-simulator-visualizer branch from 9f002d5 to 6199c9c Compare May 20, 2024 11:35
@oguzkaganozt
Copy link
Contributor Author

@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 oguzkaganozt force-pushed the 4538-provide-modular-images-for-openadkit-planning-simulator-visualizer branch from d998572 to 69b7859 Compare May 22, 2024 14:12
@oguzkaganozt
Copy link
Contributor Author

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 and others added 14 commits May 23, 2024 13:14
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
.
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
oguzkaganozt and others added 15 commits May 23, 2024 13:14
.
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
.
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
.
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
.
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
.
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
@oguzkaganozt oguzkaganozt force-pushed the 4538-provide-modular-images-for-openadkit-planning-simulator-visualizer branch from 363229f to cbc9263 Compare May 23, 2024 13:15
Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

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

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.

.github/workflows/docker-build-and-push-main.yaml Outdated Show resolved Hide resolved
.github/actions/docker-build-and-push/action.yaml Outdated Show resolved Hide resolved
docker/docker-bake.hcl Show resolved Hide resolved
docker/build.sh Outdated Show resolved Hide resolved
docker/openadkit/etc/ros_entrypoint.sh Outdated Show resolved Hide resolved
docker/openadkit/etc/ros_entrypoint.sh Show resolved Hide resolved
docker/openadkit/modules/planning-control/Dockerfile Outdated Show resolved Hide resolved
docker/run.sh Show resolved Hide resolved
@oguzkaganozt
Copy link
Contributor Author

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.

Signed-off-by: Oguz Ozturk <oguzkaganozt@gmail.com>
oguzkaganozt and others added 2 commits May 24, 2024 18:19
@oguzkaganozt
Copy link
Contributor Author

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
Copy link
Member

youtalk commented May 25, 2024

#4771 reduces the image size. Please merge the latest main branch and check the CI again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:planning Route planning, decision-making, and navigation. component:system System design and integration. DevOps Dojo: Build & Run type:ci Continuous Integration (CI) processes and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants