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

gr.Number() max/min limit #3991

Merged
merged 177 commits into from
Jun 11, 2023
Merged

gr.Number() max/min limit #3991

merged 177 commits into from
Jun 11, 2023

Conversation

dawoodkhan82
Copy link
Collaborator

Description

Building off of artegoser PR (which is merged into this branch). Adding frontend support for min/max of gr.Number() component.

As of now, the border color for the input turns red when a number out of range is typed in.

Screenshot 2023-04-27 at 3 57 34 PM

Please include:

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

Closes: #934

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.

artegoser and others added 3 commits April 27, 2023 08:50
* feat: add min max handling for Number

* Update CHANGELOG.md

* fix: Error when min or max is not specified

* fix formatting

* fix: error when nothing is specified

* Just put infinity in min and max

---------

Co-authored-by: Dawood Khan <[email protected]>
@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Apr 27, 2023

🎉 The demo notebooks match the run.py files! 🎉

@gradio-pr-bot
Copy link
Collaborator

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

@abidlabs
Copy link
Member

Very cool @dawoodkhan82! This is the first time we're doing any sort of validation in the frontend, so cc @pngwn @aliabid94 @gary149 on the design piece.

gradio/components.py Outdated Show resolved Hide resolved
Co-authored-by: Abubakar Abid <[email protected]>
@abidlabs
Copy link
Member

I tested this code:

import gradio as gr

gr.Interface(lambda x:x, gr.Number(min=10, max=20), gr.Number(min=15, max=25)).launch()

and on the default theme, it looks and works great:

image

However, on all of the other themes, I don't see any validation error bar:

import gradio as gr

gr.Interface(lambda x:x, gr.Number(min=10, max=20), gr.Number(min=15, max=25), theme="monochrome").launch()

image

image

@artegoser
Copy link
Contributor

Wouldn't it be better if all input was red instead of borders? It would look more noticeable and most likely be displayed in all themes.

@abidlabs
Copy link
Member

I think the border turning red like how @dawoodkhan82 has implemented it is more common, from what I've seen, e.g. see the examples here: https://www.nngroup.com/articles/errors-forms-design-guidelines/

@artegoser
Copy link
Contributor

Okay, I changed my mind.

@aliabid94
Copy link
Collaborator

So if a user submits a number out of range, it gets clamped in the backend? That might be a bit "magical", a user might think the function ran on what they entered rather than the clamped value.

Proposal that may be out of the scope of this: I think we need to allow blocking an event trigger if an input component has an invalid input. Another use-case might be an optional kwarg, that when set to False, behaves as such:

  • for Image/Audio/File/etc component , something must be uploaded for that event to trigger
  • for Dropdown/Radio/CheckboxGroup with optional=False, something must be selected for that event to trigger
  • Textboxes/Numbers cannot be empty

If an event tries to trigger with an empty optional=False component, the StatusTracker for that component could become a red outline, similar to how in "generating" mode the StatusTracker has a glowing orange outline.

@abidlabs
Copy link
Member

So if a user submits a number out of range, it gets clamped in the backend? That might be a bit "magical", a user might think the function ran on what they entered rather than the clamped value.

Ohh good point @aliabid94. Maybe we raise a gr.Error() instead of clamping the values?

Proposal that may be out of the scope of this: I think we need to allow blocking an event trigger if an input component has an invalid input. Another use-case might be an optional kwarg, that when set to False, behaves as such:

Yup this would be handy also to block the Submit button while a file is uploading, but out of scope of this PR for sure

@artegoser
Copy link
Contributor

Maybe then it would be better to limit the number in the frontend, too, so that the user can't add a number that is out of bounds. When the number changes, check it and limit it if necessary.

@abidlabs
Copy link
Member

abidlabs commented May 2, 2023

Maybe we raise a gr.Error() instead of clamping the values?

Can we do the same for Slider @dawoodkhan82? Would close #3878

@abidlabs
Copy link
Member

abidlabs commented Jun 2, 2023

