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

Add Upload Button #2591

Merged
merged 31 commits into from
Nov 21, 2022
Merged

Add Upload Button #2591

merged 31 commits into from
Nov 21, 2022

Conversation

dawoodkhan82
Copy link
Collaborator

@dawoodkhan82 dawoodkhan82 commented Nov 2, 2022

Description

Add an upload button component. Like file upload but in button form. Main use case is for the new improved chatbot coming soon.

Not sure about the style of the button.

Screen.Recording.2022-11-02.at.3.42.57.AM.mov
import gradio as gr

def test(name):
    return name.name

with gr.Blocks() as demo:
    image = gr.Image(interactive=False)
    upload_button = gr.UploadButton()
    upload_button.upload(fn=test, inputs=upload_button, outputs=image)

demo.launch()

Please include:

  • relevant motivation
  • a summary of the change
  • which issue is fixed.
  • any additional dependencies that are required for this change.

Closes: #2062

Checklist:

  • I have performed a self-review of my own code
  • I have added a short summary of my change to the CHANGELOG.md
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

A note about the CHANGELOG

Hello 👋 and thank you for contributing to Gradio!

All pull requests must update the change log located in CHANGELOG.md, unless the pull request is labeled with the "no-changelog-update" label.

Please add a brief summary of the change to the Upcoming Release > Full Changelog section of the CHANGELOG.md file and include
a link to the PR (formatted in markdown) and a link to your github profile (if you like). For example, "* Added a cool new feature by [@myusername](link-to-your-github-profile) in [PR 11111](https://github.com/gradio-app/gradio/pull/11111)".

If you would like to elaborate on your change further, feel free to include a longer explanation in the other sections.
If you would like an image/gif/video showcasing your feature, it may be best to edit the CHANGELOG file using the
GitHub web UI since that lets you upload files directly via drag-and-drop.

@abidlabs
Copy link
Member

abidlabs commented Nov 2, 2022

Very cool @dawoodkhan82! I think this can be very useful in general (and also closes #2062 by the way).

A high-level question: have you considered whether UploadButton be its own component or should it be something that is generated by passing in a parameter to the regular Button class? On one hand, I feel like there is going to be a fair bit of duplicated code, but on the other hand, we might want to add parameters like the ability to restrict upload file types that are specific to uploading.

@dawoodkhan82
Copy link
Collaborator Author

Yeah I thought about this. It didn't make sense to create a parameter on the Button class since, we wanted to specify file types + maybe add more customization to UploadButton in the future.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-2591-all-demos

@pngwn
Copy link
Member

pngwn commented Nov 2, 2022

I don't know if it makes sense to have both Upload and UploadButton. At least one of those is too niche to be a core component, in the same way we don't want two slightly different Image components.

@abidlabs
Copy link
Member

abidlabs commented Nov 2, 2022

@pngwn what is Upload?

@dawoodkhan82
Copy link
Collaborator Author

Upload is only a frontend component that's used by File, Model3D, Video, etc. as a generic upload component

@dawoodkhan82
Copy link
Collaborator Author

@pete do you mean File component and UploadButton are too similar?

@pngwn
Copy link
Member

pngwn commented Nov 2, 2022

Oh yeah, I meant the various uploads we have i guess. In this case it seems File is the most similar.

@abidlabs abidlabs closed this Nov 2, 2022
@abidlabs abidlabs reopened this Nov 2, 2022
@abidlabs
Copy link
Member

abidlabs commented Nov 2, 2022

I'm on the fence about combining File and UploadButton -- there could certainly be shared parameters / internal logic, such as serialization and restricting file types, but on the other hand, there are lots of parameters that differ -- for example, "label" is something that applies to a File, but not an UploadButton, and "variant" is something that applies to UploadButton and not to File. But we could have both sets of parameters and only apply them depending on which version we are using.

One other suggestion though -- in the same way that a File can be used to upload and download, so this should this component in principle. So if we end up deciding to create a separate component, perhaps we call it a FileButton?

@pngwn
Copy link
Member

pngwn commented Nov 2, 2022

I don't really think they should be merged, I don't think that have overlapping APIs and that could be confusing.

The only reason I raise it is that we have rejected component requests in the past for being too niche, I don't know which Upload (Button or File) is the most useful but having both seems overkill. And I don't want us to get into a situation where we overlook this discrepancy when we need something (for whatever reason) but outside contributors have their proposals rejected. Typically we've told people to wait for custom components when this has happened in the past.

I'm not against the idea of an UploadButton particularly.

@dawoodkhan82
Copy link
Collaborator Author

Hmm could it be possible, to add more style parameters to the File component to make it more button-ish. Maybe that could be an alternative to making a new component.

@abidlabs
Copy link
Member

abidlabs commented Nov 3, 2022

I synced with @dawoodkhan82 on this a bit, and here's what we're thinking:

  • Let's make this component a variant of the File component given the numerous similarities. We can add a parameter (what's a good name? "layout"?) that allows a user to select between the two variants (the "button" vs "wide layout")

