Conversation
|
All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-2359-all-demos |
freddyaboulton
left a comment
freddyaboulton
left a comment
There was a problem hiding this comment.
@abidlabs Thanks! The changes related to the DuplicateBlockError look good to me but some of the tests are failing due to the refactoring of the analytics payload. What's the intended purpose of that change?
| @@ -1,3 +1,7 @@ | |||
| class DuplicateBlockError(KeyError): | |||
There was a problem hiding this comment.
I think KeyError is usually reserved for when a key in a dictionary is not present? Minor point but I think a ValueError or LookupError would be more appropriate here.
There was a problem hiding this comment.
Sure, will change to ValueError
|
Thanks for the review @freddyaboulton! I added the reason for the analytics refactor above:
I'll fix the remaining tests and make the requested change! |
|
Should be all set to merge now (cc @freddyaboulton). |
| with Column(variant="panel"): | ||
| for component in self.output_components: | ||
| component.render() | ||
| if not (isinstance(component, State)): |
There was a problem hiding this comment.
Out of curiosity - this was a pre-existing bug?
There was a problem hiding this comment.
Originally we would unnecessarily create a second State component with the identical properties as the first. It didn't break anything, but better to avoid it imo.
2 participants
Fixes: #2320
Also avoids making a call to figure out the IP address if analytics is disabled. The reason for that change is that I was working with airplane Wifi and noticed it was extremely slow to launch a Gradio Interface because it was taking a long time to get the IP address. The real solution is to use async requests, but this is a temporary bandaid