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

Docker support #34

Merged
merged 17 commits into from
Jun 13, 2023
Merged

Docker support #34

merged 17 commits into from
Jun 13, 2023

Conversation

frasergr
Copy link
Contributor

#26

  • Docker related items are in ./docker/
  • Running docker-compose.yml file from the ./docker/ directory will build an image which
    • Installs system dependencies
    • Installs frontend and backend packages
    • Builds the frontend
    • Node serves the built frontend
  • The docker-compose.yml file sets up some host volumes including ./collector/hotdir/ where you can add files to process
  • Watch for local file changes: docker exec -it --workdir=/app/collector anything-llm python watch.py
  • Process document from service: docker exec -it --workdir=/app/collector anything-llm python main.py

To have this running as a single container and serving the built frontend files, I had to make some changes on the server so that the "API" routes now have a path prefix of /api (see in ./server/index.js)

@frasergr frasergr marked this pull request as draft June 12, 2023 16:17
@jwaltz
Copy link
Contributor

jwaltz commented Jun 12, 2023

This guy dockers. Really nice! May I ask why that specific Ubuntu image as the base image and not an alpine or slim variant?

Edit: my initial--and likely naive--approach would have been to separate the frontend, server, and collector into their own services, each with their own Dockerfile, and use docker-compose to tie them together. I was thinking a benefit could be only rebuilding portions of the app when needed and being able to use a specific base image like python3 alpine and node alpine. Can you help me understand the tradeoffs of this and your approach?

@timothycarambat timothycarambat self-assigned this Jun 12, 2023
@frasergr
Copy link
Contributor Author

specific Ubuntu image as the base image and not an alpine or slim variant?

The vectordb node library seems to have a few different dependencies which didn't work with Alpine or slim (Debian 11) node images. OpenSSL 3 is required as well as some other stuff I don't remember. I did try to make things work on the smaller images but in the end I went with a specific LTS Ubuntu version (the same as I'm using locally) where things are working.

It would be nice if we could make the image smaller as it's around 1.5GB now but I don't think it's a dealbreaker for initial support.

The PR is a draft for now because after some more testing I realized there's some issues on fresh repos (where the SQLite db doesn't already exist). I'll be pushing the changes for that later this evening.

@@ -31,7 +31,7 @@
"sqlite3": "^5.1.6",
"uuid": "^9.0.0",
"jsonwebtoken": "^8.5.1",
"vectordb": "0.1.5-beta"
"vectordb": "0.1.5"
Copy link
Member

Choose a reason for hiding this comment

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

Ah thanks for this, Lance team didnt tell me they made the beta changes available in 0.1.5 (x86 darwin was missing so they made a beta branch for us)

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 was getting some errors or warnings on the beta branch and they went away on 0.1.5. Did you test that package running node locally? I wasn't able to test on a Mac.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't but when I checked it out I saw the missing file that was breaking the v0.1.4 release so I can manually check tonight.

@timothycarambat
Copy link
Member

@frasergr you are the 🐐

Looking great so far, ping me when ready for review but all runs great so far. This is a massive win for the repo so I very much appreciate you doing this. If you are in this Discord let me know your handle and ill give you a contributor role. Same to you @jwaltz

@jwaltz
Copy link
Contributor

jwaltz commented Jun 13, 2023

Sorry, I think I was editing my message as you replied @frasergr. Here is my additional query:

Edit: my initial--and likely naive--approach would have been to separate the frontend, server, and collector into their own services, each with their own Dockerfile, and use docker-compose to tie them together. I was thinking a benefit could be only rebuilding portions of the app when needed and being able to use a specific base image like python3 alpine and node alpine. Can you help me understand the tradeoffs of this and your approach?

And after reading your response: I understand now why some of the smaller images might not work but I feel like we can chip away at 1.5gb as a production docker image. Maybe the lowest common denominator image approach with individual services vs. the greatest common factor approach with an Ubuntu image and a singular service in the docker-compose might be beneficial?

@frasergr
Copy link
Contributor Author

@jwaltz I do like the separate service approach you describe and I think it's probably a much better solution (I'm not a Docker expert, so I don't really know) but for my use case I wanted a single container. I'm happy to hold off on this PR if you're working on something that we can review.

@timothycarambat
Copy link
Member

@frasergr @jwaltz I am happy to merge in the larger image for now while we then find a way to chip away at it later.

My rationale for this is that a lot of the issues Windows users are suffering from would be resolved by a Dockerization of the collector script. I always lean more toward a shipping-oriented approach since at the end of the day this is a convenience tool and 1.5GB image download is still nothing compared to the 10GB+ model/LLM downloads you would need for others.

It's certainly a whole other issue to optimize this but the sooner more people can use the tool, the better for all. Don't get me wrong though, it's important it is addressed down the line, but for now, I think we can proceed with the larger image.

Just my two cents, I only want to merge work that you are also proud of, so if you think it needs optimization - then so be it. 💯

@frasergr frasergr marked this pull request as ready for review June 13, 2023 06:03
@frasergr
Copy link
Contributor Author

frasergr commented Jun 13, 2023

@timothycarambat I think we're good to go now! As for the image size, I checked the filesystem after a build and found a few hundred MB of savings for things that weren't cleaned up during the build, including 100MB of unneeded .node binaries from vectordb. It's now showing as 1.01GB, not too bad I think.

There's absolutely room for more improvements on this Dockerfile, if someone else feels so inclined in the future. A number of the packages being installed may not be necessary. I was having issues getting chromium to work but it worked with all the ones that are there and I didn't test removing one-by-one...

Longer term work of separating the services into minimal containers, different environments, etc. would be a nice future improvement too.

A few more things to note:

  • Tested watch.py with .txt and .odt
  • Tested main.py with Article/Blog and YouTube
  • Tested Pinecone and Chroma
  • Tested on Windows 10 WSL 2 and Ubuntu 22.04.2
  • The UID and GID can be set in the .env file (defaults to 1000)
  • Never tested lancedb
  • Some of the hardcoded storage paths changed so updating to these changes will require moving data around

@jwaltz
Copy link
Contributor

jwaltz commented Jun 13, 2023

I think this is great work that should be merged as soon as it is ready. I agree that the value is immediate docker support and can be iterated up on in the future to break out services, etc.

Copy link
Member

@timothycarambat timothycarambat left a comment

Choose a reason for hiding this comment

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

Tested end-to-end all flows and all vector DBs.

Will need to add a README to this because I can already know this is going to cause issues for those less familiar with Docker.

LGTM - lets GOOOOOO.

Thank you so much and lmk if you want a contributor role in the Discord

@timothycarambat timothycarambat merged commit 9f33b3d into Mintplex-Labs:master Jun 13, 2023
@frasergr frasergr deleted the 26-docker branch June 13, 2023 21:37
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.

None yet

4 participants