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

Gui fix #2025

Merged
merged 5 commits into from
Mar 2, 2024
Merged

Gui fix #2025

merged 5 commits into from
Mar 2, 2024

Conversation

wkpark
Copy link
Contributor

@wkpark wkpark commented Mar 2, 2024

  • use sys.executable
  • check *_dir validity and remove remove_doublequote()
  • set scriptdir
  • use gr.update() use gr.*.update()
  • unify Folders for finetune
  • use subprocess.run(run_cmd, shell=True, env=env)
  • check print_only_bool

 - use sys.executable
 - check *_dir validity and remove doublequotes strip
 - set `scriptdir`
 - use gr.update()
 - unify Folders for finetune
 - use subprocess.run(run_cmd, shell=True, env=env)
@bmaltais
Copy link
Owner

bmaltais commented Mar 2, 2024

Is there a reason for reverting back to gr.update? This method has been deprecated by Gradio in recent release.

@wkpark
Copy link
Contributor Author

wkpark commented Mar 2, 2024

Is there a reason for reverting back to gr.update? This method has been deprecated by Gradio in recent release.

Oh! I didn't know that, I'm testing/fixing on gradio 3.43.2 env. for sd-webui extension testing.
will fix it soon fixed now

@bmaltais
Copy link
Owner

bmaltais commented Mar 2, 2024

Is there a reason for reverting back to gr.update? This method has been deprecated by Gradio in recent release.

Oh! I didn't know that, I'm testing/fixing on gradio 3.43.2 env. for sd-webui extension testing. will fix it soon fixed now

The gradio recent update don't need the .update part either... if it is included it will cause the deprecation warning...

According to Gradio gr.Group.update(visible=False), need to be gr.Group(visible=False),

But testing the current commit it does not appear to throw a warning... so I guess this should be ok until it does ;-)

Are you doing this so thew code work well under auto1111 gradio version? He is using gradio==3.41.2 and I am using gradio==3.50.2 so this can lead to gradio bugs with my code if I make use of gradio features not present in 3.41.2...

@bmaltais bmaltais merged commit f8b1f2d into bmaltais:dev Mar 2, 2024
1 check passed
@wkpark
Copy link
Contributor Author

wkpark commented Mar 2, 2024

oops.. gr.*.update() also deprecated... OK. will fix later (or check any possible workaround) and thanks again for your hard work!

  • gr.Component.update() works upto gradio 3.41.2 ~ 3.50.2
  • gr.Component.update() emit error on gradio 4.xx

@bmaltais
Copy link
Owner

bmaltais commented Mar 2, 2024

One possibility would be to backrev the gradio version to align with a1111... but for now don't bother... I think it is still supported and not throwing a fix about the deprecation...

I wish I could start over the GUI code and just "clone" the sd-script folder during setup and leverage that for execution for the GUI.

Right now both are sort of intertwine and not the best... but it is what it is ;-) When I started coding the GUI kohya did not us git so I was copying his "notes" as code in my repo. After we talked a bit more he decided to adopt my gui structure but in his own repo... so there started the mess of code it is ;-)

But with the refactoring you have started to do I could see the possibility of totally removing his code from my repo and just "clone" a specific commit for use instead of carrying his code base in mine... this would prevent users from attempting to commit updates to kohya's code in my repo...

@wkpark
Copy link
Contributor Author

wkpark commented Mar 2, 2024

One possibility would be to backrev the gradio version to align with a1111... but for now don't bother... I think it is still supported and not throwing a fix about the deprecation...

I wish I could start over the GUI code and just "clone" the sd-script folder during setup and leverage that for execution for the GUI.

Right now both are sort of intertwine and not the best... but it is what it is ;-) When I started coding the GUI kohya did not us git so I was copying his "notes" as code in my repo. After we talked a bit more he decided to adopt my gui structure but in his own repo... so there started the mess of code it is ;-)

But with the refactoring you have started to do I could see the possibility of totally removing his code from my repo and just "clone" a specific commit for use instead of carrying his code base in mine... this would prevent users from attempting to commit updates to kohya's code in my repo...

oh!! thank you for the detailed explanation.

yes, that would be one possible way. (+ git submodules or some scripts?)
anyway, your gui frontend is/are so convenient that many people will use it for a while, I think.

@bmaltais
Copy link
Owner

bmaltais commented Mar 2, 2024

I am starting to investigate how feasible it would be...

I will see if I can clone sd-script at the root of my repo, then switch to a specific commit, .gitignore it so it does not turn into a submodule, then update my requirements.txt file to build the library from -e ./sd-script rathen than -e .

If that work then the kohya_gui would be clean of all sd-scripts files and then contain only pure code just for the gui... and all calls for execution would go to the sd-script cloned folder...

Not sure if it is worth doing... but this would probably be the best.

@bmaltais
Copy link
Owner

bmaltais commented Mar 2, 2024

OK... it look like it might work... I did a quick test on a subset of the code (just running lora_gui.py, and there was no issues using the library build from the cloned folder...

Now, this is a significant change and I don't want to introduce too many issues for the work you have started. So how should we proceed with this... let the changes you have done stabilise and then attempt to clense the repo from sd-scripts files... or attempt to do this while being at code refactoring...

@bmaltais
Copy link
Owner

bmaltais commented Mar 2, 2024

@wkpark I have created a dev-pure branch for experimenting... the hard part will be the setup of the right sd-scripts release supported by the gui... and progressing from release to release... but my minimal test is working... I will keep trimming it more and more until it is as clean as possible

@bmaltais
Copy link
Owner

bmaltais commented Mar 2, 2024

@wkpark I think this is really looking good... I am slowly testing each utility and training script to make sure there are no surprises... but I think it is very doable to get to a "pure" gui repo and only leverage a cloned version of sd-scripts release...

So... right now I can do the windows setup cloning using the setup.bat script... but not linux... This will need to be fixed I guess.

Also, I don't know what kind of an effect this might have on your a1111 extension. Are you mostly using windows or linux? If you use linux, would you prefer to figure out the best cloning approach for it?

@bmaltais
Copy link
Owner

bmaltais commented Mar 2, 2024

I have gone over pretty much all the GUI tools and there does not appear to be any errors... so I think this is totally viable...

@wkpark
Copy link
Contributor Author

wkpark commented Mar 3, 2024

@bmaltais you did amazing job!! a1111 extension works well with minor fixes. thanks again!

@bmaltais
Copy link
Owner

bmaltais commented Mar 3, 2024

@wkpark Let's mode the discussion to #2029

I opened it so we can openly discuss issues with the migration and approach so it is the best for both the standalone GUI of exposure to a1111 as an extension

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.

2 participants