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

Sisco/docker allow relative paths for invokeai data #5344

Conversation

dsisco11
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Community Node Submission

Have you discussed this change with the InvokeAI team?

  • Yes
  • No, because: it's small, discuss it in the comments

Have you updated all relevant documentation?

  • Yes
  • No
  • Maybe?

Description

This PR removes the docker-compose absolute path restriction for the data path of InvokeAI.
The compose file will now bind the external Invoke data directory using the following envar values (in order of priority):

  1. INVOKEAI_LOCAL_ROOT
  2. INVOKEAI_ROOT
  3. ~/invokeai

Since this change provides a fallback to the original INVOKEAI_ROOT variable, it should not impact existing users.

Related Tickets & Documents

  • Related Issue #
  • Closes #

QA Instructions, Screenshots, Recordings

Merge Plan

Added/updated tests?

  • Yes
  • No

[optional] Are there any post deployment tasks we need to perform?

Copy link
Member

@ebr ebr left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal. I believe we had it like this at some point, but removed it because the usage was unclear, and it caused issues for those who ran invoke both in and outside of docker.

Perhaps we could solve this by introducing a new env var CONTAINER_INVOKEAI_ROOT, defaulting to /invokeai? This way behaviour of INVOKEAI_ROOT remains the same between bare-metal and docker setups, and the new env var can be used if there's a need to change the root path in container.
something like:

source: ${INVOKEAI_ROOT:-~/invokeai}
target: ${CONTAINER_INVOKEAI_ROOT:-/invokeai}

Would that work - thoughts?

docker/docker-compose.yml Outdated Show resolved Hide resolved
@dsisco11
Copy link
Contributor Author

dsisco11 commented Dec 24, 2023

But wait, isn't the INVOKEAI_ROOT var used internally by various pieces of InvokeAIs' backend code so it can find the root path?
So wouldn't INVOKEAI_ROOT being different from the folder that the container internally binds to, prevent invoke from finding the path correctly?

Edit
I just tried it and I was right, INVOKEAI_ROOT has to point to the internal container directory, or invoke can't find the data and sees an empty data folder and tries to perform the initial setup.
So we definitely can't make the existing INVOKEAI_ROOT var satisfy relative paths due to this fundamentally preventing invoke from locating it inside the container. I think that docker-compose will use the filesystem root when resolving the relative path during binding, but invoke will use its install directory or something that isn't FS root.

So that would leave just a few options here:

  1. change invokes work-directory in the container (to system root? red flag)
  2. we need to determine the desirable semantics for a new var that lets users specify a relative external path while also not affecting current users. (Basically, figure out a better name for the new envar I am proposing here).

Maybe it would be better to separate the concept of the "container" from "invoke", so instead of INVOKEAI_LOCAL_ROOT something like CONTAINER_INVOKEAI_EXTERNAL_PATH.
So the full bind would be:

- type: bind
  source: ${CONTAINER_INVOKEAI_EXTERNAL_PATH:-${INVOKEAI_ROOT:-~/invokeai}}
  target: ${INVOKEAI_ROOT:-/invokeai}

The major points here are:

  • existing behavior doesn't change, the normal envar still functions as expected with the new one being an override.
  • new envar is still related to invokeai, so its not generic enough for conflicts.
  • new envar is descriptive enough to let users know it is just for containers.

@ebr
Copy link
Member

ebr commented Dec 25, 2023

Sounds good. May I ask that we name it HOST_INVOKEAI_ROOT? that way we make it clear that it's the invoke root on the docker host rather than in container. Other than that, this hits all the marks as you've described. Please rename and I'll test and approve. Thank you!

@dsisco11
Copy link
Contributor Author

done!

@Millu Millu requested a review from ebr December 27, 2023 09:53
docker/.env.sample Outdated Show resolved Hide resolved
docker/.env.sample Outdated Show resolved Hide resolved
@ebr ebr enabled auto-merge (squash) December 29, 2023 16:10
@hipsterusername hipsterusername force-pushed the sisco/docker-allow-relative-paths-for-invokeai-data branch from 7d7afd3 to 70896c7 Compare January 2, 2024 13:16
@ebr ebr merged commit 5413bf0 into invoke-ai:main Jan 2, 2024
7 checks passed
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

3 participants