Form min_width guard #4693
Form min_width guard #4693
Conversation
|
All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4693-all-demos |
|
LGTM, thanks @akx. @aliabid94 could you also take a quick look? I believe you wrote the related code |
| @@ -323,15 +323,16 @@ def __init__(self, *, scale: int = 0, min_width: int = 0, **kwargs): | |||
| scale: relative width compared to adjacent Columns. For example, if Column A has scale=2, and Column B has scale=1, A will be twice as wide as B. | |||
| min_width: minimum pixel width of Column, will wrap if not sufficient screen space to satisfy this value. If a certain scale value results in a column narrower than min_width, the min_width parameter will be respected first. | |||
| """ | |||
| self.scale = scale | |||
| self.min_width = min_width | |||
| self.scale = int(scale) | |||
There was a problem hiding this comment.
why is this casting necessary? the constructor expects an int
There was a problem hiding this comment.
The type annotation doesn't prevent anyone from passing in something else.
This makes e.g. an inadvertent max_width=None (which would be valid for some other components) fail at construction time instead of when something is attempting to do math with it.
There was a problem hiding this comment.
I don't think we should guard against people passing the wrong type in, do we do that anywhere else?
There was a problem hiding this comment.
I see, assert isinstance(min_width, int) would be clearer in intent
There was a problem hiding this comment.
@aliabid94 asserts could be compiled out in -O mode. See https://beta.ruff.rs/docs/rules/assert/
There was a problem hiding this comment.
@pngwn Why not? I think it makes sense here since the overwhelmingly more common types for scale and min_width are
scale: int | None = None,
min_width: int | None = None,
but this component only accepts ints and will break otherwise.
There was a problem hiding this comment.
Yeah I think it's a good idea to check here, even if we don't do it generally.
| Test that `Form.add_child` does not crash with a component whose `min_width` is `None`. | ||
| """ | ||
| r = Row() | ||
| f = Form() |
There was a problem hiding this comment.
this seems to test the internal implementation, using .parent and .add_child. How about this instead, which is how Blocks are naturally built?
with Row() as r:
with Form() as f:
b = Button()
assert f.min_width == 0
There was a problem hiding this comment.
@aliabid94 This doesn't trigger the original bug (for whichever reason) – I did mention I didn't have a great standalone repro :(
There was a problem hiding this comment.
That's really strange, can we figure out why the bug is not being triggered and write a test that captures the real usage?
There was a problem hiding this comment.
I think it has to do with the pseudo_parent logic in fill_expected_parents, and truth be told my eyes glazed over trying to figure out what happens there. 😅
There was a problem hiding this comment.
I don't think this is a reasonable test as it uses an internal unsupported API. We have to reproduce the original problem with our supported API. Was the original bug discovered when using the API in this way as well?
There was a problem hiding this comment.
No, the original bug simply appears when trying to upgrade the stable-diffusion-webui repo to the latest version of Gradio. It doesn't use the internal APIs (but it does have some special subclassed components).
There was a problem hiding this comment.
@akx as a general principle, we can't be making changes to the internals of the gradio library based on nonstandard usage. The fix or modification should be downstream library using gradio. If you can find an example where using gradio as intended produces this bug, then we're happy to fix, but in the meantime I think we should close this PR
There was a problem hiding this comment.
@abidlabs What constitutes "nonstandard usage"? 😁 As I see it, no non-public APIs are being used in the webui.
There was a problem hiding this comment.
We need a simple repro so we can test against regressions without testing implementation details.
|
Closing until we have a better repro: #4693 (comment) |
Description
This PR ensures no weird algebra is being attempted between integers and Nones for
max_width, as was already done forscale, inForm.add_child(ref #4374).On a similar note, it ensures an user attempting to do
Form(min_width=None)will get an error immediately rather than at a later stage.I don't have a great standalone repro for this, but the symptom is