ArtVentureX / sd-webui-agent-scheduler

634 stars 67 forks source link

Feedback: mostly very good with some broken items and few suggestions #2

Closed vladmandic closed 1 year ago

vladmandic commented 1 year ago

First, I love the functionality of the extension. And majority of it is coded very cleanly.

But here are few feedbacks:

Main action panel

Last update sort-of went in a wrong direction by adding model selector and restyling entire action column in general.
That breaks some of the functionality as well as a lot of themes and actually changes the behavior of the core repo.
Not to mention that causes massive incompatibility with the new UI which is currently in development.

IMO, extension should create a single button next to generate - nothing more.
(with optional number in the button label that shows number of queued jobs if its greater than zero)
Everything else can be done from the extension tab - like changing which model should be used for each queued operation.

Extension Tab

I don't understand why does the extension tab have its own gallery component instead of reusing main gallery element?

Also, a suggestion - the queue table would benefit from two-line approach where prompt is in first line and all other params (including checkpoint which is now for some reason before prompt) in the second line.

Other than that, I saw you're planning a history in your roadmap - which is a great feature. One additional one I'd suggest on top of that is to add send to ... buttons so settings from historical generate request can be restored and reused - I can help out with that as that functionality can be tricky.

3rd Party JS libraries

Extension uses AG Grid control that throws JS warnings

'ag-grid-enterprise' has not been loaded. Check you are using the Enterprise bundle:

Also, 3rd party JS lib is loaded from CDN which means that extension cannot work offline and will cause runtime errors
Please package all dependencies inside extension itself image

API mode

Although extension does create nice API, it doesn't actually work in API-only (--api-only) mode
and causes runtime errors in the main repo

extensions/sd-webui-agent-scheduler/scripts/helpers.py:35 in get_component_by_elem_id
  34 def get_component_by_elem_id(root: Block, elem_id: str):
  35     if root.elem_id == elem_id:

AttributeError: 'NoneType' object has no attribute 'elem_id'

Optional add to queue without immediate execution

Currently any item added to queue will be immediately executed and user needs to go to extension tab to press pause. Feature request is to add option (using shared.opts so its persistent) to enable/disable execution auto-start.

artventuredev commented 1 year ago

Hi @vladmandic, very appreciate your detailed feedback. I just push an update to address some of the mentioned issues:

And some cool features are under development.

The extension was developed to serve our company's internal team so some UI/UX/features are designed based on our team requests. I'll try to add settings to make the extension more comfortable to use for both internal team and public community.

Once again thank you very much for the feedback.

vladmandic commented 1 year ago

@artventurdev thanks for the quick turnaround! and you've addressed most of my concerns - and no issues with the layout. one trivial one - perhaps "below generate" is better default than "above generate"

in the tab itself, i'm not sure if its a bug or a start of a feature which is not yet implemented, but now i'm seeing two tables and above one is always empty while bottom one has actual jobs?

and i'm guessing you're using licensed version of ag grid javascript library in your environment so not much can be done about warnings?

AG Grid: unable to use sideBar as 'ag-grid-enterprise' has not been loaded. Check you are using the Enterprise bundle: AG Grid: unable to use getContextMenuItems as 'ag-grid-enterprise' has not been loaded. Check you are using the Enterprise bundle:

which does leave one js bug:

AG Grid: tried to call sizeColumnsToFit() but the grid is coming back with zero width, maybe the grid is not visible yet on the screen?

btw, why am i being so nitpicky? because i really love this extension - and with your permission, i'd like to include it in list of default ones.

zaileron commented 1 year ago

Really like the direction this plugin is taking! I'm on @vladmandic 's fork of A1111.

Possible bugs?

  1. While generating images, the preview often flickers with all-static images. They seem to generate fine, but v1111 wasn't doing this before.
  2. Two tables in the agent viewer, similar to vlad. When a bunch of tasks are in there, scrolling is broken.
  3. Unable to select different checkpoints to queue when an operation is in progress. At least the table appears to say that all my enqueued requests are going to be the same open checkpoint.

Other feature request ideas:

  1. Edit tasks in Agent Scheduler
  2. Pause (very) long tasks to allow others to run first, then resume afterward.
  3. Just have the generate button automatically queue tasks when a task is currently running, instead of having different buttons for the same task.
vladmandic commented 1 year ago

I agree with all except pause as that is a blocking function, you can't pause in the middle of the job AND let something else continue.

Instead I'd add reorder queue list to wish list 😁

zaileron commented 1 year ago

Nice!

Admittedly don't know how it might be implemented, but would probably aim to block between image runs (ie. I have a multi-hour X/Y/Z running -- seems like that job could be paused between images then resumed? Might break the grids etc...)

I think on the pull I did earlier tonight I can drag/drop the tasks in different orders. I don't know if that formally reordered them but it looks like it.

artventuredev commented 1 year ago

@vladmandic @zaileron

I forgot to remove the old js file in the public version so the two tables issue. Should be fixed now.