Let's get this merged in @dawoodkhan82. For now, the only change that needs to be made is that we should raise a gr.Error() if a user submits a value that is outside of the range of the gr.Number(), instead of clamping it.

Later on, we can implement @aliabid94's suggestion, which is to block the submit button altogether

cc @hannahblair if you have any suggestions on validation of the gr.Number()

github-actions bot and others added 11 commits June 2, 2023 10:42
* [create-pull-request] automated change

* Trigger Build

---------

Co-authored-by: aliabd <[email protected]>
Co-authored-by: aliabd <[email protected]>
…nSaver`) (#3973)

* Draft for a safer HuggingFaceDatasetSaver

* Rename (and replace) gr.SaferHuggingFaceDatasetSaver as gr.HuggingFaceDatasetSaver

* update changelog

* ruff

* doc

* tmp work

* merge 2 classes

* remove useless code

* adapt tests

* Update gradio/flagging.py

Co-authored-by: Abubakar Abid <[email protected]>

* Update CHANGELOG.md

* fix typing

* code formatting

* removed print from tests

* removing imports

* removing imports

* fix paths

* formatting

* wording

* formating

* fix tests

---------

Co-authored-by: testbot <[email protected]>
Co-authored-by: Abubakar Abid <[email protected]>
* state render

* add test

* formatting

* changelog
* allow decoding b64 string without headers

* install gradio-client in edittable mode

* update GH actions

* add test for decoding headerless b64

* add test for decoding headerless b64 image

* some linting

* fix test

---------

Co-authored-by: Abubakar Abid <[email protected]>
* matplotlib-agg

* fix

* context manager

* Update CHANGELOG.md

* update demos

* linting

* removed warning

* fix test

* fixes

* fix tests
* fastapi guide

* changelog

* writing

* finish guide

* fix

* Update guides/06_client-libraries/fastapi-app-with-the-gradio-client.md

Co-authored-by: Ali Abdalla <[email protected]>

* Update guides/06_client-libraries/fastapi-app-with-the-gradio-client.md

Co-authored-by: Ali Abdalla <[email protected]>

* Update guides/06_client-libraries/fastapi-app-with-the-gradio-client.md

Co-authored-by: Ali Abdalla <[email protected]>

* Update guides/06_client-libraries/fastapi-app-with-the-gradio-client.md

Co-authored-by: Ali Abdalla <[email protected]>

* dependencies

* html

---------

Co-authored-by: Ali Abdalla <[email protected]>
…emp files are created (#4047)

* temporary file

* tests

* formatting

* rename

* added another test

* guide

* formatting

* changelog

* added custom gradio temp directory (#4053)

* added custom gradio temp directory

* Update 03_sharing-your-app.md

* rename test

* address review

* remove print
* first pass

* fixes

* more fixes

* remove breaks

* format

* version

* pr fixes

* changelog

* test fix

* background color

* format

* revert test fix

* changes

* changes

* test

* changes

* changes

* changes

* changes

* changes

---------

Co-authored-by: Abubakar Abid <[email protected]>
Co-authored-by: Ali Abid <[email protected]>
aliabid94 and others added 4 commits June 6, 2023 18:18
* changes

* changes

---------

Co-authored-by: Abubakar Abid <[email protected]>
@aliabid94
Copy link
Collaborator

In Slider, we've called the arguments minimum and maximum. I feel like we should use the same arg names (could change slider to use min and max, and then pull minimum and maximum out of kwargs with a warning for backwards compatibility)

@aliabid94
Copy link
Collaborator

The error border theme variable that you've changed also affects the error messages. See below the pink borders.
Screen Shot 2023-06-07 at 2 30 58 PM
Screen Shot 2023-06-07 at 2 32 18 PM

@dawoodkhan82 dawoodkhan82 merged commit e9aa9ee into main Jun 11, 2023
@dawoodkhan82 dawoodkhan82 deleted the number-limit branch June 11, 2023 01:12
@akx
Copy link
Contributor

akx commented Jun 12, 2023

How are there 177 commits in this PR..?

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.

Implement minimum and maximum to the number input