core: Use Blockbuster to detect blocking calls in asyncio during tests #29043
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
dosubot
bot
added
the
size:L
cbornet
force-pushed
the
blockbuster
branch
2 times, most recently
from
0983c3d to
7ce4b18
Compare
|
|
||
| with tempfile.NamedTemporaryFile(delete=True, suffix=".jpg") as temp_file: |
There was a problem hiding this comment.
It's useless to use a real file here.
| """Test invoking nested runnable lambda.""" | ||
| blockbuster.deactivate() |
There was a problem hiding this comment.
The code makes a sync call from async on purpose...
There was a problem hiding this comment.
Looks good overall. Should we enable/disable for unit tests?
I like that it forces us to separate async tests from sync tests and avoid being lazy, but at the same time there's some changes that aren't technically necessary and seem to unnecessarily complicate the testing code (e.g., updating a file open to a non blocking request)?
| @@ -121,7 +121,7 @@ def _config_with_context( | |||
| return patch_config(config, configurable=context_funcs) | |||
|
|
|||
|
|
|||
| def aconfig_with_context( | |||
| async def aconfig_with_context( | |||
There was a problem hiding this comment.
technically breaking change, but probably okay looks a lot like a private function to me.
Would you mind adding a comment about why this needs to be async (i.e., is inspect.get_source is making os calls?)
| @@ -90,9 +91,11 @@ async def test_inmemory_dump_load(tmp_path: Path) -> None: | |||
| output = await store.asimilarity_search("foo", k=1) | |||
|
|
|||
| test_file = str(tmp_path / "test.json") | |||
| store.dump(test_file) | |||
| await asyncio.to_thread(store.dump, test_file) | |||
There was a problem hiding this comment.
Feels like a false positive for the test? it doesn't really matter whether this is using blocking or non blocking code here?
There was a problem hiding this comment.
Well, with the way blockbuster is configured here (activated before the test, deativated after), the test code itself needs to be non-blocking.
Maybe we could add aload/adump methods to InMemoryVectorStore using aiofile/aiofiles?
| @@ -298,17 +299,17 @@ def parent(a: int) -> int: | |||
| # Now run the chain and check the resulting posts | |||
| cb = [tracer] | |||
| if method == "invoke": | |||
| res: Any = parent.invoke(1, {"callbacks": cb}) # type: ignore | |||
| res: Any = await asyncio.to_thread(parent.invoke, 1, {"callbacks": cb}) # type: ignore | |||
There was a problem hiding this comment.
Is there any benefit to doing this? feel like it's complicating the test code?
There was a problem hiding this comment.
I refactored this part to cleanly separate sync and async tests: see c2162c2
With this the asyncio.to_thread calls are not needed.
dosubot
bot
added
size:XL
cbornet
force-pushed
the
blockbuster
branch
2 times, most recently
from
427bc71 to
2498ab6
Compare
asyncio.Event is not thread-safe so it must be created in the asyncio thread
This PR uses the blockbuster library in langchain-core to detect blocking calls made in the asyncio event loop during unit tests.
Avoiding blocking calls is hard as these can be deeply buried in the code or made in 3rd party libraries.
Blockbuster makes it easier to detect them by raising an exception when a call is made to a known blocking function (eg:
time.sleep).Adding blockbuster allowed to find a blocking call in
aconfig_with_context(it ends up callingget_function_nonlocalswhich loads function code).Dependencies:
Twitter handle: cbornet_