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

Handle async funcs within sync scopes #1547

Merged
merged 10 commits into from
Jun 14, 2022
Merged

Handle async funcs within sync scopes #1547

merged 10 commits into from
Jun 14, 2022

Conversation

omerXfaruq
Copy link
Contributor

@omerXfaruq omerXfaruq commented Jun 13, 2022

Description

  1. Creates utility functions for running async coroutines within sync scopes. It was quite troublesome to create proper solutions for this, finally resolved.
  2. Gives async support to using demo("input") programmatically. Which was introduced in a recent PR recently that I can't find right now 🐱

- save temporary work
- handle async functions in sync domain both in background or sync
@omerXfaruq omerXfaruq changed the title Handle async funcs Handle async funcs within sync scopes Jun 13, 2022
@abidlabs
Copy link
Member

@farukozderim is there any way we can avoid adding another dependency to gradio? Especially as fsspec has < 500 stars.

I took a quick look at the fsspec.asyn.sync() function and it doesn't seem to be too complex, can we just implement it as a utility function ourselves? https://filesystem-spec.readthedocs.io/en/latest/_modules/fsspec/asyn.html

@omerXfaruq
Copy link
Contributor Author

omerXfaruq commented Jun 14, 2022

Why do you want to avoid adding another dependency?

I think this is a needed dependency, why implement a complex functionality that deals with async, threading and multiple event loops when there is a library handling it? I just played around for a bit, and I don't think it will be easy or quick to get it safe&right ourselves and adding the dependency causes no problems, we should add a dependency when we need it.

Btw I don't think fsspec having 500 stars is a dangerous situation, because it has very decent maintainers/contributors.

@abidlabs
Copy link
Member

Why do you want to avoid adding another dependency?

For all of the standard reasons: security, increased installation time, dependency conflict, etc. I'm not totally against adding a new dependency, but since it looks like we are using a single function, purely written in Python, from this dependency (or am I mistaken about that?), I think we should at least consider implementing it ourselves

@omerXfaruq
Copy link
Contributor Author

omerXfaruq commented Jun 14, 2022

I think we should at least consider implementing it ourselves

Agreed, this sounds good but

I think this is a needed dependency, why implement a complex functionality that deals with async, threading and multiple event loops when there is a library handling it? I just played around for a bit, and I don't think it will be easy or quick to get it safe&right ourselves

Additionally, I think that repeating complex processes is unnecessary, and resource consuming. And If we're going to copy&paste the related functions I would prefer to use the library instead. Libraries exist for this reason 😄

Open to suggestions about this or you can also take it if you think it'd help a lot or see a clean&easy way of implementing it ourselves.

Could also take @aliabid94's opinion on this, it might help.

@aliabid94
Copy link
Collaborator

there was a discussion in hf tech slack channel today actually about importing vs reimplementing libraries, seems like the general idea was just reimplement to keep a library lightweight if only using a small part of it.

@omerXfaruq
Copy link
Contributor Author

I don't think we're currently a lightweight project(considering our dependencies and tarball size), and reimplementing a complex library handling multiple event loops + multithreading + async coroutines does not look like a good approach to me 😸

@omerXfaruq
Copy link
Contributor Author

omerXfaruq commented Jun 14, 2022

Let's merge this PR for now to unblock this, and remove the fsspec dependency if we implement the functionality ourselves in the future?

@abidlabs
Copy link
Member

Just taking a look into the PR now @farukozderim!

Gradio (pronounced GRAY-dee-oh) is an open-source Python library that is used to build machine learning and data science demos and web applications.

With Gradio, you can quickly create a beautiful user interface around your machine learning models or data science workflow and let people "try it out" by dragging-and-dropping in their own images, pasting text, recording their own voice, and interacting with your demo, all through the browser.
"guides/getting_started.md", AND THEN RUN: "python render_readme.py" -->
Copy link
Member

@abidlabs abidlabs Jun 14, 2022

Choose a reason for hiding this comment

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

Why are these changes inside the gradio.egg-info here? Can we remove them to clean up the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you talking about all of them or some of them?
These are automatically generated when I used bash install_gradio.sh, it seems my machine generates it a little different, could you also try running it on your machine, maybe that will fix them?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting ok

@abidlabs
Copy link
Member

Ok yeah it seems like we would also have to implement fsspec.asyn.get_loop() which requires rewriting a lot of code ourselves, so agree with @farukozderim let's just incorporate this dependency.

Tested and everything works well. LGTM!

@omerXfaruq omerXfaruq merged commit 04a0764 into main Jun 14, 2022
@omerXfaruq omerXfaruq deleted the handle_async_funcs branch June 14, 2022 19:19
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.

3 participants