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 setup #68

Merged
merged 13 commits into from
Dec 7, 2023
Merged

Conversation

cardosofede
Copy link
Contributor

This PR fixes the errors when trying to run the project from Docker.
Main changes:

  1. Remove general Dockerfile and add specific Dockerfile for Frontend and Backend.
  2. Make the URL of the Backend configurable via env variables.
  3. Refactor docker-compose.yml file to run redis + backend + frontend.
  4. Minor fix in .env placeholders since the values were requested when starting the product --> is necessary to add error handling in the app.

Copy link

cla-bot bot commented Nov 21, 2023

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the username @cardosofede on file. In order for us to review and merge your code, please complete the Individual Contributor License Agreement here https://forms.gle/bJtcHNhhWwarQf83A .

This process is done manually on our side, so after signing the form one of the maintainers will add you to the contributors list.

For more details about why we have a CLA and other contribution guidelines please see: https://github.com/langchain-ai/opengpts/blob/main/CONTRIBUTING.md.

@cla-bot cla-bot bot added the cla-signed label Nov 21, 2023
@donatienthorez
Copy link
Contributor

This fix is really good and helped me getting it running with Docker. Is there any blocker from merging it? 🤔

@albertoecf
Copy link

Same here! It ran successfully on my instance. @nfcampos / @eyurtsev is there any blocker for merging it ?

@cardosofede
Copy link
Contributor Author

Thanks @donatienthorez @albertoecf for the feedback!

@nfcampos
Copy link
Contributor

The main blocker for merging this is the removal of the root docker file, that can be used for deployments so it shouldn't be remove imo

@cardosofede
Copy link
Contributor Author

cardosofede commented Nov 30, 2023

Hello @nfcampos, thank you for your feedback.

I've adjusted the Docker setup for better modularity. The main Dockerfile, previously used for the backend, has been moved to the backend directory. A new Dockerfile is introduced for the frontend. This separation allows independent development and deployment of each component.
I can add a Dockerfile.backend and Dockerfile.frontend in the root of the project but I think that will be the same as it is now.

For deployment, you can use docker-compose.yml, which builds both the backend and frontend from their respective Dockerfiles.

The steps to get started are:

  1. Clone the repository.
  2. Run docker compose up.
  3. Wait for the Docker images for both frontend and backend to build and Redisearch to be downloaded.
  4. The system is ready for use.

I've dropped you a message on Slack to discuss this approach in more detail. I'm open to feedback and willing to make any adjustments necessary to facilitate the merging of this PR.

Thanks, Fede.

@@ -4,7 +4,7 @@
"version": "0.0.0",
"type": "module",
"scripts": {
"dev": "vite",
"dev": "vite --host",
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The --host option allows the server to be accessed over the network, not just localhost. it's necessary when running via docker.
When running without --host and exposing the port from the container to the local machine we weren't able to see the output. With that argument, the Frontend was exposed.

@nikshepsvn
Copy link

hey, any updates here?

@fengtality
Copy link

I installed OpenGPTs from both source and this Docker setup.

Docker is much simpler since docker compose up works and you don't need to deal with Redis, installing dependencies, and adding placeholder values for env variables.

@nfcampos nfcampos merged commit 2142604 into langchain-ai:main Dec 7, 2023
7 checks passed
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.

6 participants