-
Notifications
You must be signed in to change notification settings - Fork 161
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
forge and gradio4 compatibility #467
base: main
Are you sure you want to change the base?
Conversation
Hello! Thank you for this effort, it is crucial that the extension will work even after A1111 starts using gradio4. Also, supporting forge is a nice addition. However, can not merge as-is, since this introduces a lot of code duplication. If it ever would be necessary to change GUI for whatever reason, we will need to change it in two different places. This is quite undesirable. There must be a different way. For different argument(s), something like
can be used, or:
For different objects, something like this:
This works thanks to very convinient * and ** python operators. I believe this way we don't need to duplicate code. |
This is fair. I will do a rewrite later in the day. gradio 4 update
But I think the first method will work in most situations. |
c766b45
to
a829ca5
Compare
Okay I still think it requires a bit of testing but I did minimise a lot of repeats. (I also fixed a typo that's been annoying me for a while) I also haven't tested it with all features, but it shouldn't interfere with older gradio3 users (still should test a little more) |
I gave the new code a quick glance and it looks nice. I will look more, test and then merge once I get back to the keyboard. |
I've noticed some issues
|
Thank you!
Look like there's some more work to do. |
@semjon00 So I added fixes for 1+2. I didn't notice any issues with 3. If you don't think this is ready to merge into main we should probably do a branch. |
Found another issue with my fix for 2 |
I've been sort of lurking on this. Has there been any luck getting the extension to work with Forge yet? Thanks |
So in it's current state this pull request should work with Forge, but the last update break backwards compatibility so it now the pull request doesn't work with base auto1111. I'll try to fix it this weekend. |
e4e2ab9
to
f7778f3
Compare
@semjon00 could you give this another check. I tested it on auto1111 and forge and it seemed to work without issues. |
For clarity on my part -- I'm looking for a Depth tab across the top, right? I removed the old extension in Forge and then installed using git pull https://github.com/graemeniedermayer/stable-diffusion-webui-normalmap-script, restart, no tab. The old script also didn't show a tab in Forge. But the 'Depth' tab does show in A1111. What did I overlook? |
You do need to switch to the specific branch. Method 1. in the extension directory using a command like Method 2. If you are using the extensions tabs |
Forge uses gradio version 4 which makes a number of breaking changes.
Some of the most noteworthy
file
type must bebinary
instead for file loadersupdate
on components.gradio-app/gradio#6339
This led me to re-instantiating a number of components. Perhaps this can be further simplified.
I split off all the gradio4 files although maybe there's a clever way to fold them back in.