Skip to content

Call fn by api_name when using Blocks.load#2593

Merged
freddyaboulton merged 3 commits into
mainfrom
2523-call-fn-by-api-name
Nov 2, 2022
Merged

Call fn by api_name when using Blocks.load#2593
freddyaboulton merged 3 commits into
mainfrom
2523-call-fn-by-api-name

Conversation

@freddyaboulton
Copy link
Copy Markdown
Collaborator

@freddyaboulton freddyaboulton commented Nov 2, 2022

Description

Fixes #2523

Added unit tests. Can also test with some of the spaces we have in the gradio org.

Checklist:

  • I have performed a self-review of my own code
  • I have added a short summary of my change to the CHANGELOG.md
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

A note about the CHANGELOG

Hello 👋 and thank you for contributing to Gradio!

All pull requests must update the change log located in CHANGELOG.md, unless the pull request is labeled with the "no-changelog-update" label.

Please add a brief summary of the change to the Upcoming Release > Full Changelog 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)".

If you would like to elaborate on your change further, feel free to include a longer explanation in the other sections.
If you would like an image/gif/video showcasing your feature, it may be best to edit the CHANGELOG file using the
GitHub web UI since that lets you upload files directly via drag-and-drop.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 2, 2022

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

@freddyaboulton freddyaboulton marked this pull request as ready for review November 2, 2022 15:48
Comment thread gradio/blocks.py Outdated
Comment on lines 749 to 756
Copy link
Copy Markdown
Member

@abidlabs abidlabs Nov 2, 2022

Choose a reason for hiding this comment

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

Out of curiosity, what is the advantage of this next(generator) approach over (imo the more readable) list.index()?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I like it cause in the case of the api_name not being there, it will return None instead of raising an exception.

Are you suggesting?

try:
    fn_index = [d.get('api_name') for d in self.dependecies].index(api_name)
except ValueError:
    raise (....)    

Happy to switch implementation!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes that's what I was suggesting, but either way is fine! It probably speaks more to my personal comfort that I prefer the second over the first, I don't think there is one that's actually more pythonic

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yea I agree both are equivalently pythonic (or non pythonic 😆 )

I personally prefer if the failure cause of a function is None (or some value like that) as opposed to raising an exception 🤷

Comment thread test/test_external.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggestion -- instead of just checking the type, can we assert the correct value for a more stringent test?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

For sure!

Comment thread test/test_external.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above. Instead of just checking the type, can we assert the correct value for a more stringent test?

Also, I think it would be good to test with a different Space that (A) does not have a time.sleep (B) and has multiple functions that do different things to ensure that the correct function is being called

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This space has multiple functions. I'll make a copy without the sleep!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yup great but it would also be good if the functions were doing different things to ensure that the correct function is being called

@abidlabs
Copy link
Copy Markdown
Member

abidlabs commented Nov 2, 2022

Thanks for adding this @freddyaboulton! Please see a few suggestions above

@freddyaboulton freddyaboulton force-pushed the 2523-call-fn-by-api-name branch from 8f81077 to e931ada Compare November 2, 2022 16:51
@freddyaboulton
Copy link
Copy Markdown
Collaborator Author

Thanks for the review @abidlabs ! Updated the test to use this space which has two separate functions that do separate computations

https://huggingface.co/spaces/gradio/multiple-api-name-test

Copy link
Copy Markdown
Member

@abidlabs abidlabs left a comment

Choose a reason for hiding this comment

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

Amazing, thanks for addressing the suggestions @freddyaboulton!

@freddyaboulton freddyaboulton merged commit 4a51cec into main Nov 2, 2022
@freddyaboulton freddyaboulton deleted the 2523-call-fn-by-api-name branch November 2, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use the api_name when calling an app like a function

2 participants