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: add vulkan images #22

Merged
merged 4 commits into from
Jul 29, 2024
Merged

feat: add vulkan images #22

merged 4 commits into from
Jul 29, 2024

Conversation

lstocchi
Copy link
Contributor

What does this PR do?

libkrun provider has been added to podman and soon we should have a way to start an inference server that is able to use gpu on a mac. This PR adds the creation of vulkan images so that, eventually, we can use them to start an inference server with gpu capabilities.

Screenshot / video of UI

N/A

What issues does this PR fix or reference?

N/A

How to test this PR?

  1. check that the image build correctly

@lstocchi lstocchi requested a review from a team as a code owner June 27, 2024 09:13
@lstocchi lstocchi requested review from benoitf, feloy and jeffmaury June 27, 2024 09:13
chat/vulkan/amd64/Containerfile Outdated Show resolved Hide resolved
Comment on lines 3 to 9
RUN dnf install -y python3-dnf-plugin-versionlock
RUN dnf install -y mesa-vulkan-drivers-23.3.3-1.el9.x86_64
RUN dnf versionlock mesa-vulkan-drivers-23.3.3-1.el9.x86_64
RUN dnf install -y https://dl.fedoraproject.org/pub/epel/epel-release-latest-9.noarch.rpm
RUN dnf install -y git cmake ninja-build gcc gcc-c++
RUN dnf copr enable -y ligenix/enterprise-sandbox epel-9-x86_64
RUN dnf install -y vulkan-headers vulkan-tools
Copy link
Contributor

Choose a reason for hiding this comment

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

as it looks like it's not a multi-stage build, you should mergre layers and cleanup .rpm files / rpm cache at the end to reduce the size of the image

or maybe better, use the default image and copy the custom rootfs so we've a scratch/smaller image (this is how ubi micro is done)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been trying to copy what they did on ubi-micro but i cannot make it work with installroot. There is something else to set up but i was not able to find it.
I went to the good old way to check which folders to copy and i added the commands. If you know how to make the custom rootfs works, i'm all ears

chat/vulkan/arm64/Containerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

The one from ai-lab-recipes uses library krunkit specific ?

@lstocchi
Copy link
Contributor Author

The one from ai-lab-recipes uses library krunkit specific ?

It is primarily intended to be used on macOS with podman and krunkit but Sergio also said that it can be used on Linux with a VMM exposing a Venus-capable virtio-gpu.

@jeffmaury
Copy link
Contributor

The one from ai-lab-recipes uses library krunkit specific ?

It is primarily intended to be used on macOS with podman and krunkit but Sergio also said that it can be used on Linux with a VMM exposing a Venus-capable virtio-gpu.

Did you check that performances with this images are the same compared to the benchmarks we did on MacOS/krunkit ?

@lstocchi
Copy link
Contributor Author

Did you check that performances with this images are the same compared to the benchmarks we did on MacOS/krunkit ?

What do you mean? Didn't we use the image generated by the ai-lab-recipes team for the benchmark? I just copy/paste the containerfile so the result should be the same, no?

@jeffmaury
Copy link
Contributor

Did you check that performances with this images are the same compared to the benchmarks we did on MacOS/krunkit ?

What do you mean? Didn't we use the image generated by the ai-lab-recipes team for the benchmark? I just copy/paste the containerfile so the result should be the same, no?

Does not seem to me as the ai-lab-recipe one contains libkrun specific libraries: see https://github.com/containers/ai-lab-recipes/blob/624155c699b7c2abc3c5878fba21ad848b23af25/model_servers/llamacpp_python/vulkan/arm64/Containerfile#L6

build-image-name: "ai-lab-playground-chat"
archs: amd64, arm64
- containerfile: "./chat/vulkan/amd64/Containerfile"
build-image-name: "ai-lab-playground-chat-vulkan-amd64"
Copy link
Contributor

Choose a reason for hiding this comment

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

The image name should not contain the arch so this should be handled through a manifest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are two versions of the same image. One for amd and one for arm. We should use the one we need based on the system

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but we should select based on the arch not the name just like the base image (CPU)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the arch from the name. is that enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops, updated the other file :face-palm:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@lstocchi lstocchi requested review from jeffmaury and benoitf June 28, 2024 14:24
@@ -32,8 +32,13 @@ env:

jobs:

build:
tag:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this extra job ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that separating it in various jobs was more clean. Updated to merge all jobs into one as before

@lstocchi lstocchi force-pushed the addVulkan branch 3 times, most recently from e529d18 to 7d9e218 Compare July 23, 2024 07:52
@lstocchi lstocchi requested a review from jeffmaury July 23, 2024 07:54
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM

lstocchi added 4 commits July 29, 2024 16:18
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
@jeffmaury jeffmaury merged commit ef689a9 into containers:main Jul 29, 2024
4 checks passed
@lstocchi lstocchi deleted the addVulkan branch August 6, 2024 10:33
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