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

Merged
merged 10 commits into from
Apr 15, 2024

Conversation

norton120
Copy link
Contributor

Please describe the purpose of this pull request.
Squaring away the compose implementation to make things a little more tidy.

nginx reverse-proxy

  • with the reverse proxy in place you can do docker compose up -d && open http://memgpt.localhost and open the local server. localhost:8083 and localhost:8283 still work, so since it seems like ports are hardcoded differently in different places we can be flexible.
  • all 3 hosts are available in swagger docs so users can set as needed. eventually it would be good to standardize here
  • cors hosts are now settable via an envar
  • Moved from the un-namespaced pgdata docker volume to a locally mounted .pgdata folder. If you are a dev with multiple containerized projects on your machine this is an easy collision to avoid.
  • added hot reload in the compose
  • moved entrypoints to commands (really what they should be in this case)

pydantic_settings

  • moving away from directly reading os envars all over and managing in a settings singleton.
  • consolidated the memgpt_pg_uri envar with the component envars - so you only set one.
  • dry'd up the default pg_uri logic.
    There is more of this to go, but this at least starts to establish the pattern and make it easier to abstract config logic for the server.

How to test
Workin' on this. contributors should be able to run the full test suite in the container. I for one put nothing on my metal, and containerized tests drop the barrier to entry.

Have you tested this PR?
Eyeballed, still working on running the test suite in containers.

Related issues or PRs
#1233

Is your PR over 500 lines of code?
This is a lot of dry'ing and cleanup, so it will ideally reduce the codebase by more than 500 lines.

@norton120 norton120 changed the title Clean up and simplify docker entrypoint fix: Clean up and simplify docker entrypoint Apr 9, 2024
@cpacker
Copy link
Owner

cpacker commented Apr 9, 2024

👀 😍

@norton120
Copy link
Contributor Author

@cpacker are there sample ~/.memgpt/credentials and ~/.memgpt/config files somewhere I can use to configure the test suite? I'd like to get it working without needing an installed instance if possible

@norton120
Copy link
Contributor Author

> @cpacker are there sample ~/.memgpt/credentials and ~/.memgpt/config files somewhere I can use to configure the test suite? I'd like to get it working without needing an installed instance if possible
nvm found it in test/data

@norton120
Copy link
Contributor Author

So we've got most of the test suite passing in the container, can be run with docker compose --profile tests run --rm memgpt_tests at the moment. I am running into some snags:

  • there's a state leak somewhere that errors out the tests in tests/test_client.py in a full run with Embedding column must be of type Vector, got BINARY. The test module passes when run in isolation
  • I'm trying to figure out how test_metadata_store.test_storage is working, on line 89 there's an undefined variable referenced
    # config is not defined in this module or imported
        llm_config=config.default_llm_config,
        embedding_config=config.default_embedding_config,
  • There are also a couple of tests where I'm not super clear on what they are doing - for example in test_openai_client.py the OAI base_url is set to a local address, but I'm not sure what should be catching those calls (and they were erroring out)

I can keep at it, however this PR is getting large; I could alternatively exclude the problem tests when run in docker for the moment, and then chip away with smaller PRs to keep cleaning up?

@cpacker let me know what makes the most sense

@sarahwooders
Copy link
Collaborator

@cpacker @norton120 maybe we can merge this into a separate branch from main, so I can help push it over the finish line wrt passing tests, formatting, etc.? This would be a great PR to get merged asap.

@cpacker cpacker changed the base branch from main to staging-pr1235 April 15, 2024 19:37
@cpacker
Copy link
Owner

cpacker commented Apr 15, 2024

@cpacker @norton120 maybe we can merge this into a separate branch from main, so I can help push it over the finish line wrt passing tests, formatting, etc.? This would be a great PR to get merged asap.

SGTM, just changed the PR base to a new staging branch.

@cpacker cpacker marked this pull request as ready for review April 15, 2024 19:37
@sarahwooders sarahwooders merged commit 6de6993 into cpacker:staging-pr1235 Apr 15, 2024
2 of 5 checks passed
@sarahwooders
Copy link
Collaborator

@norton120 I left a comment on #1259 with a few questions!

sarahwooders added a commit that referenced this pull request Apr 19, 2024
Co-authored-by: Ethan Knox <norton120@gmail.com>
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