cc @freddyaboulton @pngwn @aliabid94

@pngwn
Copy link
Member

pngwn commented Nov 4, 2022

Maybe variant="panel" and variant="button"?

CHANGELOG.md Outdated Show resolved Hide resolved
gradio/components.py Outdated Show resolved Hide resolved
@abidlabs
Copy link
Member

Thanks for the fix @dawoodkhan82 -- I can confirm that value parameter works as expected for me. Noticed a couple of other things I added to the review, but besides that, the backend code looks good. Would be great to get a review from @pngwn or @aliabid94 as well for the svelte

@dawoodkhan82
Copy link
Collaborator Author

@abidlabs Actually using value broke the component, since the frontend uses the value to pass back to preprocess. So I created button-label to use to change the name for the button. I know this kinda diverges from the button api tho.

@abidlabs
Copy link
Member

Hmm not a fan of button_label as a parameter name. I suppose we could just call it label now, right?

The other thing is that I think this parameter (button_label or label) should be the first argument, so that you could instantiate an UploadButton more easily. Something like:

gr.UploadButton("Click to Upload")

rather than having to do:

gr.UploadButton(label="Click to Upload")

@aliabid94
Copy link
Collaborator

Just seeing this now - I agree with @pngwn's original opinion that this should just be a kwarg to the File component. Especially the variant solution, that seems very clean. I think the variant="button" could be applied to all upload media types, e.g. Audio, Video, File, etc. And then the label= kwarg just refers to the button text.

Copy link
Collaborator

@aliabid94 aliabid94 left a comment

Choose a reason for hiding this comment

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

LGTM besides small comments

ui/packages/app/src/components/UploadButton/types.ts Outdated Show resolved Hide resolved
gradio/components.py Outdated Show resolved Hide resolved
gradio/components.py Outdated Show resolved Hide resolved
gradio/components.py Outdated Show resolved Hide resolved
@abidlabs
Copy link
Member

abidlabs commented Nov 21, 2022

Pointed out a few remaining nits, but otherwise looks great. Very cool @dawoodkhan82!

I would suggest creating a demo using the UploadButton and adding it under demos in the docstring so that there is some reference code on how to use the component.

@dawoodkhan82 dawoodkhan82 self-assigned this Nov 21, 2022
gradio/components.py Outdated Show resolved Hide resolved
@abidlabs
Copy link
Member

Did another round of testing and looks good @dawoodkhan82! Awesome PR

dawoodkhan82 and others added 2 commits November 21, 2022 13:17
Co-authored-by: Abubakar Abid <abubakar@huggingface.co>
@dawoodkhan82 dawoodkhan82 merged commit b5f2267 into main Nov 21, 2022
@dawoodkhan82 dawoodkhan82 deleted the dawood/upload-button branch November 21, 2022 18:34
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.

Button for uploading images
4 participants