-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Client: Support endpoints that return layout components #4871
Client: Support endpoints that return layout components #4871
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4871-all-demos You can install the changes in this PR by running: pip install https://gradio-builds.s3.amazonaws.com/f2fd37ee59cd91467ad43942fe2a2730cbdfe0d9/gradio-3.36.1-py3-none-any.whl |
🎉 Chromatic build completed! There are 24 visual changes to review. |
UI and gradio unit tests should pass when new version of the client is released so marking as ready for review now! |
Instead of making "row", etc. all serializable components, wouldn't a simpler approach be to modify Line 505 in 5dc445b
to include "row", etc. Then, we could just remove this logic altogether: Line 520 in 5dc445b
I think that's more appropriate since the layouts are not actually serializable and we shouldn't treat them as such. |
@@ -427,7 +427,7 @@ def view_api( | |||
|
|||
# Versions of Gradio older than 3.29.0 returned format of the API info | |||
# from the /info endpoint | |||
if version.parse(self.config.get("version", "2.0")) > version.Version("3.29.0"): | |||
if version.parse(self.config.get("version", "2.0")) > version.Version("3.36.1"): |
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.
Is this >
on purpose (c.f. >=
)?
The comment above is also now outdated.
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.
Yes. The next release of gradio will include the updated logic for fetching the api info in this PR and so the app's /info
route can be used.
abca220
to
aec3eda
Compare
@abidlabs The skip_components in |
aec3eda
to
f90ac58
Compare
Nice, I missed this. But if we are already skipping the layout components, why do we need to add serializers for them? Imo that doesn't make sense because the serializers should only exist for components that can hold values since they define how this value is serialized between different formats. We should instead "skip" the serialization of these components. (Skipping could just mean the identity function). Maybe its just semantics so happy to sync if that's easier |
In I guess I I could remove the layout components from the assert (
component_name in serializing.COMPONENT_MAPPING
), f"Unknown component: {component_name}, you may need to update your gradio_client version."
serializer = serializing.COMPONENT_MAPPING[component_name]
serializers.append(serializer()) # type: ignore |
I think we should replace: else:
assert (
component_name in serializing.COMPONENT_MAPPING
), f"Unknown component: {component_name}, you may need to update your gradio_client version."
deserializer = serializing.COMPONENT_MAPPING[component_name] with elif component_name in utils.SKIP_COMPONENTS:
deserializer = serializing.SimpleSerializable
else:
assert (
component_name in serializing.COMPONENT_MAPPING
), f"Unknown component: {component_name}, you may need to update your gradio_client version."
deserializer = serializing.COMPONENT_MAPPING[component_name] And then just remove these from serializing.COMPONENT_MAPPING: "row": SimpleSerializable,
"column": SimpleSerializable,
"tabs": SimpleSerializable,
"tab": SimpleSerializable,
"box": SimpleSerializable,
"form": SimpleSerializable,
"accordion": SimpleSerializable,
"group": SimpleSerializable, That reduces the footprint of the PR and should make it even easier to maintain since we only need to add components in one place: SKIP_COMPONENTS. |
Followed your suggestion @abidlabs ! |
client/python/gradio_client/utils.py
Outdated
@@ -31,10 +31,22 @@ | |||
CONFIG_URL = "config" | |||
API_INFO_URL = "info" | |||
RAW_API_INFO_URL = "info?serialize=False" | |||
SPACE_FETCHER_URL = "https://gradio-space-api-fetcher-v2.hf.space/api" | |||
SPACE_FETCHER_URL = "https://freddyaboulton-space-api-fetcher.hf.space/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.
We just need to remember to update this
@@ -22,7 +22,9 @@ def test_duplicate(serializer_class): | |||
def test_check_component_fallback_serializers(): | |||
for component_name, class_type in COMPONENT_MAPPING.items(): | |||
# skip components that cannot be instantiated without parameters | |||
if component_name in ["dataset", "interpretation"]: | |||
if component_name in ["dataset", "interpretation"] + list( | |||
SKIP_COMPONENTS - {"state"} |
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.
Should we just add ["dataset", "interpretation"]
to SKIP_COMPONENTS and remove them from COMPONENT_MAPPING?
Small nit above. Code looks good + tested. LGTM thanks @freddyaboulton |
2f1d883
to
5022927
Compare
5022927
to
62469d6
Compare
Thanks for the review @abidlabs !! |
Description
Closes: #4836
If an endpoint returns a layout component, it will not be returned by the api but all other IOComponents will (excluding State). These components are excluded from the view_api page as well. But the endpoint is still considered valid for the client.
🎯 PRs Should Target Issues
Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.
Not adhering to this guideline will result in the PR being closed.
Tests & Changelog
PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests:
bash scripts/run_all_tests.sh
You may need to run the linters:
bash scripts/format_backend.sh
andbash scripts/format_frontend.sh
Unless the pull request is labeled with the "no-changelog-update" label by a maintainer of the repo, all pull requests must update the changelog located in
CHANGELOG.md
:Please add a brief summary of the change to the Upcoming Release section of the
CHANGELOG.md
file and includea 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)
".