Skip to content
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

Fix loading spaces with api_name=False #4886

Merged
merged 4 commits into from
Jul 12, 2023
Merged

Conversation

freddyaboulton
Copy link
Collaborator

Description

Closes #4884

🎯 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

  1. 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

  2. You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

  3. 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 include
a 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)".

@vercel
Copy link

vercel bot commented Jul 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
gradio ✅ Ready (Inspect) Visit Preview Jul 12, 2023 4:19am

@gradio-pr-bot
Copy link
Collaborator

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4886-all-demos

@freddyaboulton freddyaboulton added the no-visual-update Add this to a PR to skip chromatic deployment and tests label Jul 12, 2023
@gradio-pr-bot
Copy link
Collaborator

Chromatic build successful 🎉

@@ -550,7 +550,7 @@ def _infer_fn_index(self, api_name: str | None, fn_index: int | None) -> int:
if api_name is not None:
for i, d in enumerate(self.config["dependencies"]):
config_api_name = d.get("api_name")
if config_api_name is None:
if config_api_name is None or config_api_name is False:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not writing as if not config_api_name cause the api_name can theoretically be "" which evaluates to false.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@freddyaboulton
Hmm, but I think api_name='' should be considered invalid.

Looks like use via API page becomes weird when it has a method with api_name=''.
In the case of the Space used for testing this PR, the API page is empty.

Also, if a Space has only one method and whose api_name is '', then the API page will become like this:

Though it says there's one endpoint, but no API was shown.

I think we should change to make empty API name invalid (in another PR?).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I opened a new issue for this weird behavior of API page. #4894

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea we can solve that issue in a separate PR!

@freddyaboulton freddyaboulton marked this pull request as ready for review July 12, 2023 04:27
@abidlabs
Copy link
Member

Nice! Code looks good + can confirm that the original issue has been fixed.

Copy link
Collaborator

@hysts hysts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix. LGTM!

@freddyaboulton
Copy link
Collaborator Author

Thanks for the reviews @hysts and @abidlabs !

@freddyaboulton freddyaboulton merged commit 59b492a into main Jul 12, 2023
@freddyaboulton freddyaboulton deleted the fix-bool-false-api branch July 12, 2023 15:09
@pngwn pngwn mentioned this pull request Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-visual-update Add this to a PR to skip chromatic deployment and tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client doesn't work properly when a Space has a method with api_name=False
4 participants