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

Update 'monkeypatched' controlnet class #4269

Merged
merged 3 commits into from
Aug 17, 2023
Merged

Update 'monkeypatched' controlnet class #4269

merged 3 commits into from
Aug 17, 2023

Conversation

StAlKeR7779
Copy link
Contributor

@StAlKeR7779 StAlKeR7779 commented Aug 14, 2023

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

Related Tickets & Documents

  • Related Issue #
  • Closes #

QA Instructions, Screenshots, Recordings

Added/updated tests?

  • Yes
  • No : please replace this line with details on why tests
    have not been included

[optional] Are there any post deployment tasks we need to perform?

Should be removed when added in diffusers
huggingface/diffusers#4599

@GreggHelt2
Copy link
Contributor

Just want to understand the plan here.
Comparing this monkeypatch for ControlNetModel to diffusers ControlNetModel, looks like all changes in this PR are to integrate changes to diffusers ControlNetModel since the monnkeypatch was created?
But I see you also have a PR out to diffusers: PR #4599: Add encoder_attention_mask argument to controlnet model #4599 to get monkeypatched stuff into diffusers ControlNet Model.
So is the intent still to eliminate the monkeypatch once diffusers PR gets approved?

If so this PR makes sense to me, at least until diffusers PR goes through. I think original purpose of monkeypatch was to support long prompts when using ControlNet? If so I can post on diffusers PR showing how this makes a difference when using long prompts.

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.

LGTM, but we need to remember to pull this out when diffusers is updated. Is there a way you can add some code prior to the monkeypatch being installed that checks whether the current version of diffusers is already patched and issues a log message to this effect?

@lstein lstein enabled auto-merge August 17, 2023 19:41
@lstein lstein merged commit 8323359 into main Aug 17, 2023
8 checks passed
@lstein lstein deleted the fix/sdxl_controlnet branch August 17, 2023 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants