-
Notifications
You must be signed in to change notification settings - Fork 399
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
Patch nest_asyncio in notebooks only #520
Conversation
Thank you for opening a PR! It was indeed to make interaction with notebooks easier. What’s your use case? |
outlines/base.py
Outdated
"Couldn't patch nest_asyncio because it's not installed. Running in the notebook might be have issues" | ||
) | ||
except ValueError as e: | ||
print("Could not apply nest_asyncio:", e) |
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.
When is ValueError
raised?
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 was there from #481 . Presumably some vllm integration but I didn't run into it myself
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 can remove it
4381b8b
to
b182ea9
Compare
We're trying to use Outlines in some proprietary codebase (thank you for building it!). In one of the tests just importing Btw, I verified that |
b182ea9
to
7c80a84
Compare
7c80a84
to
fca8868
Compare
Thank you for contributing! |
The logic to call
nest_asyncio
was added in 587f9ac presumably to make notebook interactions easier.However, it's a bad practice to unconditionally call global python patches from library code.
It caused some issues with vLLM integration and was patched in #481
We ran into different issues when using Outlines in our codebase. The unittests started hanging on shutdown because of stray asyncio threads.
I propose to make the patch happen only when running inside IPython. According to https://stackoverflow.com/questions/15411967/how-can-i-check-if-code-is-executed-in-the-ipython-notebook it's an ok detention method for notebooks. Other proposed methods are too narrow and e.g. don't capture Colab.
However, even better fix might be to remove this patch all together and instead include it explicitly in the notebooks of choice. @rlouf - do you have more context on what was the original issue?