Session bug (fix with context variables) #161
Conversation
log10/load.py
Outdated
| except Exception as e: | ||
| logging.warn(f"LOG10: failed to log: {e}. Skipping") | ||
| # Print the exception tracompletion_urlceback |
log10/load.py
Outdated
| if last_completion_response_var.get() is None: | ||
| return None | ||
| response = last_completion_response_var.get() | ||
| return url + "/app/" + response["organizationSlug"] + "/completions/" + response["completionID"] |
There was a problem hiding this comment.
Maybe write as fstring eg
f'{url}/app/{response["organizationSlug"]}/completions/{response["completionId"]}'
| # | ||
| # Context variables | ||
| # | ||
| session_id_var = contextvars.ContextVar("session_id", default=get_session_id()) |
There was a problem hiding this comment.
Slight preference to not set a default here and force an explicit set.
There was a problem hiding this comment.
We need this to keep the existing behavior - where llm calls in outer most "context" i.e. no context, still gets grouped together
| import uuid | ||
|
|
||
|
|
||
| session_id_var = contextvars.ContextVar("session_id", default=str(uuid.uuid4())) |
There was a problem hiding this comment.
Same here - slight preference to not set a default. Makes it more explicit and simpler to test.
There was a problem hiding this comment.
This was just for testing, if that's ok to keep
| session_id_var.reset(self.token) | ||
|
|
||
|
|
||
| def simulated_llm_call(): |
There was a problem hiding this comment.
If you removed the default value - I'd expect to see an explicit set here (or in an outer scope) before get is possible.
There was a problem hiding this comment.
Note sure I follow. Is it to set uuids explicitly by having it as an input to the session object?
| async def test_nested_async_contexts(): | ||
| print("") | ||
| before_outer_session = await simulated_llm_acall() | ||
| async with ExampleSession(): | ||
| before_inner_session = await simulated_llm_acall() | ||
|
|
||
| async with ExampleSession(): | ||
| inner_session = await simulated_llm_acall() | ||
|
|
||
| assert inner_session != before_inner_session | ||
| assert inner_session != before_outer_session | ||
|
|
||
| assert await simulated_llm_acall() == before_inner_session | ||
|
|
||
| assert await simulated_llm_acall() == before_outer_session | ||
|
|
||
|
|
||
| asyncio.run(test_nested_async_contexts()) |
There was a problem hiding this comment.
adding the @pytest.mark.asyncio before the def func, then we don't need to call asyncio.run here. We have setup that way in other tests (e.g openai tests)
| await asyncio.gather(run_session(delay=0, run_time=3), run_session(delay=1, run_time=1)) | ||
|
|
||
|
|
||
| asyncio.run(test_overlapping_async_contexts()) |
No description provided.