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

controlnet extension - few compat fixes #474

Closed
wants to merge 1 commit into from

Conversation

light-and-ray
Copy link
Collaborator

@light-and-ray light-and-ray commented Mar 3, 2024

In support forge in my extension I've faced external_code no longer has to_processing_unit. And also it produces errors if few units are None

Checklist:

RubenBernardezHernande133z

This comment was marked as off-topic.

Copy link
Contributor

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

I don't think the changes here are well justified.

  • For to_processing_unit, can you share the code which call to_processing_unit?
    • As no code in this repo actually calls to_processing_unit, I think you can implement it in your code, instead of adding the logic here.
  • For None filter, can you provide the code path that None could be passed as ControlNet's script args?

@light-and-ray
Copy link
Collaborator Author

light-and-ray commented Mar 4, 2024

@huchenlei this commit: light-and-ray/sd-webui-replacer@27b965c

For any reason sd-webui works fine with dict and convert it "on fly", when forge required to replace all dicts to units right in p.script_args. And second difference between forge and webui: yes, if in p.script_args I've provided only 1 unit, forge didn't work due to NoneType and assert with type checking

@light-and-ray
Copy link
Collaborator Author

light-and-ray commented Mar 4, 2024

I'm looking the code, and I think I've misunderstood and forge doesn't require replace dicts in p.script_args, and it was the same error about NoneType. So the part with to_processing_unit can be removed if you want. But I think it's better to keep it for backward compatibility. But None should be fixed in any way I think, because the webui works with them

@light-and-ray
Copy link
Collaborator Author

light-and-ray commented Mar 4, 2024

But I think it's better to keep it for backward compatibility.

Btw I still need to check is unit enabled, and maybe other fields in future, and I need to have this function. I can write my analog in my code, but why when it's better to have universal conversion function inside external_code module

Before this I had faced only with different lib path and mask which doesn't require special handling. It doesn't a big deal for extension like mine which just uses cn script and nothing else. But why external_code module shouldn't has the same interface with webui by your opinion, when this module was designed like a lib for usage by external extensions? 😐

I don't think the changes here are well justified.

@light-and-ray
Copy link
Collaborator Author

@huchenlei

Copy link
Contributor

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

waiting for @lllyasviel's review

Panchovix pushed a commit to Panchovix/stable-diffusion-webui-reForge that referenced this pull request Jul 13, 2024
@lllyasviel
Copy link
Owner

hi we are going to close PRs before forge's recent major revision
if we missed some important PRs, please consider reopen (if that is not already on our todo list

@lllyasviel lllyasviel closed this Aug 1, 2024
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.

None yet

4 participants