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

Follow internal representation in gradio.components.dropdown.Dropdown when updating instruction template choices #5197

Merged
merged 22 commits into from
Jan 9, 2024

Conversation

mamei16
Copy link
Contributor

@mamei16 mamei16 commented Jan 7, 2024

The problem

When gradio.components.dropdown.Dropdown is instantiated with the choices parameter set to a list (as returned by modules.utils.get_available_instruction_templates), the following code on lines 86-92 in gradio/components/dropdown.py converts the list to a list of tuples before assigning it to the self.choices member variable:

self.choices = (
            # Although we expect choices to be a list of tuples, it can be a list of tuples if the Gradio app
            # is loaded with gr.load() since Python tuples are converted to lists in JSON.
            [tuple(c) if isinstance(c, (tuple, list)) else (str(c), c) for c in choices]
            if choices
            else []
        )

When the refresh button is clicked to update the available choices in the dropdown, the choices member variable is updated to be a list of strings:

ui.create_refresh_button(shared.gradio['instruction_template'], lambda: None, lambda: {'choices': utils.get_available_instruction_templates()}, 'refresh-button', interactive=not mu)

However, the Dropdown instance still expects choices to be a list of tuples, so it fails the next time gradio.components.dropdown.Dropdown._warn_if_invalid_choice is called.

The fix

Update the choices member variable with a list of tuples, not a list of strings.


Fixes #5118 and #4313

Checklist:

oobabooga and others added 20 commits December 14, 2023 22:39
@oobabooga
Copy link
Owner

You are likely not using the gradio version specified in the requirements. It works fine for me.

@oobabooga oobabooga closed this Jan 7, 2024
@mamei16
Copy link
Contributor Author

mamei16 commented Jan 7, 2024

According to requirements_amd.txt, the gradio version is specified to be gradio==3.50.* and I have 3.50.2 installed.

@oobabooga oobabooga reopened this Jan 7, 2024
@oobabooga
Copy link
Owner

Okay, I reproduced it now after selecting and loading some templates a few times and then trying to refresh. gr.update is supposed to allow you to change an initialization parameter of the component on the fly. The instruction template dropdown takes

choices=utils.get_available_instruction_templates()

so I don't know why doing gr.update(choices=utils.get_available_instruction_templates()) wouldn't work. Is there a cleaner workaround than converting to a list of tuples? Maybe returning a tuple in utils.get_available_instruction_templates()?

@mamei16
Copy link
Contributor Author

mamei16 commented Jan 7, 2024

modules.ui.create_refresh_button uses setattr directly instead of gr.update (which is deprecated BTW), but gr.update(choices=utils.get_available_instruction_templates()) should also work. In fact, it's being used at the end of modules.chat.upload_character.

One possibility could be to replace the return statement in utils.get_available_instruction_templates() with:

template_list = ['Select template to load...'] + sorted(set((k.stem for k in paths)), key=natural_keys)
return list(zip(template_list, template_list))

But in this case a comment should probably be added to explain why this is being done, e.g.:
"gradio Dropdown expects choices to be a list of lists/tuples of the form (name, value), where name is the displayed name of the dropdown choice and value is the value to be passed to the function, or returned by the function."

@oobabooga
Copy link
Owner

Can you check if this alternative solution fixes the problem? c4e005e

@mamei16
Copy link
Contributor Author

mamei16 commented Jan 9, 2024

That unfortunately doesn't work. Do you happen to know why setattr is used anyway? The commit in stable-diffusion-webui that added create_refresh_button doesn't offer any explanation, and removing lines 244-245:

for k, v in args.items():
    setattr(refresh_component, k, v)

also fixes the issue.

PS: gr.update is not deprecated. I just confused it with gradio.components.dropdown.Dropdown.update, my bad!

@oobabooga
Copy link
Owner

No idea, I just copied and pasted this refresh component. Can you modify this PR to remove those lines? I don't think they are necessary.

@mamei16
Copy link
Contributor Author

mamei16 commented Jan 9, 2024

I see. The latest commit 5303fdc I have pushed only removes the two lines and rolls back the original changes I made.

@oobabooga
Copy link
Owner

Thank you for fixing this!

@oobabooga oobabooga merged commit bec4e0a into oobabooga:dev Jan 9, 2024
@mamei16
Copy link
Contributor Author

mamei16 commented Jan 9, 2024

My pleasure 🤝

PoetOnTheRun pushed a commit to PoetOnTheRun/text-generation-webui that referenced this pull request Feb 22, 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.

2 participants