-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[Refactor] Better align from_single_file
logic with from_pretrained
#7496
Conversation
from_single_file
with from_pretrained
from_single_file
logic with from_pretrained
docs/source/en/api/single_file.md
Outdated
- [`StableDiffusionPipeline`] | ||
- [`StableDiffusionImg2ImgPipeline`] | ||
- [`StableDiffusionInpaintPipeline`] | ||
- [`StableDiffusionControlNetPipeline`] | ||
- [`StableDiffusionControlNetImg2ImgPipeline`] | ||
- [`StableDiffusionControlNetInpaintPipeline`] | ||
- [`StableDiffusionUpscalePipeline`] | ||
- [`StableDiffusionXLPipeline`] | ||
- [`StableDiffusionXLImg2ImgPipeline`] | ||
- [`StableDiffusionXLInpaintPipeline`] | ||
- [`StableDiffusionXLControlNetPipeline`] |
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.
We should consider having a utility method to find this programmatically as well.
docs/source/en/api/single_file.md
Outdated
## Models that currently support `from_single_file` loading | ||
|
||
- [`UNet2DConditionModel`] | ||
- [`StableCascadeUNet`] | ||
- [`AutoencoderKL`] | ||
- [`ControlNetModel`] |
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.
Same here as well.
docs/source/en/api/single_file.md
Outdated
scheduler = DDIMScheduler() | ||
pipe = StableDiffusionXLPipeline.from_single_file(ckpt_path, scheduler=scheduler) | ||
``` | ||
|
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.
Add a line here?
src/diffusers/loaders/single_file.py
Outdated
config_file = hf_hub_download( | ||
default_pipeline_config["pretrained_model_name_or_path"], | ||
filename=cls.config_name, | ||
cache_dir=cache_dir, | ||
revision=revision, | ||
proxies=proxies, | ||
force_download=force_download, | ||
resume_download=resume_download, | ||
token=token, | ||
local_files_only=local_files_only, | ||
) |
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.
There are many single file checkpoints for which we don't have equivalent diffusers checkpoints, but they can be loaded into a pipeline. How does this stand in that case? We fetch the corresponding config file examining the checkpoint and do a best effort to map accordingly? If so, I think there should be a comment about it.
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.
do you have examples of these?
There are many single file checkpoints for which we don't have equivalent diffusers checkpoints
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.
Have seen many but cannot recall any exact names of the top of my head. You can find a couple on CivitAI too (it hosts full finetunes of SD and SDXL too not just LoRAs). Cc: @vladmandic here.
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
@vladmandic I think this is almost ready to merge, if you have the time would you be able to test the branch to see if any breaking changes occur. I tried to make sure we have full backwards compatibility, but given the number of changes here, I just want to be on the safe side. |
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.
Thanks for working on this. Some tests seem to be failing. Here's you can find the detailed logs:
https://huggingface.co/datasets/sayakpaul/sample-datasets/blob/main/text_outputs_single_file/
os.path.join(pretrained_model_name_or_path, subfolder, cls.config_name) | ||
): | ||
config_file = os.path.join(pretrained_model_name_or_path, subfolder, cls.config_name) | ||
elif os.path.isfile(os.path.join(pretrained_model_name_or_path, cls.config_name)): | ||
# Load from a PyTorch checkpoint |
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.
# Load from a PyTorch checkpoint | |
# Load from a PyTorch checkpoint (SD checkpoints usually have some configuration details in them) |
local_files_only=None, | ||
token=None, | ||
): | ||
allow_patterns = ["**/*.json", "*.json", "*.txt", "**/*.txt"] |
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.
Why do we allow txt files here?
Also, nit: ["**/*.json", "*.json", "*.txt", "**/*.txt"]
can be reduced to just ["*.json", "*.txt"]
, I think.
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.
Tokenizer subfolders have text files as part of their configs sometimes. e.g.
https://huggingface.co/stabilityai/stable-diffusion-xl-refiner-1.0/tree/main/tokenizer_2
allow_patterns=allow_patterns, | ||
) | ||
|
||
return cached_model_path |
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.
Prefer cached_model_config_path
.
# We shouldn't allow configuring individual models components through a Pipeline creation method | ||
# These model kwargs should be deprecated | ||
scaling_factor = kwargs.get("scaling_factor", None) | ||
if scaling_factor is not None: | ||
deprecation_message = ( | ||
"Passing the `scaling_factor` argument to `from_single_file is deprecated " | ||
"and will be ignored in future versions." | ||
) | ||
deprecate("scaling_factor", "1.0.0", deprecation_message) |
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.
Can we include instructions on what the users should use instead in this message?
src/diffusers/loaders/single_file.py
Outdated
"Detected legacy `from_single_file` loading behavior. Attempting to create the pipeline based on inferred components.\n" | ||
"This may lead to errors if the model components are not correctly inferred. " | ||
"To avoid this warning, please explicity pass the `config` argument to `from_single_file` with a path to a local diffusers model repo " | ||
"or run `from_single_file` with `local_files_only=False` first to update the local cache directory with " | ||
"the necessary config files.\n" |
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.
An example config="..."
in the message would be helpful for the users.
@@ -123,7 +123,7 @@ jobs: | |||
shell: bash | |||
strategy: | |||
matrix: | |||
module: [models, schedulers, lora, others] | |||
module: [models, schedulers, lora, others, single_file] |
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.
Shouldn't we also include this in the nightly tests?
@@ -0,0 +1,78 @@ | |||
# coding=utf-8 |
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.
WDYT about further grouping this so that it's easier for the contributors to pique through a certain test?
Grouping like so:
single_file
controlnet
vae
stable_diffusion
stable_cascade
...
image = torch.from_numpy(load_hf_numpy(self.get_file_format(seed, shape))).to(torch_device).to(dtype) | ||
return image | ||
|
||
def test_single_file_inference_same_as_pretrained(self): |
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 is failing on a T4:
FAILED tests/single_file/test_model_vae_single_file.py::AutoencoderKLSingleFileTests::test_single_file_inference_same_as_pretrained - AssertionError: Max diff is absolute 0.9882212281227112. Diff tensor is tensor([0.0288, 0.9882, 0.0790, 0.1767, 0.1501, 0.9556, 0.1592, 0.0806]).
Could you please check?
self._compare_component_configs(pipe, single_file_pipe) | ||
|
||
|
||
class SDXLSingleFileTesterMixin: |
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.
Is there a major difference between SDXLSingleFileTesterMixin
and SDSingleFileTesterMixin
?
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.
SDXLSingleFileTesterMixin
accounts for the multiple text encoder components that may or may not be present in the pipeline depending on the checkpoint.
@@ -0,0 +1,183 @@ | |||
import gc |
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.
Failing tests:
FAILED tests/single_file/test_stable_diffusion_controlnet_inpaint_single_file.py::StableDiffusionControlNetInpaintPipelineSingleFileSlowTests::test_single_file_components_with_original_config - AssertionError: single file sample_size: 512 differs from pretrained 256
FAILED tests/single_file/test_stable_diffusion_controlnet_inpaint_single_file.py::StableDiffusionControlNetInpaintPipelineSingleFileSlowTests::test_single_file_components_with_original_config_local_files_only - AssertionError: single file sample_size: 512 differs from pretrained 256
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.
Issue is that the config on the hub is incorrect.
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.
thanks!!!!!
i left only some nits! let's merge this in soon!!!
@@ -252,69 +362,190 @@ def from_single_file(cls, pretrained_model_link_or_path, **kwargs): | |||
local_files_only = kwargs.pop("local_files_only", False) | |||
revision = kwargs.pop("revision", None) | |||
torch_dtype = kwargs.pop("torch_dtype", None) | |||
use_safetensors = kwargs.pop("use_safetensors", None) | |||
|
|||
is_legacy_loading = False |
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.
docstring needed here
src/diffusers/loaders/single_file.py
Outdated
@@ -252,69 +362,190 @@ def from_single_file(cls, pretrained_model_link_or_path, **kwargs): | |||
local_files_only = kwargs.pop("local_files_only", False) | |||
revision = kwargs.pop("revision", None) | |||
torch_dtype = kwargs.pop("torch_dtype", None) | |||
use_safetensors = kwargs.pop("use_safetensors", None) |
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.
is this relevant now because we will run from_pretrained()
for missing components? need a doc string here:)
|
||
``` | ||
|
||
## Override configuration options when using single file loading |
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 think we should swap this section with the next one, because in the example we used the config
argument that hasn't been explained yet (but it will be in the next section)
use_safetensors = kwargs.pop("use_safetensors", None) | ||
|
||
is_legacy_loading = False | ||
|
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.
should we also accept a local_dir_use_symlinks
here and consistently pass it down to any place that downloads things?
currently, the from_single_file
for models accepts local_dir_use_symlinks
but not pipeline
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.
Hmm actually might be better to not include local_dir
support in this version. I think it's better for the user to predownload stuff to local paths beforehand. I'll remove this.
|
||
SINGLE_FILE_LOADABLE_CLASSES = { | ||
"StableCascadeUNet": { | ||
"checkpoint_mapping_fn": convert_stable_cascade_unet_single_file_to_diffusers, |
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.
the single checkpoints are not in subfolders
https://huggingface.co/stabilityai/stable-cascade/tree/main
f"FromOriginalModelMixin is currently only compatible with {', '.join(SINGLE_FILE_LOADABLE_CLASSES.keys())}" | ||
) | ||
|
||
checkpoint = kwargs.pop("checkpoint", None) |
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.
oh maybe rename it to pretrained_model_name_or_path_or_dict
and consolidate these two arguments?
it is done like this for load_lora_weights
and load_ip_adapter
too
diffusers/src/diffusers/loaders/lora.py
Line 82 in 0d7c479
def load_lora_weights( |
mapping_functions = SINGLE_FILE_LOADABLE_CLASSES[class_name] | ||
|
||
checkpoint_mapping_fn = mapping_functions["checkpoint_mapping_fn"] | ||
if "config_mapping_fn" in mapping_functions: |
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.
nit: I would move this inside the if original_config: ...
block because it is only used there (less code to read when it is not applicable)
src/diffusers/loaders/unet.py
Outdated
@@ -1002,156 +997,3 @@ def _load_ip_adapter_weights(self, state_dicts, low_cpu_mem_usage=False): | |||
self.config.encoder_hid_dim_type = "ip_image_proj" | |||
|
|||
self.to(dtype=self.dtype, device=self.device) | |||
|
|||
def _load_ip_adapter_loras(self, state_dicts): |
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.
+1
should not go away I think
…d` (#7496) * refactor unet single file loading a bit. * retrieve the unet from create_diffusers_unet_model_from_ldm * update * update * updae * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * tests * update * update * update * Update docs/source/en/api/single_file.md Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * Update docs/source/en/api/single_file.md Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * update * update * update * update * update * update * update * update * update * update * update * update * update * Update docs/source/en/api/loaders/single_file.md Co-authored-by: YiYi Xu <yixu310@gmail.com> * Update src/diffusers/loaders/single_file.py Co-authored-by: YiYi Xu <yixu310@gmail.com> * Update docs/source/en/api/loaders/single_file.md Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * Update docs/source/en/api/loaders/single_file.md Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * Update docs/source/en/api/loaders/single_file.md Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * Update docs/source/en/api/loaders/single_file.md Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update --------- Co-authored-by: sayakpaul <spsayakpaul@gmail.com> Co-authored-by: YiYi Xu <yixu310@gmail.com>
What does this PR do?
Single file loading is still far from ideal. We should aim to align the behaviour as close to from_pretrained as possible (with the goal of converging to a single loading method)
Some of the issues that need to be addressed
This PR attempts to get single file loading behaviour much closer to the logic used in from_pretrained
For Models
OriginalModelMixin
to all Diffusers models that are meant to support from_single_file loadingThis allows us to rely on a lot of the functionality already defined in
ModelMixin
andDiffusionPipeline
to create the correct modelFor Pipelines
model_index.json
file for this checkpoint and load the components using similar logic tofrom_pretrained
. Note loading a Pipeline/Model via YAML is still supported, it is just not the default anymore.local_dir
andlocal_dir_use_symlinks
arguments to control where checkpoints are downloaded and to disable symlinking if users request it.TODO:
single_file_utils
This should make it a bit easier to add more Pipelines/Models with single file support. The process would become
We already have to take care of steps 1 and 2 when adding a model to diffusers. So it's just a matter inferring the proper config from the checkpoint (not easy but very possible).
Fixes # (issue)
Before submitting
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.