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: Clean up and simplify docker entrypoint (#1235) #1259

Merged
merged 12 commits into from
Apr 19, 2024
Merged

Conversation

sarahwooders
Copy link
Collaborator

@sarahwooders sarahwooders commented Apr 15, 2024

Continuation of #1235 by @norton120

@sarahwooders
Copy link
Collaborator Author

@norton120 thanks for all these contributions -- they really cleanup the code so really appreciate it!

A couple questions about this PR:

  • We intended the docker compose up command to start a memgpt server that users could connect to via the client -- can we remove the tests from the compose.yaml into a separate file potentially? Also ideally the compose command would pull from pre-built containers on dockerhub and only build from local source if in development mode. Could we change the docker compose step to allow for two options - 1. development mode where a container is built and run locally, 2. prod mode where the latest (or specified version) of the published memgpt docker container is run?
  • When I run docker compose up -d && open http://memgpt.localhost, I get a "502 Bad Gateway" error. Is there some setup step I'm missing?

Also regarding the test failures, I believe the Embedding column must be of type Vector, got BINARY that you mentioned here is because the tests are using the sqlite+chroma storage backend instead of postgres. The tests are a bit messy right now and rely on overriding the config file at ~/.memgpt/config to alternate between different storage backends, so probably the config got overridden with the wrong storage backend and wasn't reset somehow. I'm not sure what the exact issue is, but its probably fine to skip the failing tests for now until we refactor the tests to be cleaner.

@norton120
Copy link
Contributor

@sarahwooders happy to help - I'll run through this today and get things working as expected for running in docker, and isolate all the bits for developing in docker so they don't hold it up. I can button up the docker dev env in a separate PR after

@norton120
Copy link
Contributor

@sarahwooders Got those fixes ready but it looks like I can't push to this branch, should I push them to #1259?

@sarahwooders
Copy link
Collaborator Author

@norton120 actually yeah lets maybe move back to your fork, since I don't think there's an easy way to give you permissions to push (sorry didn't realize). Can you give us access to your fork?

Thanks!

@sarahwooders
Copy link
Collaborator Author

@norton120 could you open a new PR again, and make sure to allow maintainers to edit the pull request?

IMG_0764

@norton120
Copy link
Contributor

@sarahwooders Updated PR is here with this as the base branch, you should have access but lemme know if not. If it is easier to point to main then let's do that - whatever works!

Let me know if you run into any issues with the proxy, I've scrubbed all my local images a couple times to make sure it works without any leftover artifacts (ie docker rm -f $(docker ps -qa) && docker image rm -f $(docker image ls -q) ) so it should work everywhere now

@sarahwooders sarahwooders merged commit b17c653 into main Apr 19, 2024
5 checks passed
@sarahwooders sarahwooders deleted the staging-pr1235 branch April 19, 2024 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants