-
Notifications
You must be signed in to change notification settings - Fork 49
Update Dockerfiles to rocm 7.0 #1991
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1991 +/- ##
===========================================
+ Coverage 79.50% 79.76% +0.26%
===========================================
Files 100 102 +2
Lines 31016 32361 +1345
Branches 4819 5104 +285
===========================================
+ Hits 24659 25811 +1152
- Misses 4245 4340 +95
- Partials 2112 2210 +98
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
||
# Use the rocm/mlir image as base | ||
FROM rocm/mlir:rocm6.4-latest | ||
FROM rocm/mlir:rocm7.0-latest |
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.
May I ask why this change is needed? Buildbot is actually stuck in 6.3, it's not even in 6.4. More background here: https://github.com/ROCm/rocMLIR-internal/issues/1979#issuecomment-3270870992
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 guess he replaced all rocm6.4 found in the project. Let's a add a comment here saying this should stay in 6.3 to avoid this happening in the future.
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.
So in conclusion I think this should be 6.3 and, at some point, should be updated to 7.0, for which we will need a newer machine to run the buildbot, see https://github.com/ROCm/rocMLIR-internal/issues/1981, so adding a comment here makes sense to me 👍
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 think we should discuss if changes on the buildbot Dockerfile are needed before merging this
@@ -1,9 +1,9 @@ | |||
FROM ubuntu:22.04 |
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.
do we want to use 24.04?
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.
Yes
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 am not sure though if that would lead to different python version and trigger more changes.
can we update dockerImage() and similar functions in Jenkins files? |
also, I think perfRunner.py and tuningRunner.py might have some issues. rocprofv3 needs "-f csv" at least on MI350, so I suspect it's needed for rocm7. |
I'll cover this in the next PR once the Docker image has been created and pushed |
I think we should run nightly or weekly CI? |
The build image job gets triggered after merging a PR that modifies the Dockerfile, so this one needs to be merged first. I'll add these checks in a new PR with Jenkinsfile changes. |
Motivation
Preparing CI Dockerfiles for rocm 7.0
Technical Details
Test Plan
Manually tested