core[patch]: Fix unit test that tests error callbacks on chat models #30228
Conversation
dosubot
bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
dosubot
bot
added
Ɑ: core
cbornet
force-pushed
the
fix-test_stream_error_callback
branch
from
ed7006b to
52d464a
Compare
| llm = FakeListChatModel( | ||
| responses=[message], | ||
| error_on_chunk_number=i, | ||
| ) | ||
| cb_async = FakeAsyncCallbackHandler() | ||
| llm_astream = llm.astream("Dummy message", config={"callbacks": [cb_async]}) |
There was a problem hiding this comment.
llm.astream("Dummy message", callbacks=[cb_async]) was incorrect.
Changed it to llm.astream("Dummy message", config={"callbacks": [cb_async]})
| cb_async = FakeAsyncCallbackHandler() | ||
| async for _ in llm.astream("Dummy message", callbacks=[cb_async]): | ||
| pass | ||
| eval_response(cb_async, i) |
There was a problem hiding this comment.
This code was actually not evaluated before because we were exiting the pytest.raises block when getting FakeListChatModelError
| @@ -93,34 +96,40 @@ async def test_async_batch_size() -> None: | |||
| assert (cb.traced_runs[0].extra or {}).get("batch_size") == 1 | |||
|
|
|||
|
|
|||
| async def test_stream_error_callback() -> None: | |||
| message = "test" | |||
| async def test_error_callback() -> None: | |||
There was a problem hiding this comment.
It's no use to test the error callback in streaming. The error is set in LLM.generate()
| @@ -83,9 +83,6 @@ class FakeStreamingListLLM(FakeListLLM): | |||
| chunks in a streaming implementation. | |||
| """ | |||
|
|
|||
| error_on_chunk_number: Optional[int] = None | |||
There was a problem hiding this comment.
I don't think we want to make this change -- it's breaking the API and users may be using the fake models now in unit tests
dosubot
bot
added
size:M
I rolledback the changes on FakeListLLM. PTAL. |
dosubot
bot
added
the
lgtm
eyurtsev
changed the title
Fixes #29436