Skip to content

Check correct model type is passed to from_pretrained #10189

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.

Sign up for GitHub

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

Merged
merged 26 commits into from
Dec 19, 2024
Merged

Check correct model type is passed to from_pretrained #10189

Changes from 2 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
2fd4508
Check correct model type is passed to `from_pretrained`
hlky Dec 11, 2024
78c6e68
Merge branch 'main' into check-model-passed-pipeline
hlky Dec 11, 2024
185a78f
Flax, skip scheduler
hlky Dec 11, 2024
679c18c
test_wrong_model
hlky Dec 11, 2024
6aad7a7
Fix for scheduler
hlky Dec 11, 2024
c1db3bd
Update tests/pipelines/test_pipelines.py
hlky Dec 11, 2024
50b740a
Merge branch 'main' into check-model-passed-pipeline
hlky Dec 11, 2024
b8fa81a
EnumMeta
hlky Dec 11, 2024
44f24a4
Flax
hlky Dec 11, 2024
5af8c7f
scheduler in expected types
hlky Dec 11, 2024
baea141
make
hlky Dec 11, 2024
c5e1e2d
type object 'CLIPTokenizer' has no attribute '_PipelineFastTests__name'
hlky Dec 11, 2024
dba12b6
support union
hlky Dec 11, 2024
99b0f92
fix typing in kandinsky
hlky Dec 11, 2024
3a43c8a
make
hlky Dec 11, 2024
803e33f
add LCMScheduler
hlky Dec 11, 2024
c81415b
'LCMScheduler' object has no attribute 'sigmas'
hlky Dec 11, 2024
13a824e
tests for wrong scheduler
hlky Dec 11, 2024
5679067
make
hlky Dec 11, 2024
24d79a3
update
hlky Dec 13, 2024
3f841d5
warning
hlky Dec 13, 2024
f18687f
tests
hlky Dec 13, 2024
87f8f03
Update src/diffusers/pipelines/pipeline_utils.py
hlky Dec 16, 2024
56ac8b4
import FlaxSchedulerMixin
hlky Dec 16, 2024
87dcf54
skip scheduler
hlky Dec 16, 2024
e193563
Merge branch 'main' into check-model-passed-pipeline
sayakpaul Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/diffusers/pipelines/pipeline_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,13 @@ def load_module(name, value):

init_dict = {k: v for k, v in init_dict.items() if load_module(k, v)}

for key, (_, expected_class_name) in zip(init_dict.keys(), init_dict.values()):
Copy link
Collaborator

@DN6 DN6 Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since we already have the types extracted in expected_types can't we fetch them using the key and then check if the passed object is an instance of the type? If the expected type is an enum then we can check if the passed obj class name exists in the keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#10189 (comment)

Copy link
Collaborator

@DN6 DN6 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it might be better to make this check more agnostic to the component names.

We have a few pipelines with Union types on non-scheduler components (mostly AnimateDiff). So this snippet would fail even though it's valid, because init_dict is based on the model_index.json which doesn't support multiple types.

from diffusers import (
	AnimateDiffPipeline,
	UNetMotionModel,
)

unet = UNetMotionModel()
pipe = AnimateDiffPipeline.from_pretrained(
	"hf-internal-testing/tiny-sd-pipe", unet=unet
)

Enforcing scheduler types might be a breaking change cc: @yiyixuxu . e.g. Using DDIM with Kandinsky is currently valid, but with this change any downstream code doing this it would break. It would be good to enforce on the pipelines with Flow based schedulers though? (perhaps via a new Enum)

I would try something like:

        for key, (_, expected_class_name) in zip(init_dict.keys(), init_dict.values()):
            if key not in passed_class_obj:
                continue

            class_obj = passed_class_obj[key]
            _expected_class_types = []
            for expected_type in expected_types[key]:
                if isinstance(expected_type, enum.EnumMeta):
                    _expected_class_types.extend(expected_type.__members__.keys())
                else:
                    _expected_class_types.append(expected_type.__name__)

            _is_valid_type = class_obj.__class__.__name__ in _expected_class_types
            if isinstance(class_obj, SchedulerMixin) and not _is_valid_type:
                # Handle case where scheduler is still valid 
                # raise if scheduler is meant to be a Flow based scheduler?
            elif not _is_valid_type:
                raise ValueError(f"Expected types for {key}: {_expected_class_types}, got {class_obj.__class__.__name__}.")

Copy link
Contributor Author

@hlky hlky Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Added this for scheduler

                _requires_flow_match = any("FlowMatch" in class_type for class_type in _expected_class_types)
                _is_flow_match = "FlowMatch" in class_obj.__class__.__name__
                if _requires_flow_match and not _is_flow_match:
                    raise ValueError(f"Expected FlowMatch scheduler, got {class_obj.__class__.__name__}.")
                elif not _requires_flow_match and _is_flow_match:
                    raise ValueError(f"Expected non-FlowMatch scheduler, got {class_obj.__class__.__name__}.")

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need a value error here, a warning is enough, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A warning should be sufficient, it's mainly for the situation here #10093 (comment) where the wrong text encoder is given because the resulting error is uninformative.

Copy link
Collaborator

Choose a reason for hiding this comment

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

let's do a warning then:)

Copy link
Member

Choose a reason for hiding this comment

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

Just chiming here a bit to share a perspective as a user (not a strong opinion). Related to #10189 (comment).

Here

https://github.com/huggingface/diffusers/blob/main/src/diffusers/pipelines/pipeline_loading_utils.py#L287-L290

if there's an unexpected module passed we raise a value error. I think the check is almost along similar lines -- users are passing assigning components that are unexpected / incompatible. We probably cannot predict the consequences of allowing the loading without raising any errors but if we raise an error, users would know what to do to fix the in correct behaviour.

if key not in passed_class_obj:
continue
class_name = passed_class_obj[key].__class__.__name__
if class_name != expected_class_name:
raise ValueError(f"Expected {expected_class_name} for {key}, got {class_name}.")
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a test for this too?


# Special case: safety_checker must be loaded separately when using `from_flax`
if from_flax and "safety_checker" in init_dict and "safety_checker" not in passed_class_obj:
raise NotImplementedError(
Expand Down
Loading