Skip to content

feat(toolbox-llamaindex): Enable sync and async context management for ToolboxClient #307

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

Merged
merged 2 commits into from
Jul 15, 2025

Conversation

anubhav756
Copy link
Contributor

@anubhav756 anubhav756 commented Jul 11, 2025

Similar to #308

This PR enhances ToolboxClient to support both synchronous and asynchronous context managers, improving its flexibility and ensuring proper resource management.

Users can now manage the client's lifecycle using either pattern:

Sync Usage

with ToolboxClient("...") as toolbox:
    # Client is automatically closed here
    ...

Async Usage:

async with ToolboxClient("...") as toolbox:
    # Client is automatically closed here
    ...

Additionally, a close() method has been added to automatically clean up underlying HTTP sessions when the client is garbage collected, preventing potential resource leaks.

@anubhav756 anubhav756 self-assigned this Jul 11, 2025
@anubhav756 anubhav756 marked this pull request as ready for review July 11, 2025 06:28
@anubhav756 anubhav756 requested a review from a team as a code owner July 11, 2025 06:28
@anubhav756 anubhav756 changed the title feat(toolbox-llamaindex): Add context manager support for sync and async clients feat(toolbox-llamaindex): Enable sync and async context management for ToolboxClient Jul 11, 2025
@twishabansal
Copy link
Contributor

Have we decide to close user provided sessions too?

@anubhav756
Copy link
Contributor Author

anubhav756 commented Jul 14, 2025

Have we decide to close user provided sessions too?

To clarify, assuming you're asking whether this implementation might incorrectly close a session provided by the user, the answer is no.

We intentionally do not close sessions that are provided by the user (i.e., not managed internally). This is handled by a check in the core library here.

In the specific context of toolbox-llamaindex, this situation doesn't arise. To support both sync and async methods, we need to run async calls in a background loop/thread. This loop isn't accessible to the user, so the ToolboxClient must always create and manage the session object internally. As a result, there's no mechanism for a user to provide their own session in these packages.

Therefore, the scenario of a user-provided session being incorrectly closed by the client won't happen in toolbox-llamaindex.

Let me know if you have any other questions!

@anubhav756 anubhav756 requested a review from twishabansal July 14, 2025 18:38
@anubhav756 anubhav756 merged commit a5897c8 into main Jul 15, 2025
20 checks passed
@anubhav756 anubhav756 deleted the anubhav-cm-li branch July 15, 2025 06:22
@release-please release-please bot mentioned this pull request Jul 15, 2025
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.

2 participants