-
Notifications
You must be signed in to change notification settings - Fork 571
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
Properly close session in AsyncInferenceClient
#2496
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
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.
Awesome, thanks @Wauplin!
def __del__(self): | ||
if len(self._sessions) > 0: | ||
warnings.warn( | ||
"Deleting 'AsyncInferenceClient' client but some sessions are still open. " | ||
"This can happen if you've stopped streaming data from the server before the stream was complete. " | ||
"To close the client properly, you must call `await client.close()` " | ||
"or use an async context (e.g. `async with AsyncInferenceClient(): ...`." | ||
) |
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.
Out of curiosity, shouldn't this just close the session as well?
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.
Well well well...
I got a hard time getting to make it work. The thing is that you don't know when __del__
is called by Python's garbage collector. Since self.close
is an async function, you need an active event loop to make the close method run -which is not guaranteed. I first tried some hacks but gave up given the complexity compared to having a proper close
method or using an async context manager.
This answer on stackoverflow also convinced me of doing things like this.
Thanks for the review! |
Close #2493.
This PR adds the possibility to properly close sessions from a
AsyncInferenceClient
object using eitherclose
or a context manager:or a context manager:
If some sessions are still opened when a client is being deleted, a warning is emitted to the user to tell them how to properly handle things.
Thanks @tomjorquera for the very well described issue which helped a lot building this PR :)