and i'm guessing you're using licensed version of ag grid javascript library in your environment so not much can be done about warnings?

AG Grid: tried to call sizeColumnsToFit() but the grid is coming back with zero width, maybe the grid is not visible yet on the screen?

I did use some of the enterprise features in the internal version, but removed them all. The new warning is about the grid tried to resize the columns while being hidden, so the width and heigh are 0. You can just ignore the warning.

btw, why am i being so nitpicky? because i really love this extension - and with your permission, i'd like to include it in list of default ones.

That would be a great honor for us <3.

artventuredev commented 1 year ago

@zaileron

  1. Edit tasks in Agent Scheduler

Yes I also planned to do this.

  1. Pause (very) long tasks to allow others to run first, then resume afterward.

I think you can only stop a task (all saved images so far will be kept), but you can't just restart at the stopped point.

  1. Unable to select different checkpoints to queue when an operation is in progress. At least the table appears to say that all my enqueued requests are going to be the same open checkpoint.

The image generation is a blocking task so you can't change the checkpoint while a task is running. But you can do this:

I think on the pull I did earlier tonight I can drag/drop the tasks in different orders. I don't know if that formally reordered them but it looks like it.

Yes you can just hold the handle in the beginning of each row then drag/drop to reorder tasks.

zaileron commented 1 year ago

Fantastic! I guess my specific stop/start case is related to X/Y/Z or similar scripts. I can see how that would probably require a fair bit of custom work to figure out. Being able to queue checkpoints is a great step for me, though.

vladmandic commented 1 year ago

@zaileron actually, i don't see an issue with enqueue of xyz scripts - enqueue captures params as-is (which is the best way to do it), it doesn't try to parse them, so you can actually queue up pretty much anything.

vladmandic commented 1 year ago

@artventurdev ignore my previous "can we set default placement ...", i tested few options and...

i would completely get rid of opts.queue_button_placement and replace entire code section

if shared.opts.queue_button_placement

with simple:

    parent = generate.parent.parent
    parent.children.insert(1, row)

that places enqueue button right underneath generate and before second row buttons (stop/skip/pause) i think it's the most logical place for it and any other option is not really necessary.

and one fix to make grid scale correctly:

ui/src/extension/index.ts:

- eGridDiv.style.height = window.innerHeight - 240 + 'px';
+ eGridDiv.style.height = 'calc(100vh - 250px)';

That would be a great honor for us <3.

perfect - its happening soon!

artventuredev commented 1 year ago

that places enqueue button right underneath generate and before second row buttons (stop/skip/pause) i think it's the most logical place for it and any other option is not really necessary.

@vladmandic Agreed on that, but after tried it with the default black-orange theme I have to put it down below the tools. But for other themes that's not an issue.

Screenshot 2023-06-02 at 03 24 19 Screenshot 2023-06-02 at 03 29 53
vladmandic commented 1 year ago

i've updated default theme (and simplified entire action box layout, no elements have absolute/fixed positioning anymore, its all flow based), it should not be an issue anymore.

artventuredev commented 1 year ago

That's awesome. Let me update the Under Generate button placement option.

The Between Prompt and Generate button option is mainly for our internal version. I have some other buttons so put them all below the Generate button would make the UI a bit off.

zaileron commented 1 year ago

@zaileron actually, i don't see an issue with enqueue of xyz scripts - enqueue captures params as-is (which is the best way to do it), it doesn't try to parse them, so you can actually queue up pretty much anything.

Yep works well -- sorry I was probably unclear. I was doing a 2 hour large XYZ run yesterday when I wished I could pause that larger job and make a single image in the middle of it. That was the use case I was thinking a "pause this job" approach would work. Not mid-image, but between images in a script.

Re: X/Y/Z, looks like you can't enqueue an X/Y/Z script that changes the checkpoint. It loads the right checkpoint, then immediately loads the default checkpoint again thereafter.

artventuredev commented 1 year ago

@zaileron Hmm, I tried X/Y/Z with other checkpoints (not the current one), and it worked correctly. The checkpoint was updated during the run and restored to the original at the end, but all generated images used the correct checkpoint. Can you double check the parameters on generated images.

zaileron commented 1 year ago

Updated everything this morning just in case. Pretty specific use-case (I like to iterate an image I like over many checkpoints to see which style I prefer best), but FYI. If this doesn't replicate for you, then can see if there's something else going on, but I have very few extensions, etc., installed. Note, this is Vlad's fork, not A1111.

Here's how I replicate the problem:

1) Insert any prompt, seed, variation seed as needed to get an image that repeats with each generation. (Note, occasionally I see very slight differences between "Generate" and "Enqueue" but mostly they're the same.

2) In script, select X/Y/Z plot.

3) In X, select "Checkpoint name". Select some or all of your checkpoints.

4) If you hit Generate, everything will run as expected. If you hit Enqueue, it will run an appropriate number of images, but they will all be rendered with the same Checkpoint, so you'll wind up with a bunch of near-identical images in your Images folder.

From what I can tell, it loads a couple checkpoints but ultimately comes back to my default (utopia, in this instance), rendering identical images:

image

artventuredev commented 1 year ago

@zaileron Confirm that this issue some how happen with vlad fork, the original A1111 has no issue. I will spend some time investigate it and get back to you.

vladmandic commented 1 year ago

i'll take a look as well.

artventuredev commented 1 year ago

@vladmandic, I believe the root cause of the problem is the combination of the custom checkpoint feature in the extension and the difference in the apply_checkpoint functions.

To be more specific, the extension uses the override_settings['sd_model_checkpoint'] parameter to override the checkpoint used for rendering. In the A1111 webui version, the apply_checkpoint function updates this parameter when applying a checkpoint from the xyz config. However, in your version, this update is missing:

# A1111 version

def apply_checkpoint(p, x, xs):
    info = modules.sd_models.get_closet_checkpoint_match(x)
    if info is None:
        raise RuntimeError(f"Unknown checkpoint: {x}")
    p.override_settings['sd_model_checkpoint'] = info.name
# vlad version
def apply_checkpoint(p, x, xs):
    if x == shared.opts.sd_model_checkpoint:
        return
    info = sd_models.get_closet_checkpoint_match(x)
    if info is None:
        shared.log.warning(f"XYZ grid: unknown checkpoint: {x}")
    else:
        sd_models.reload_model_weights(shared.sd_model, info)

As a result, during the generation process, override_settings['sd_model_checkpoint'] overrides the preferred checkpoint set by the apply_checkpoint function.

vladmandic commented 1 year ago

if that's it, thats a trivial one - i just pushed an update.

Trevor-Z commented 1 year ago
  1. Insert any prompt, seed, variation seed as needed to get an image that repeats with each generation. (Note, occasionally I see very slight differences between "Generate" and "Enqueue" but mostly they're the same.
  2. In script, select X/Y/Z plot.
  3. In X, select "Checkpoint name". Select some or all of your checkpoints.

We can run the X/Y/Z plot with this extension, iterating the queue with different models? If so, what do we choose for the Y type?

I guess I have an even more specific use-case, I want to compare several models by generating the same set of images with each of them. But the closest I got so far to automate this was with the queue extension, which saves the prompts, but doesn't allow me to schedule models for multiple runs.

The X/Y/Z plot would be ideal for this, but it doesn't allow me to queue complete prompts and parameters...

zaileron commented 1 year ago

You don't need the Y type -- just set X as "Checkpoint name" and it'll iterate across any checkpoints you select. It'll respect your batch settings for each checkpoint as well. Quick script to generate 100s of images if you're searching an idea space.

My workflow might be similar to what you're going for. You can use a prompt by itself and then X/Y/Z across checkpoints +/- samplers +/- whatever you want. I'll generally identify a few candidate images and set them aside, then do img2img and ControlNet reference (or T2IA but that seems to error out for me more) across checkpoints with a batch of 4-8 and let it go overnight.

Helps to do a few trial runs to make sure you're getting the level of variation you prefer.

Trevor-Z commented 1 year ago

Took me a while to get it, I thought the whole queue would be processed with the script, but actually it's this extension that saves the script settings for each prompt.

I'll add a feature suggestion to automate tasks like that.

artventuredev commented 1 year ago

@Trevor-Z, if I understand correctly, you're trying to queue a set of P prompts across N different checkpoints, right?

In case you haven't already, I would recommend checking out the Dynamic Prompts extension. It allows you to use prompt syntax like A {house|apartment|lodge|cottage} in {summer|winter|autumn|spring}..., which could be very helpful for your use case.

Trevor-Z commented 1 year ago

It's not quite what that extension offers. I tried to explain some of it here: https://github.com/ArtVentureX/sd-webui-agent-scheduler/issues/12

I'm gathering a set of ~30 prompts that work well in different models and that vary in theme and style (photo, cartoon, pastel painting, portrait, scenery, simple, complex, etc), in the way positive and negative prompts are written, in image size and aspect ratio, use of hires fix, LoRA's, etc.

They'd serve as reference to do systematic comparisons between models, track their origins and changes between versions, to test several merge parameters at once to pick the best one, and to track the progress of training and fine-tuning.

Kind of what these guys did, but with several prompts instead of just one: https://arca.live/w/aiart/%EB%AA%A8%EB%8D%B8

Like I said, right now it takes way more time to manually set up these prompts than to do the merges on which they will be used. I was sure this feature would have been implemented somewhere by now. :laughing:

zaileron commented 1 year ago

Additional feedback point of note, not sure if there's something you can do. When one job CUDA OOM's out, it kills the whole queue. Would be great if it could auto-pause. Instead it errors out the whole queue CUDA'ing out because the memory isn't released.

vladmandic commented 1 year ago

I like that idea and can provide a flag in core to check it oom occurred. But perhaps creating a new feature request is better than "anotger one" in a long thread?

artventuredev commented 1 year ago

Agreed, we definitely should start a new thread.