-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Adds ability to provide a gr.update()
dictionary even if postprocess=False
#2385
Conversation
All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-2385-all-demos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @abidlabs ! Looks good to me - left some minor suggestions.
I'll review the other PR once this one is merged.
continue | ||
block = self.blocks[output_id] | ||
if getattr(block, "stateful", False): | ||
if not utils.is_update(predictions[i]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to your PR but why do we not set the value of a state to be an update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good point. I suppose that if the update includes a value
key, we should assign the value
to the state.
I think this check was introduced because if a function returns values in a component-dictionary format (e.g. {textbox: "hi"}
), we create placeholder updates for the rest of the components, and those placeholder components are a singleton dictionary: {"__type__"="generic_update"}
. And we wouldn't want to assign this dictionary itself to the state.
Let me make a fix and add a test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine if we keep it for now too - don't mean to slow down this PR. I was just curious hehe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok actually gr.State()
doesn't have an update()
method at the moment, so passing in a dictionary of updates for gr.State()
is not supported. There's no strong reason to add update()
when a function can just return the value itself, so I'll leave this out for now.
Thanks for the review and great catches @freddyaboulton! Merging into the other branch |
* fixing examples * adding parameters * postprocess * examples * added tests * changelog * Update CHANGELOG.md * added docstrings * Adds ability to provide a `gr.update()` dictionary even if `postprocess=False` (#2385) * docs * fixes * added docs, fixed test * changelog * fix suggestoins * formatting * fix tests * fix tests
Previously, if
postprocess
wasFalse
in an event trigger, thegr.update()
dictionary would not work. This PR fixes that and can be tested by running the example code in the issue:Fixes: #2378