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

XY Grid #1210

Closed
wants to merge 14 commits into from
Closed

XY Grid #1210

wants to merge 14 commits into from

Conversation

ClayShoaf
Copy link
Contributor

@ClayShoaf ClayShoaf commented Apr 15, 2023

EDIT: It should be ready to go. I would like for at least one other person to test it before I take this out of draft again. I still kind of want to add LoRA to the axis options, but I'm hesistant to add it until the whole monkeypatch thing is fully worked out.

For prompts, they are comma separated, but if you want to include commas in a prompt, you can enclose it in quotes, like so:

this is a prompt, "this is a prompt, but with a comma", this is a third prompt

There is still some vestigial stuff in here from trying to figure out how to get the information correctly. It will be cleaned up and improved in the coming days.

Here is an example output:
image

(if you want to track my progress, I'm working on this branch before merging "major" commits.)

Feedback is welcome!

@Loufe
Copy link
Contributor

Loufe commented Apr 15, 2023

You may consider adding the "draft" label if you don't mean for this to be merged right away.

@ClayShoaf ClayShoaf marked this pull request as draft April 15, 2023 03:46
@ClayShoaf
Copy link
Contributor Author

@Loufe I wasn't aware of that. Thank you!

Cleaned up the interface a little bit. It will now also write the output
to an html file and provide a link to open the file for better viewing.
Looks a lot better now and you can choose characters now too.
TODO:
- better error handling, like for a comma at the beginning of a line
- clean up the code. I know a lot of stuff is not optimal yet
- make an easier way to add new axis options
- add more axis options
	- LoRA
	- Seed
	- Individual parameters
	- Models (probably way down the road)
- checkbox for "constant seed"
@ClayShoaf ClayShoaf marked this pull request as ready for review April 19, 2023 02:22
@ClayShoaf
Copy link
Contributor Author

ClayShoaf commented Apr 19, 2023

Ok, I think I'm finally ready for review. I'll probably add more axis options tomorrow. Specifically, I want to add options for individual parameter presets (shouldn't be hard at all). I'd also like to add LoRAs and models to the mix, but I think I'll wait until they are a little easier to use and the standards are a little more nailed down. I also need to prune down the tracked keys from shared.gradio. I just captured everything while I was working on this, because I wasn't sure exactly what I would need. I'll work on that stuff tomorrow.

I think it looks pretty good in the web interface, and I did a fair bit of bug testing, but there's nothing like sending something out to the public to really figure out where the problems are. Would appreciate any help with tightening up my code, as I'm still a novice. It looked a lot jankier before than it does now, though.

Here's an updated picture of the UI with a sample generation. I asked them all the same question for testing. I also intentionally made the axis values weird to show how error handling works. If you don't use "characters" for an axis, it will use the currently selected character. If you don't use "prompts" for an axis, it will pull the prompt from the regular input textbox.

image

@oobabooga oobabooga added the extensions Pull requests concerning extensions and not the core functionality of the web UI. label Apr 19, 2023
Adding these axis options was causing weird bugs to start appearing
that, as far as I can tell, should have appeared before. I added some
logic to the main run function to clean them up. It's getting pretty
crowded in there though. I might move the run function to its own file.
Not sure yet. I also changed some formatting around and got rid of some
variables. Still haven't pruned the trackers.
when you load characters, the instruction following characters will also
be loaded with the "instruction-following/prefix"
@kaiokendev
Copy link
Contributor

@ClayShoaf
I think you need to clear the history buffer before each run, as it is contaminating the prompt for subsequent generations. A user noted the results of my LoRA from the grid implied the LoRA was overfitted, but from my debugging it is populating the context with the first response generated, which inherently fits future generations to produce a similar response

grid bug fixed

@ClayShoaf
Copy link
Contributor Author

@kaiokendev Good catch! That fix will work for this particular use case, but it's a little more complicated, because we don't necessarily want to remove all of the history in all cases. This would make the "use chat history" button non-functional, for when people want to include the whole history to influence a prompt. I'll figure out where and what exactly needs to be changed on Monday. I just kind of threw instruct mode in there as an afterthought, because I had a feeling people might want to use that mode.

I'll go ahead and throw this back into draft mode until it is working properly. I also need to fix the way that "-1" is handled for the seed options. I didn't think about this until today, but if you put in a seed of "-1" on one of the axes, it's going to generate a new seed for each cell, rather than the whole column/row.

I also want to change how the table is output to make the columns uniform in size.

@ClayShoaf ClayShoaf marked this pull request as draft April 22, 2023 23:52
@ClayShoaf
Copy link
Contributor Author

ClayShoaf commented Apr 23, 2023

@kaiokendev I am not able to test it right now, but I took a quick look, and I suspect, in line 347 (as well as it's respective duplicates in other parts of the code), if > is changed to >=, that may fix it. I know I didn't include that for no reason, though. So, it might cause other errors. I remember I was getting errors while working on it, but I can't remember the exact variables. I believe the issue was that chatbots generally have an introductory message, while instruct mode characters do not. This can create a problem if the introductory message is expected for the bot to behave as expected, but it is removed from the prompt. Again, I will dig into this on Monday. I'm glad you pointed it out.

I might end up breaking the logic out into different files, for instruct vs. chat, but I'm still not sure on that. I also just want to clean it up a bit as well. The mass of code blocks is becoming a little unwieldy. Maybe that's just the nature of the beast. This project is the most complicated thing I've worked on so far (not a programmer by trade).


# Remove the last outputs, so they don't influence future generations
gen_output.pop()
if len(shared.history['internal']) > 1:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the fix for the context poisoning that was happening with chat mode

- Cleaned up that hideous pile of code blocks. I don't know why I
decided to do it like that originally, but I had a day off with just my
old laptop and it let me think about my formatting better. It looks a
lot cleaner now and should be a lot easier to work on.
- Fixed the context poisoning bug when using instruct mode or instruct characters.
- Slightly changed the way the table is made, so it should look
better when making large grids.
shared.history['visible'].pop()
elif len(shared.history['visible']) == 1:
if shared.history['visible'] == gen_output:
shared.history['visible'].clear()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaiokendev I fixed the context poisoning with this block here. Can you test the new script and see if you find any bugs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how or if I can show more lines of code with my comment, but the instruct portion is at the top of this block, a few lines up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Works correctly! one thing I forgot to mention before -- in the output file saving you should change from

mkdir

to

os.makedirs(output_folder)

So it can make the intermediate folders if they don't already exists
Thanks for fixing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll do that. I was considering putting this in an outputs/xy_grid folder in the root directory, but I don't know how @oobabooga would feel about that. Personally, I hate when extensions output things to their own folder in SD, but it felt a little presumptuous.

I also would like to include models as an axis option, but it's gonna take a little work to figure out exactly how I want to go about getting the correct instruct characters lined up with the correct models. If you were to load three different models with 3 different instruct characters, it would be pretty much useless for 2/3 of the generations.

@oobabooga
Copy link
Owner

Some comments:

  1. This needs a refactor. In the web UI, the parameters are not updated in the backend for every change. Instead, they are all collected at once by the ui.gather_interface_values() function. In server.py, several examples of use of this function can be found.
  2. I agree that having the option of using different models as one of the axis would be very useful. It's not obvious to me how to do that. The closest thing so far is the perplexity evaluation tab, where you can select multiple models from the dropdown and then click on "Evaluate" to evaluate all of them, one at a time.
  3. Saving outputs to the root folder: all extensions so far use the subfolder for saved files (silero comes to mind). It's not too important to worry about that as it can be changed later

@ClayShoaf
Copy link
Contributor Author

  1. This needs a refactor. In the web UI, the parameters are not updated in the backend for every change. Instead, they are all collected at once by the ui.gather_interface_values() function. In server.py, several examples of use of this function can be found.

I'll play around with that. The method I'm using now came from a comment in a discussion thread, because I couldn't figure out how to access those values. I know the way I'm doing it now is pretty hacky.

  1. I agree that having the option of using different models as one of the axis would be very useful. It's not obvious to me how to do that. The closest thing so far is the perplexity evaluation tab, where you can select multiple models from the dropdown and then click on "Evaluate" to evaluate all of them, one at a time.

My initial thought was to either try to parse the model names and auto select the instruct character and include it in the axis textbox with some sort of delimiter, but I think that would look really messy. I also thought about having another dropdown appear for models that would have respective characters for models, but that might be too confusing for an end user, not to mention easy to mess up (although adding pause/stop buttons and live loading the grid should help with that). I'll take a look at the perplexity evaluation and see if I can draw any inspiration (or, you know, just take)

  1. Saving outputs to the root folder: all extensions so far use the subfolder for saved files (silero comes to mind). It's not too important to worry about that as it can be changed later

Yea, that's pretty much been my method so far. Just try to format things the way everyone else is doing it. Personally I'm a fan of camel case, but when in Rome...

I appreciate the reply.

@oobabooga
Copy link
Owner

My initial thought was to either try to parse the model names and auto select the instruct character

This is what the web UI does when you select a model from the dropdown. For instance, if you select a model with "alpaca" in its name, instruct mode with the Alpaca template will be automatically set. It does that based on the information in models/config.yaml

The solution is probably to reuse the get_model_specific_settings function to find the appropriate templates and then manually load them.

https://github.com/oobabooga/text-generation-webui/blob/main/server.py#L254

@ClayShoaf
Copy link
Contributor Author

Allowing server to be imported is a game changer. I will need to look at everything again. I wish I had as much free time this week as I had last week.

@ClayShoaf
Copy link
Contributor Author

@oobabooga I have tried several different methods of ui.gather_interface_values(). I tried it gradio style via the button click and I tried it in a function within my extension. I've tried outputting to a global variable in my script.py as well as shared.gradio['xy_state'] (after creating shared.gradio['xy_state'] = gr.State({k: None for k in shared.input_elements}) in my ui()). I even tried going directly to shared.gradio['interface_state'], hoping that because it was defined in server.py it might have more access to the main interface. All of them have the same problem, which is that any variables that are gathered are only pulling the original values from when the main interface was created. The method I'm currently using is still grabbing the updated variables, but I know that it creates a lot of overhead by tracking every change.

My suspicion is that, because these functions are not being called by extensions.py, they are not being passed the updated values. I'll keep messing with it later today, but that's where I am right now.

@ClayShoaf
Copy link
Contributor Author

I'm waiting for huggingface/peft#430 to be resolved before I continue working on this.

@oobabooga
Copy link
Owner

Closing this for inactivity.

@oobabooga oobabooga closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extensions Pull requests concerning extensions and not the core functionality of the web UI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants