Skip to content
This repository has been archived by the owner on Sep 30, 2023. It is now read-only.

Change from alpine to ubuntu in Dockerfile.default #60

Merged
merged 3 commits into from
Aug 26, 2023

Conversation

nvtienanh
Copy link
Contributor

I got error similar to #51
When I try to use ubuntu instead of alpine it can work.
And we should use ubuntu based image for consistent with GPU version.

Copy link
Owner

@ravenscroftj ravenscroftj left a comment

Choose a reason for hiding this comment

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

Thanks for your feedback.

The reason I didn't use ubuntu originally was because I wanted to keep the size of the containers as small as possible. As you can see below my build of your PR is about 4x bigger than the build with alpine:
image

However, I do take your point that it is probably better to be consistent.

There are a lot of variables that could explain why building with ubuntu works for you when using the alpine build doesn't. It is possible that the MUSL C runtime from alpine causes issues though.

Ultimately 80MB is still pretty small image size and the models make the image size pretty much irrelevant anyway - they are 10-100X the size so I think the other benefits probably outweight the size thing.

I just have one change request that I've noted and then I'll merge.


RUN mkdir /turbopilot/build

WORKDIR /turbopilot/build

RUN cmake -D GGML_STATIC=ON ..
RUN cmake -DGGML_OPENBLAS=ON ..
Copy link
Owner

Choose a reason for hiding this comment

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

This is a significant change - I do not want to enable openblas by default for the normal docker image as OpenBlas support is still unstable and is not guaranteed to speed up computation for all models and users.

If we're shipping with ubuntu there's no advantage to having a static build either so I think this line should be

RUN cmake ..

I am open to adding an openblas flavour of the docker build to the CI as well. I might simplify the docker build process and use a single file with different docker build args.

Copy link
Owner

@ravenscroftj ravenscroftj left a comment

Choose a reason for hiding this comment

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

Looks good to me thank you for your contribution

@ravenscroftj ravenscroftj merged commit f840ea0 into ravenscroftj:main Aug 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants