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

Allow symlinks creation during model install rather than copying the model #5780

Closed
wants to merge 4 commits into from

Conversation

brandonrising
Copy link
Collaborator

@brandonrising brandonrising commented Feb 22, 2024

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:

Have you updated all relevant documentation?

  • Yes
  • No

Description

This PR allows users to elect into using symlinks for model installs rather than doubling the disk space usage of the models they're installing

@github-actions github-actions bot added python PRs that change python files services PRs that change app services labels Feb 22, 2024
@brandonrising brandonrising force-pushed the next-allow-symlinks-instead-of-copy branch from b9a4054 to 453c688 Compare February 22, 2024 20:49
@brandonrising
Copy link
Collaborator Author

Is there a reason not to do it this way?

@hipsterusername
Copy link
Member

@brandonrising - Have you tested? If this works, it'd make sense to me.

@brandonrising
Copy link
Collaborator Author

@brandonrising - Have you tested? If this works, it'd make sense to me.

Only tested some on a mac so far. Planning on testing on windows/linux today, but it works really well on a mac for standard generations.

@brandonrising brandonrising force-pushed the next-allow-symlinks-instead-of-copy branch from 453c688 to 5686287 Compare February 23, 2024 18:33
@lstein
Copy link
Collaborator

lstein commented Feb 28, 2024

Symlinks require elevated permissions on windows. There needs to be a check for this

Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

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

A couple of comments.

  1. As noted above, symlinks won’t work on Windows unless the user is running as an Admin or has enabled Developer mode. So you should catch an exception and revert to physical copying if the call fails.

  2. As an alternative to doing the symlinking, there is an unused option named inplace that can be passed to the install methods. If this is set, then local model file is not moved or copied; instead its current path is simply registered with the model configuration record. It looks like I simply neglected to expose this feature to the API, but I could do this. What do you think?

@brandonrising
Copy link
Collaborator Author

A couple of comments.

  1. As noted above, symlinks won’t work on Windows unless the user is running as an Admin or has enabled Developer mode. So you should catch an exception and revert to physical copying if the call fails.
  2. As an alternative to doing the symlinking, there is an unused option named inplace that can be passed to the install methods. If this is set, then local model file is not moved or copied; instead its current path is simply registered with the model configuration record. It looks like I simply neglected to expose this feature to the API, but I could do this. What do you think?
  1. Do you think it's potentially fine if it throws errors without reverting to copy since it's an opt in feature in the config? Maybe we can just update the description of the config to say it requires admin rights on windows? As a user, if I set this I think I'd be annoyed if I found out it was copying when it failed to do symlinks even if there was a log in the terminal for it.

  2. I think inplace would solve all the issues I made this PR for, but it is also kind of nice to have the folder on the file system telling you every model that's installed on invoke and where they're located since the alternative to that now is to open the sqlite db and query a table.

@brandonrising brandonrising force-pushed the next-allow-symlinks-instead-of-copy branch from 7f2705b to b8ef940 Compare February 28, 2024 14:58
@psychedelicious
Copy link
Collaborator

Not a fan of platform-specific behaviours and special handling.

You shouldn't need to access the db to see model paths, you can just open the model manager tab right? We can easily expose the paths there if we haven't already.

@brandonrising
Copy link
Collaborator Author

Not a fan of platform-specific behaviours and special handling.

You shouldn't need to access the db to see model paths, you can just open the model manager tab right? We can easily expose the paths there if we haven't already.

Yeah that's fair. I didn't run into any issues running it on windows, but maybe I've just always been running it in admin mode. Honestly, in-place installs like @lstein suggested would resolve the issues I'm having anyway. I could switch this PR to do that instead.

@psychedelicious
Copy link
Collaborator

I'd much prefer an in-place install option. It probably shouldn't be the default, because it shifts the responsibility of managing the model files to the user. We can add a checkbox to the MM UI, with a blurb explaining the feature.

How does in-place interact with the diffusers conversion cache? What if my in-place model is on a different drive from my root models dir - say, a mounted network drive or external drive - is that going to tank performance of the JIT model conversion?

@psychedelicious
Copy link
Collaborator

That said, I don't think we need this for v4. We have a lot of changes to model installation happening, maybe we should work towards this for a minor release after we are confident in the model installation process.

@brandonrising
Copy link
Collaborator Author

Closing in favor of in place installs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python PRs that change python files services PRs that change app services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants