-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Non-interactive model download (support HUGGINGFACE_TOKEN
)
#1578
Conversation
Also note that they've changed the settings on some of the repos so now not all of the Stable Diffusion models require a token. As of last time I checked earlier this weekend, the |
That is how I initially implemented it, but the behaviour becomes inconsistent between using the officially supported env var |
848a700
to
0d4f297
Compare
Moving the initialization file is proving tricky if we want to maintain backwards compatibility with the current setup, because we want to move it into the runtime dir, but users might already have a conflicting TLDR; Perhaps the initfile should be dealt with in a separate PR so that this one isn't blocked for too long. Unless we decide to break backwards-compatibility, then it's easy |
Why not have an environment variable that contains the path to the init file? As an aside, introducing Globals wasn't my favorite approach, but it was better than carrying a directory path across multiple nested functions. Just to set expectations, unless the release gets delayed significantly I'm going to go with the existing configuration script for 2.2.0, and release the new and improved one that you're working on for 2.2.1. This is primarily because I've got a lot of documentation-writing to do and won't be able to do thorough testing on this. I very much appreciate your work on this bit of the code. I also have a suggestion for a new feature. Either as a command-line option or as an automatic feature during interactive processing, it would be great if the script could scan the existing |
That being said, if I do have time I will try very hard to get this in. The improvements look very good. |
All sounds great to me. I actually might have a viable approach to handling the |
fe44bd7
to
db50446
Compare
I think I was able to wrangle the config file location into a good state; this is now ready for review. @lstein would you like me to tackle the auto-scanning of models in this same PR, or open a new feature request for it? I'd prefer the latter as it might get messy to review otherwise, but it's your call, please let me know. |
Go ahead and start a new PR. I'm going to integrate what you've done here with a few minor changes I made on a different branch and then merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Will do some functional testing prior to merge.
I seem to have oddly broken some tests, let me look into that 🤔 Edit: I can't reproduce these test failures locally. On closer look, this might be a transient failure: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started testing and then realized that the default location for the init file has become the InvokeAI repo. This is when neither --root nor INVOKEAI_ROOT is provided.
I don't really want the init file to be stored in the repo because ultimately I want to be able to do a non-source install. When this happens the repo is going to be hidden deep inside a venv folder somewhere and the user will never be able to find the init file. This is really tricky to finesse because the init file tells invoke where the root directory is, but if the init file is inside the root directory how does invoke find it?
In addition, the repo is really cluttered already.
So I want to consult with you about the design decision here. Maybe a better thing to do is to create the initfile in the user's home directory, don't make it hidden, and give clear instructions how they can move it by creating an environment variable or pointing invoke.py to it with --root each time. Sadly, users are not savvy about setting environment variables.
That sounds unexpected and not intentional. The logic I was trying to implement was to: 1. if an existing config file is found in the user's home directory, then keep using it, and 2. if no existing file is found, then create one in the runtime directory. If the default location of the runtime directory happens to be the root of the repo, then yes, this is a problem. I think this stems from: # This is usually overwritten by the command line and/or environment variables
- Globals.root = '.'
+ Globals.root = root_dir if (root_dir := os.getenv("INVOKEAI_ROOT")) else "." But this behaviour should have remained unchanged if neither the env var nor the Agreed that we never want to mix the runtime dir and neither the cloned codebase nor especially the dev-installed Python module, as you mention. Would it make sense to instead default the runtime dir to be located at
Definitely, that's the catch-22 situation I was talking about earlier. I think the best solution is to remove ambiguity by having a sane default that's more explicit and guaranteed to be outside of the codebase. |
Yeah, I think we need to reverse the logic and have a predictable location of the |
Just marking this as draft again while we work out the strategy. |
dca7715
to
2bfc89d
Compare
I also just noticed that the @lstein perhaps leaving this PR unmerged until after 2.2.0 is indeed for the best, so that we can clean it all up after the release dust settles. I will keep rebasing+updating it though. Do you agree? (CLI argument handling and various entrypoints could use quite a bit of future refactoring + consolidation in general, IMHO) |
2bfc89d
to
77b468a
Compare
77b468a
to
442d846
Compare
I moved the init file / root dir changes to another branch and will do a separate PR for it, as it was getting quite messy and needs a larger refactor. |
fdfa615
to
66d7c46
Compare
This is ready to be reviewed again, because the runtime dir / init file location refactoring will be dealt with separately, likely in #1615 |
HUGGINGFACE_TOKEN
)
Very useful for installation via UI 🍾 |
also implement a generic way of reporting issues at the end of installation
this mirrors the behaviour when using the officially supported env var
…ion in globals.py comment
c296d3a
to
da39724
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Thanks.
This PR adds the following improvements:
Support for the
HUGGINGFACE_TOKEN
environment variableEnsures that the SD weights download proceeds in a truly non-interactive fashion when the
--yes
flag is used and a valid authentication token is present in any of the supported locations (env vars or cached).Tested as follows:
HUGGING_FACE_HUB_TOKEN
env var is not empty:--yes
: weights download proceeds; token is not persisted in cache--yes
: weights download proceeds without an additional prompt; token is not persisted in cacheHUGGINGFACE_TOKEN
env var is not empty:--yes
flag: user is prompted for the token, token is cached only if valid (same as current behaviour)--yes
flag: weights download is skipped (same as current behaviour)Additionally:
outdir
location was not printed due to missing f-string--outdir
argument in a message