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

fixed rocm permission error and updated rocm containerfile #376

Closed
wants to merge 1 commit into from

Conversation

bmahabirbu
Copy link
Collaborator

@bmahabirbu bmahabirbu commented Oct 26, 2024

I updated the rocm container to include the ability to build with a selected GPU architecture. The original rocm container didnt work for me until I built it with the RDNA3 gfx code=1100 (supports 7800 and 7900).

I followed the steps in the build doc for hipblas
https://github.com/ggerganov/llama.cpp/blob/master/docs/build.md

Also I used-DGGML_HIPBLAS=ON instead of -DGGML_HIPBLAS=1 when using cmake. The container would build but llama.cpp would not utilize the gpu even with -ngl enabled.

Also, I'm currently running Ubuntu 24.04 and I ran into a peculiar issue where podman wasn't giving the correct permission for passing -device path inside the container. It turned out to be an issue with running in rootless. I followed this thread [https://github.com/containers/podman/issues/10166]
and added

--group-add keep-groups

and it worked! Not sure if this issue is unique to my environment. For this to work podman oci runtime should be crun. Not sure if runc still has this issue, I'll dig into this some more.

I'll write up a doc based on my troubleshooting. It might help speed debugging for others!

@ericcurtin
Copy link
Collaborator

DCO build failed @bmahabirbu for all commits in the https://github.com/containers namespace you need to sign your commits with git commit -s. You can amend and push previous commits with:

git commit --amend -s
git push -f

-DGGML_HIPBLAS=ON -DAMDGPU_TARGETS=${ROCM_DOCKER_ARCH} && \
cmake --build build --config Release -j$(nproc) && \
# Move whisper binaries to /usr/bin
mv build/bin/main /usr/bin/whisper-main && \
Copy link
Collaborator

@ericcurtin ericcurtin Oct 26, 2024

Choose a reason for hiding this comment

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

Might be simpler to do a:

cmake --install build

and do two mv's

haven't tried.

The reason the mv's are there is these two binaries are badly named. The llama.cpp equivalents used to also be named main and server, so you couldn't have both installed in the same directory because they have the same names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense I tried it with cmake --install build and then the mv's but it didnt work. could be the second cmake --install overrides it still

Copy link
Collaborator

@ericcurtin ericcurtin Oct 26, 2024

Choose a reason for hiding this comment

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

Maybe something like this:

+    cmake -B build -DCMAKE_INSTALL_PREFIX:PATH=/usr -DGGML_CCACHE=0 \
+      -DGGML_HIPBLAS=1 && \
+    cmake --build build --config Release -j $(nproc) && \
+    mv build/bin/main /usr/bin/whisper-main && \
+    mv build/bin/server /usr/bin/whisper-server && \
+    cmake --install build && \

the install doesn't cover the examples

@@ -16,23 +16,37 @@ RUN curl --retry 8 --retry-all-errors -o \
cat /etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-Official
RUN rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-Official

RUN dnf install -y rocm-dev hipblas-devel rocblas-devel && \
Copy link
Collaborator

@ericcurtin ericcurtin Oct 26, 2024

Choose a reason for hiding this comment

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

How does this work without the dnf install's for the ROCm libs/headers hmmm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apologies and good catch! I realized this just after making the PR! I decided to make a change manually to keep as close to main as possible but accidentally deleted those lines.


# Clean up
RUN rm -rf /var/cache/*dnf* /opt/rocm-*/lib/llvm \
Copy link
Collaborator

@ericcurtin ericcurtin Oct 26, 2024

Choose a reason for hiding this comment

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

This only makes sense if you run it in the same RUN statement as where these things are added, to reduce the size of the image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gotcha! I'll reconfigure my containerfile to have it all under the same RUN statement.

@bmahabirbu bmahabirbu force-pushed the rocm-rdna3-support branch 2 times, most recently from 67beadd to c0d6de1 Compare October 26, 2024 13:22
@@ -16,23 +16,28 @@ RUN curl --retry 8 --retry-all-errors -o \
cat /etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-Official
RUN rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-Official

# Set amd gpu architecture for RDNA3
# https://llvm.org/docs/AMDGPUUsage.html#processors
ENV AMDGPU_TARGETS=gfx1100
Copy link
Collaborator

@ericcurtin ericcurtin Oct 26, 2024

Choose a reason for hiding this comment

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

I wonder should we add like llama.cpp upstream containers if it doesn't cause issues:

gfx1010 \
gfx1030 \
gfx1100 \
gfx1101 \
gfx1102

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, I checked the llama.cpp repo docs and that's how the official rocm dockerfile does it

Copy link
Collaborator

@ericcurtin ericcurtin Oct 26, 2024

Choose a reason for hiding this comment

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

If you are curious why we leave out gfx9* ones, it's because the container images were getting humongous and we were hitting limits.

It would also take an age for users to download huge images, so we had to trim.

I wouldn't be against creating a rocm-gfx9 image in future to solve that problem though, if people complained their older AMD GPUs don't work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense, I hooked up an old radeon 5450 gfx1036 to my rig and edited the llama.cpp arguments to use it but it ran out of memory it only had 512 mb. I have a vega 64 lying around I can give that a shot if it fits into my pc!

In general @ericcurtin sorry for the PR mess I'm wondering once I confirm compatibility should I make a new PR?

@rhatdan
Copy link
Member

rhatdan commented Nov 1, 2024

@bmahabirbu still working on this one?

@bmahabirbu
Copy link
Collaborator Author

bmahabirbu commented Nov 1, 2024

@bmahabirbu still working on this one?

I'm all set with these changes!

Right now I'm looking into how InstructLab and Ollama create their respective container files and I'm hoping to integrate more useful changes

@rhatdan
Copy link
Member

rhatdan commented Nov 19, 2024

@bmahabirbu any update?

@bmahabirbu
Copy link
Collaborator Author

@bmahabirbu any update?

The Containerfiles for Ramalama have gotten really neat and the version I made here is messy comparatively.

I recently tested the updated rocm container on a new Fedora 40 Workstation install on my PC and I still needed to point the target to gfx1100 (which is a change I made in this PR) to correspond with my GPU architecture for it to work.

For now, we can close this PR and I can make a new one adding those architectures to the new rocm containerfile.

@rhatdan rhatdan closed this Nov 19, 2024
@bmahabirbu bmahabirbu deleted the rocm-rdna3-support branch December 10, 2024 03:29
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.

3 participants