-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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): re-organize the autoware docker containers #4072
Conversation
@oguzkaganozt Could you also create PR to autoware-documentation to make sure that install instructions for Docker is valid with this PR? |
0f500ad
to
9f1f3ff
Compare
Requested changes have been made
The code looks okay to me. I have noticed that you have tested the action and it seems passing: https://github.com/autowarefoundation/autoware/actions/runs/7905813792 We can test with autoware.universe and other repositories to see if they can use the docker image for the CIs. |
I just noticed that update-docker-manifest is failing. It might be due to docker images that were pushed manually for CES demo... https://github.com/autowarefoundation/autoware/actions/runs/7903383046/job/21571162050 |
@oguzkaganozt let me know if this is this ready for another review. I saw that there are some conflicts with |
0183ec2
to
5fd8757
Compare
@esteve Should be resolved, please review now 👍 |
I would like to merge this once this last wave of reviews are resolved. Thanks @oguzkaganozt @ambroise-arm @esteve @mitsudome-r for your reviews and support for this task! |
Signed-off-by: oguzkaganozt <[email protected]>
Signed-off-by: oguzkaganozt <[email protected]>
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.
Made some last changes according to reviews
@ambroise-arm could you confirm if you are happy with the changes? |
I've confirmed that the the images generated by this PR performs well on the Autoware Universe side. |
@@ -0,0 +1,14 @@ | |||
FROM ghcr.io/autowarefoundation/autoware-openadk:latest-devel-cuda |
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.
If apart from this line the rest of the file is exactly the same as in base/Dockerfile then I think it would be better to keep a single Dockerfile under .devcontainer/ and instead have an ARG TAG
or something like that in order to use FROM ghcr.io/autowarefoundation/autoware-openadk:$TAG
and then pass it with build.args
(https://containers.dev/implementors/json_reference/#image-specific) in the respective devcontainer.json
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.
@oguzkaganozt will look into this tomorrow in a follow up PR.
Let's merge this as it is today.
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.
ok
Signed-off-by: M. Fatih Cırıt <[email protected]>
Signed-off-by: M. Fatih Cırıt <[email protected]>
"hostRequirements": { | ||
"gpu": true | ||
}, |
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.
This can be removed for this non-GPU file. Although it is not critical because if I remember correctly it just issues a warning in the logs, but doesn't block anything.
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.
resolving following #4072 (comment)
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.
I'm not good at VSCode with Containers, I don't know if this affects anything related to RViz. If you think we can remove it safely, I can remove it.
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.
It won't change how things get setup. From https://containers.dev/implementors/json_reference/#min-host-reqs: "you will be presented with a warning if the requirements are not met". Removing this won't affect whether rviz works or not.
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.
@ambroise-arm I see, I will let @oguzkaganozt know this, let's remove it as well in the next PR.
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.
Now it's time for the big merge 🥁🥁🥁🥁
Thanks everyone again for your invaluable comments and efforts! |
I would like to thank everyone again for all of the precious reviews and comments ! |
Signed-off-by: Kotaro Yoshimoto <[email protected]>
@oguzkaganozt And I tried a quick fix, but the build step in |
Description
Re-organize the Autoware Docker Containers for:
Related PRs:
Related links
Tests performed
Notes for reviewers
Some changes might interfere with some Github Actions
Interface changes
No interface changes
Effects on system behavior
No system effect, CI-CD and container runtime improvements made
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.