[modular] not pass trust_remote_code to external repos #13204
Conversation
sayakpaul
left a comment
sayakpaul
left a comment
There was a problem hiding this comment.
I like it!
I left some thoughts to discuss. LMK if anything is unclear.
| f"To load this component manually:\n" | ||
| f" from diffusers import AutoModel\n" | ||
| f' {name} = AutoModel.from_pretrained("{spec.pretrained_model_name_or_path}", trust_remote_code=True)\n' | ||
| f" pipe.update_components({name} = {name})\n" | ||
| ) | ||
| logger.warning(warning_msg) |
There was a problem hiding this comment.
Of course, we cannot generalize to all possible combinations but what if the underlying component is a scheduler or a non-modeling component?
For schedulers, we could have AutoScheduler or something so that users won't have to remember a specific class name. For others (such as tokenizers, image processors), we already have "auto" classes.
I guess it's not that big a use case yet, but just wanted to flag.
There was a problem hiding this comment.
Also, should we error instead? If the user-side logger is not configured appropriately then this warning could go unnoticed.
There was a problem hiding this comment.
i updated the message
tried to make it error but reverted back to warning for now: it would be a little annoying that you won't be able to use load_components() because one element is not loadable
|
|
||
| block = ModularPipelineBlocks.from_pretrained(pipeline_repo_dir, trust_remote_code=True) | ||
| pipe = block.init_pipeline() | ||
| pipe.save_pretrained(pipeline_repo_dir) |
There was a problem hiding this comment.
Should we also inspect the modular_model_index.json file here (checking if the repo paths were serialized as expected)?
There was a problem hiding this comment.
I think if we can load it back, we are fine (we loaded it back next and verified the path too)
Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>
| assert pipe.unet is not None | ||
| assert getattr(pipe, "vae", None) is None | ||
|
|
||
| def test_load_components_selective_loading_incremental(self): |
There was a problem hiding this comment.
Do we also test if pipe doesn't have other expected components (like scheduler)?
| f"to the pipeline via `pipe.update_components()`. For example, if it is a custom model:\n\n" | ||
| f' {name} = AutoModel.from_pretrained("{spec.pretrained_model_name_or_path}", trust_remote_code=True)\n' | ||
| f" pipe.update_components({name}={name})\n" | ||
| ) |
| import json | ||
|
|
||
| from diffusers import UNet2DConditionModel |
There was a problem hiding this comment.
(nit): maybe we could shift all the imports to the top.
| for key in original_state_dict: | ||
| assert torch.equal(original_state_dict[key], loaded_state_dict[key]), f"Mismatch in {key}" | ||
|
|
||
| def test_save_pretrained_updates_index_for_model_with_no_load_id(self, tmp_path): |
3 participants
based on this discussion
huggingface/blog#3278 (comment)