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

Fix docker compose up and MacBook segfault #1428

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mbianchidev
Copy link

Implemented what was written on this comment and added some tweaks to make it work on without manual actions on the user's side

Could actually be a good idea to add a Dockerfile.mac instead of messing the Dockerfile.local file up, then it needs to be used in docker-compose.yaml when running on mac.

@mbianchidev mbianchidev changed the title Tentatively fixing MacBook segfault and docker compose now should be up without any manual action Fix docker compose up and MacBook segfault Dec 22, 2023
Added max workers to avoid pool error
@imartinez
Copy link
Collaborator

The latest changes to dockerfiles merged address some of these issues, for example the download of models. Could you take a look and share your thoughts? #1445

@mbianchidev
Copy link
Author

mbianchidev commented Dec 23, 2023

The latest changes to dockerfiles merged address some of these issues, for example the download of models. Could you take a look and share your thoughts? #1445

@imartinez thanks a lot for checking in!

Docker compose still doesn't work out-of-the-box cause the model is not downloaded by default, the only way to do it without embedding it in the image (ofc not feasible) is an entrypoint script, also there are two changes not present also propedeutic to make it work for the dockerfile around poetry.

RUN poetry config installer.max-workers 10
RUN poetry install --extras chroma

And for settings to switch the vectorstore to chroma instead of qdrant

vectorstore:
  database: chroma

Also USER worker before running the python command seems also to be unnecessary

The goal of my contribution is to make the project smooth for docker and docker compose users, if you clone the repo and build image + run or just run docker compose up one should be able to have it running locally with no further action.

Let me know if I can implement any changes to match more with your vision!

@imartinez
Copy link
Collaborator

The latest changes to dockerfiles merged address some of these issues, for example the download of models. Could you take a look and share your thoughts? #1445

@imartinez thanks a lot for checking in!

Docker compose still doesn't work out-of-the-box cause the model is not downloaded by default, the only way to do it without embedding it in the image (ofc not feasible) is an entrypoint script, also there are two changes not present also propedeutic to make it work for the dockerfile around poetry.

RUN poetry config installer.max-workers 10 RUN poetry install --extras chroma

And for settings to switch the vectorstore to chroma instead of qdrant

vectorstore:
  database: chroma

Also USER worker before running the python command seems also to be unnecessary

The goal of my contribution is to make the project smooth for docker and docker compose users, if you clone the repo and build image + run or just run docker compose up one should be able to have it running locally with no further action.

Let me know if I can implement any changes to match more with your vision!

Thanks for the detailed explanation.

  • "model is not downloaded by default" -> there is already an entrypoint setup script that you can run after building the image that will take care of that. I think that's the best way to go: docker compose run --rm --entrypoint="bash -c '[ -f scripts/setup ] && scripts/setup'" private-gpt
  • poetry max worker is something we should add, I agree
  • --extras chroma should not be added by default. Qdrant is the default vector db, and therefore Chroma-related dependencies are optional. Same for the change you propose to the settings.yaml file.
  • "USER worker" not sure about that one. I could test it out.

Let me know your thoughts.

@mbianchidev
Copy link
Author

Thanks for the detailed explanation.

  • "model is not downloaded by default" -> there is already an entrypoint setup script that you can run after building the image that will take care of that. I think that's the best way to go: docker compose run --rm --entrypoint="bash -c '[ -f scripts/setup ] && scripts/setup'" private-gpt
  • poetry max worker is something we should add, I agree
  • --extras chroma should not be added by default. Qdrant is the default vector db, and therefore Chroma-related dependencies are optional. Same for the change you propose to the settings.yaml file.
  • "USER worker" not sure about that one. I could test it out.

Let me know your thoughts.

I think that the entrypoint as proposed is a bit verbose but this may be only my own preference.

I had to do a bit of research before getting up and running with compose and judging by some recent and similar issues opened I was not the only one encountering the errors.
What about adding the command: docker compose run --rm --entrypoint="bash -c '[ -f scripts/setup ] && scripts/setup'" private-gpt to both the readme file of the repo and the doc?

A middle ground could also be creating a Dockerfile.mac to avoid touching the original one at all, what do you think?

Chroma is the only one working on mac/local if I'm not mistaken, correct? If that's the case I do think it might be better to enable it by default, what would the consequences be? Ofc it's a trade-off!

The deletion of USER worker was a test I made and it brought me to run docker compose successfully on top of not having any segfault error on mac (M2 chip), please do test it out and let me know the result.

Thanks again for engaging and keep up the good work!

Copy link
Contributor

Stale pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants