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.

Sign up for GitHub

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

Jump to bottom

[ENH]: Docker compose data volume #1119

Merged
merged 4 commits into from
Dec 6, 2023
Merged

[ENH]: Docker compose data volume #1119

merged 4 commits into from
Dec 6, 2023

Conversation

tazarov
Copy link
Contributor

@tazarov tazarov commented Sep 8, 2023

Description of changes

Summarize the changes made by this PR.

  • Improvements & Bug fixes
    • Added persistent volume in all docker compose files. The advantage is that by default user's data is not lost when docker compose down is executed unless --volume is provided. This gives a default behaviour that is non-destructive towards user data.

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js

Documentation Changes

N/A

@tazarov
feat: Docker compose data volume
4482eb3 
Added persistent volume in all docker compose files. The advantage is that by default user's data is not lost when `docker compose down` is executed unless `--volume` is provided. This gives a default behaviour that is non-destructive towards user data.
@github-actions
Copy link

github-actions bot commented Sep 8, 2023

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readbility, Modularity, Intuitiveness)

@tazarov
Copy link
Contributor Author

tazarov commented Sep 8, 2023

@HammadB setting as Draft until #1114 is merged, then I'll rebase this

@tazarov tazarov mentioned this pull request Sep 8, 2023
Closed
tazarov added a commit to amikos-tech/chroma-docs that referenced this pull request Sep 8, 2023
@tazarov
docs: Docker compose warning
5e08afa 
- Aligned warnings in the usage guide

Refs: chroma-core/chroma#1119
tazarov added a commit to amikos-tech/chroma-docs that referenced this pull request Sep 9, 2023
@tazarov
docs: Docker compose warning
ca538aa 
- Added a docker command to move data out from the docker compose volume to the host machine of the user

Refs: chroma-core/chroma#1119
@tazarov tazarov marked this pull request as ready for review October 12, 2023 15:43
@tazarov
Copy link
Contributor Author

tazarov commented Oct 12, 2023

@beggers Can you take a look at this?

@beggers
Copy link
Member

beggers commented Oct 18, 2023

Really sorry to just be looking at this now. Do Docker tests pass too?

@tazarov
Copy link
Contributor Author

tazarov commented Oct 18, 2023

@beggers, no issue; this is not urgent. Yes, integration tests (using docker-compose) are passing.

beggers
beggers approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member

@beggers beggers left a comment

Choose a reason for hiding this comment

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

:)

HammadB
HammadB reviewed Nov 29, 2023
View reviewed changes
docker-compose.yml Outdated
@@ -12,6 +12,7 @@ services:
dockerfile: Dockerfile
volumes:
- ./:/chroma
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should clean this up and verify that the data is where we expect. The way I think it is working rn is we are mounting your CWD as where the data is stored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

But yes, essentially we did the following:

  • Build an image where chroma is deployed under /chroma/
  • Mount and override the container /chroma path with CWD

One of the reasons the above has worked so far is that the build and run of the docker container happens on the same machine and from the root of the repo.

HammadB
HammadB reviewed Nov 29, 2023
View reviewed changes
docker-compose.test-auth.yml Outdated
@@ -30,5 +30,3 @@ services:
volumes:
test_index_data:
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets rename the volume to something more sensible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to chroma-data. Easy to reason about.

@HammadB
Copy link
Collaborator

HammadB commented Nov 29, 2023

Let's also add docs

@tazarov tazarov marked this pull request as draft December 1, 2023 08:06
@tazarov tazarov marked this pull request as ready for review December 6, 2023 09:15
@HammadB HammadB merged commit eea76bf into chroma-core:main Dec 6, 2023
92 of 94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HammadB HammadB HammadB approved these changes

@beggers beggers beggers approved these changes

Assignees
No one assigned
Labels
taz-sprint-1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tazarov @beggers @HammadB