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 support for kwargs and default arguments in the python client, and improves how parameter information is displayed in the "view API" page #7732
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-builds.s3.amazonaws.com/2c59e5609ef78cbe54501f67ccd81d1666e20a08/gradio-4.22.0-py3-none-any.whl Install Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@2c59e5609ef78cbe54501f67ccd81d1666e20a08#subdirectory=client/python" |
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
Maintainers or the PR author can modify the PR title to modify this entry.
|
Ok I've made the change @freddyaboulton. Still working on the documentation, but should otherwise be good for a review |
{parameter_name | ||
? parameter_name + "=" | ||
: ""}<span class="example-inputs" | ||
>{represent_value( |
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.
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 looks like that but I think there's no space actually, maybe its a padding issue
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.
Interesting. I think ideal would be no padding but not a big deal
guides/08_gradio-clients-and-lite/01_getting-started-with-the-python-client.md
Outdated
Show resolved
Hide resolved
desc = ( | ||
f" ({info['python_type']['description']})" | ||
if info["python_type"].get("description") | ||
else "" | ||
) | ||
default_value = info.get("parameter_default") |
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.
I think we need to traverse the value here to only pull out the actual filepath or url
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.
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.
I'll fix in the .view_api()
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.
Yea sorry that's what I meant!
Ok the docs should be ready as well now! |
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.
Very slick PR @abidlabs ! Tested a variety of demos and they all work well!
one thing that would be nice is to have the URL of the file if it's on spaces otherwise the file upload would fail?
Thanks @freddyaboulton! Agree will figure out a better solution for files |
Ok that should address everything in the review, thanks so much @freddyaboulton: I'll merge in later today unless @aliabd has any other suggestions |
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.
Really like this @abidlabs!
I have two small suggestions that you can make the final call on.
- I feel like the multiple parameters are blending together visually. I know you have the faint orange separator (in light mode), maybe making it more prominent or increasing the space?
- I think 'Defaults to _' and 'Required' should occupy the same position. Right now 'Required' is after the name and type, where as the default value is in the end of the description. I think it would be better if you maybe put them both at the end of the first line, on the right.
Everything else looks so clean!
Thanks for the reviews @freddyaboulton and @aliabd. I agree with your suggestions @aliabd, will make the changes and merge |
This PR adds support for kwargs and default arguments in the Client. In other words, suppose you have the following demo:
When you connect to this app via the client, you can now make queries like this:
Note that parameters to
.predict()
are now supplied with key-word arguments. For backwards compatibility, the old approach using positional arguments continues to work. e.g.but the key-word approach is encouraged because it is more robust and also allows us to take advantage of default arguments. In the example above, we could also do:
taking advantage of the fact that
operation
is"add"
by default, as specified in the initial value of the component.Closes: #3540
Closes: #5344
Closes: #4498
Finally, I've updated the design of the view API page to showcase all this information about each parameter. (parameter names, whether it is required, or the default fallback value if it is not). Here's a partial screenshot: