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

Fix unpickleable fields due to openai client #79

Merged
merged 5 commits into from
Apr 2, 2024

Conversation

matt-bernstein
Copy link
Contributor

Bugfix - /submit requests to the server were failing with the following stack trace:

app-1   | ERROR:  Exception in ASGI application
app-1   | Traceback (most recent call last):
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/anyio/streams/memory.py", line 98, in receive
app-1   |   return self.receive_nowait()
app-1   |       ^^^^^^^^^^^^^^^^^^^^^
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/anyio/streams/memory.py", line 93, in receive_nowait
app-1   |   raise WouldBlock
app-1   | anyio.WouldBlock
app-1   | 
app-1   | During handling of the above exception, another exception occurred:
app-1   | 
app-1   | Traceback (most recent call last):
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/middleware/base.py", line 78, in call_next
app-1   |   message = await recv_stream.receive()
app-1   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/anyio/streams/memory.py", line 118, in receive
app-1   |   raise EndOfStream
app-1   | anyio.EndOfStream
app-1   | 
app-1   | During handling of the above exception, another exception occurred:
app-1   | 
app-1   | Traceback (most recent call last):
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/uvicorn/protocols/http/httptools_impl.py", line 419, in run_asgi
app-1   |   result = await app( # type: ignore[func-returns-value]
app-1   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/uvicorn/middleware/proxy_headers.py", line 84, in __call__
app-1   |   return await self.app(scope, receive, send)
app-1   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/fastapi/applications.py", line 1106, in __call__
app-1   |   await super().__call__(scope, receive, send)
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/applications.py", line 122, in __call__
app-1   |   await self.middleware_stack(scope, receive, send)
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/middleware/errors.py", line 184, in __call__
app-1   |   raise exc
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/middleware/errors.py", line 162, in __call__
app-1   |   await self.app(scope, receive, _send)
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/middleware/base.py", line 108, in __call__
app-1   |   response = await self.dispatch_func(request, call_next)
app-1   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app-1   |  File "/usr/src/app/server/log_middleware.py", line 33, in dispatch
app-1   |   response = await call_next(request)
app-1   |         ^^^^^^^^^^^^^^^^^^^^^^^^
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/middleware/base.py", line 84, in call_next
app-1   |   raise app_exc
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/middleware/base.py", line 70, in coro
app-1   |   await self.app(scope, receive_or_disconnect, send_no_error)
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/middleware/cors.py", line 91, in __call__
app-1   |   await self.simple_response(scope, receive, send, request_headers=headers)
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/middleware/cors.py", line 146, in simple_response
app-1   |   await self.app(scope, receive, send)
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/middleware/exceptions.py", line 79, in __call__
app-1   |   raise exc
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/middleware/exceptions.py", line 68, in __call__
app-1   |   await self.app(scope, receive, sender)
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/fastapi/middleware/asyncexitstack.py", line 20, in __call__
app-1   |   raise e
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/fastapi/middleware/asyncexitstack.py", line 17, in __call__
app-1   |   await self.app(scope, receive, send)
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/routing.py", line 718, in __call__
app-1   |   await route.handle(scope, receive, send)
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/routing.py", line 276, in handle
app-1   |   await self.app(scope, receive, send)
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/starlette/routing.py", line 66, in app
app-1   |   response = await func(request)
app-1   |         ^^^^^^^^^^^^^^^^^^^
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/fastapi/routing.py", line 274, in app
app-1   |   raw_response = await run_endpoint_function(
app-1   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app-1   |  File "/usr/src/app/.venv/lib/python3.11/site-packages/fastapi/routing.py", line 191, in run_endpoint_function
app-1   |   return await dependant.call(**values)
app-1   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app-1   |  File "/usr/src/app/server/app.py", line 109, in submit
app-1   |   serialized_agent = pickle.dumps(request.agent)
app-1   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
app-1   | TypeError: cannot pickle '_thread.RLock' object

The root cause is that the OpenAI client object in AsyncOpenAIChatRuntime and OpenAIChatRuntime was attempted to be serialized using pickle.dumps(), which is not supported after upgrading the openai client lib: openai/openai-python#837

The solution was to make it a computed field in the sync client, and remove it from the async client as it is unused.

@robot-ci-heartex
Copy link
Collaborator

@robot-ci-heartex
Copy link
Collaborator

openai_model: str = Field(alias="model")
openai_api_key: Optional[str] = Field(
default=os.getenv("OPENAI_API_KEY"), alias="api_key"
)
max_tokens: Optional[int] = 1000
splitter: Optional[str] = None

_client: OpenAI = None
@computed_field
def _client(self) -> OpenAI:
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need computed_field here but create a client on the fly in the Async version? let's use the unique approach if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the sync runtime actually uses its _client, but the async runtime does not, it directly sends post requests in async_create_completion. Even if it did use the python client lib instead of requests, it wouldn't make sense for the class to carry a single client instance because it would create them on demand to use them concurrently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@niklub if you feel strongly about this I'll change it, but I think it's a meaningful difference that the async openai runtime doesn't need a self._client

@matt-bernstein matt-bernstein force-pushed the fix/unpickleable-fields branch from 35627d7 to 013c75a Compare April 2, 2024 15:28
@robot-ci-heartex
Copy link
Collaborator

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 53.25%. Comparing base (740e0d6) to head (013c75a).
Report is 2 commits behind head on master.

Files Patch % Lines
adala/runtimes/_openai.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   53.22%   53.25%   +0.03%     
==========================================
  Files          42       42              
  Lines        1704     1703       -1     
==========================================
  Hits          907      907              
+ Misses        797      796       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@robot-ci-heartex
Copy link
Collaborator

@matt-bernstein matt-bernstein merged commit d668350 into master Apr 2, 2024
15 checks passed
@matt-bernstein matt-bernstein deleted the fix/unpickleable-fields branch April 2, 2024 18:19
@robot-ci-heartex
Copy link
Collaborator

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.

4 participants