Simplify connection config handling in Python SDK#849
Conversation
🦋 Changeset detectedLatest commit: 0c03293 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
876a7c1 to
a297a30
Compare
a297a30 to
d16de51
Compare
d16de51 to
7570417
Compare
|
One detail: Here we can use |
1395b6e to
8ea3448
Compare
02d91db to
77997f1
Compare
| access_token: Optional[str] = None, | ||
| request_timeout: Optional[float] = None, | ||
| headers: Optional[Dict[str, str]] = None, | ||
| extra_sandbox_headers: Optional[Dict[str, str]] = None, |
There was a problem hiding this comment.
I find it looks weird that you have both headers and extra_sandbox_headers
what about?
default_headers = {'User-Agent': 'xxx'}
def __init__(
headers: Dict[str, str] = {}
):
self.headers = {**default_headers, **headers}There was a problem hiding this comment.
@mishushakov Are you asking why we need two sets of headers?
There was a problem hiding this comment.
We don't want to be sending the "api" headers (like the supabase token) to the sandbox endpoints and we also don't want to send the "sandbox" headers (like the envd access token) to the API, so to prevent mixing them on the requests made from the methods on the Sandbox instance we need two separate sets.
It is possible that it can be handled more elegantly internally, but we are mostly making sure it behaves sanely to the outside now.
|
@jakubno one question - do we still need this? maybe there's a way to better handle this |
|
Can we add docstring for options also? |
@mishushakov I think this is already removed in this PR as it was both duplicated and there was a bug. |
It's basically removed, right? |
They are there: https://github.com/e2b-dev/e2b/blob/8ea3448e18edd64c01d7e723cea0ce561a8db633/packages/python-sdk/e2b/connection_config.py#L15 |
7be1034 to
4c0e609
Compare
You mean for the |
I could only add something like But there's a risk it will be obsolete |
| with ApiClient( | ||
| config, | ||
| limits=SandboxApiBase._limits, | ||
| limits=cls._limits, |
There was a problem hiding this comment.
Bug: API Client Limits Increased After Class Change
The class inheritance change from SandboxApiBase to SandboxBase (and subsequent use of cls._limits) unintentionally increased HTTP connection limits for API client operations in both sync and async sandbox API methods. The limits changed from max_keepalive_connections=10, max_connections=20, keepalive_expiry=20 to max_keepalive_connections=40, max_connections=40, keepalive_expiry=300. This results in significantly larger connection pool sizes and longer timeouts, potentially increasing resource usage.
Several improvements and refactors to the Python SDK's sandbox connection and configuration logic.
The main focus is on making header management more flexible, improving API parameter handling, and simplifying class structures for better maintainability and usability.
Also unifies doc strings