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

chore: Test all coroutine patterns and refactor op call #2684

Merged
merged 6 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
220 changes: 220 additions & 0 deletions tests/trace/test_op_coroutines.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,220 @@
import asyncio
from typing import Coroutine

import pytest

import weave
from weave.trace.weave_client import Call


def test_non_async_non_coro(client):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just call this test_sync

@weave.op()
def non_async_non_coro():
return 1

res = non_async_non_coro()
assert res == 1
res, call = non_async_non_coro.call()
assert isinstance(call, Call)
assert res == 1


def test_non_async_non_coro_method(client):
Copy link
Collaborator

Choose a reason for hiding this comment

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

test_sync_method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

class TestClass:
@weave.op()
def non_async_non_coro(self):
return 1

test_inst = TestClass()
res = test_inst.non_async_non_coro()
assert res == 1
res, call = test_inst.non_async_non_coro.call(test_inst)
assert isinstance(call, Call)
assert res == 1


@pytest.mark.asyncio
async def test_non_async_coro(client):
Copy link
Collaborator

Choose a reason for hiding this comment

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

test_sync_in_coroutine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opposite, but yeah - changed

@weave.op()
def non_async_coro():
return asyncio.to_thread(lambda: 1)

res = non_async_coro()
assert isinstance(res, Coroutine)
assert await res == 1
res, call = non_async_coro.call()
assert isinstance(call, Call)
assert isinstance(res, Coroutine)
assert await res == 1


@pytest.mark.asyncio
async def test_non_async_coro_method(client):
Copy link
Collaborator

Choose a reason for hiding this comment

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

test_sync_method_in_coroutine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed everything to test_[Op_type]_[return_type]

class TestClass:
@weave.op()
def non_async_coro(self):
return asyncio.to_thread(lambda: 1)

test_inst = TestClass()
res = test_inst.non_async_coro()
assert isinstance(res, Coroutine)
assert await res == 1
res, call = test_inst.non_async_coro.call(test_inst)
assert isinstance(call, Call)
assert isinstance(res, Coroutine)
assert await res == 1


@pytest.mark.asyncio
async def test_async_coro(client):
@weave.op()
async def async_coro():
Copy link
Collaborator

Choose a reason for hiding this comment

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

when would you write code like this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not normally, but better to test

return asyncio.to_thread(lambda: 1)

res = async_coro()
assert isinstance(res, Coroutine)
res2 = await res
assert isinstance(res2, Coroutine)
assert await res2 == 1
res, call = await async_coro.call()
assert isinstance(call, Call)
assert isinstance(res, Coroutine)
assert await res == 1


@pytest.mark.asyncio
async def test_async_coro_method(client):
class TestClass:
@weave.op()
async def async_coro(self):
return asyncio.to_thread(lambda: 1)

test_inst = TestClass()

res = test_inst.async_coro()
assert isinstance(res, Coroutine)
res2 = await res
assert isinstance(res2, Coroutine)
assert await res2 == 1
res, call = await test_inst.async_coro.call(test_inst)
assert isinstance(call, Call)
assert isinstance(res, Coroutine)
assert await res == 1


@pytest.mark.asyncio
async def test_async_awaited_coro(client):
@weave.op()
async def async_awaited_coro():
return await asyncio.to_thread(lambda: 1)

res = async_awaited_coro()
assert isinstance(res, Coroutine)
assert await res == 1
res, call = await async_awaited_coro.call()
assert isinstance(call, Call)
assert res == 1


@pytest.mark.asyncio
async def test_async_awaited_coro_method(client):
class TestClass:
@weave.op()
async def async_awaited_coro(self):
return await asyncio.to_thread(lambda: 1)

test_inst = TestClass()
res = test_inst.async_awaited_coro()
assert isinstance(res, Coroutine)
assert await res == 1
res, call = await test_inst.async_awaited_coro.call(test_inst)
assert isinstance(call, Call)
assert res == 1


@pytest.mark.asyncio
async def test_async_non_coro(client):
@weave.op()
async def async_non_coro():
return 1

res = async_non_coro()
assert isinstance(res, Coroutine)
assert await res == 1
res, call = await async_non_coro.call()
assert isinstance(call, Call)
assert res == 1


@pytest.mark.asyncio
async def test_async_non_coro_method(client):
class TestClass:
@weave.op()
async def async_non_coro(self):
return 1

test_inst = TestClass()
res = test_inst.async_non_coro()
assert isinstance(res, Coroutine)
assert await res == 1
res, call = await test_inst.async_non_coro.call(test_inst)
assert isinstance(call, Call)
assert res == 1


def test_non_async_with_exception(client):
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah basically anything that is marked "non_async" should just be called "sync"

@weave.op()
def non_async_with_exception():
raise ValueError("test")

with pytest.raises(ValueError, match="test"):
non_async_with_exception()
res, call = non_async_with_exception.call()
assert isinstance(call, Call)
assert call.exception is not None
assert res == None


def test_non_async_with_exception_method(client):
class TestClass:
@weave.op()
def non_async_with_exception(self):
raise ValueError("test")

test_inst = TestClass()
with pytest.raises(ValueError, match="test"):
test_inst.non_async_with_exception()
res, call = test_inst.non_async_with_exception.call(test_inst)
assert isinstance(call, Call)
assert call.exception is not None
assert res == None
Copy link
Collaborator

Choose a reason for hiding this comment

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

all None checks should be res is None

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, changed



@pytest.mark.asyncio
async def test_async_with_exception(client):
@weave.op()
async def async_with_exception():
raise ValueError("test")

with pytest.raises(ValueError, match="test"):
await async_with_exception()
res, call = await async_with_exception.call()
assert isinstance(call, Call)
assert call.exception is not None
assert res == None


@pytest.mark.asyncio
async def test_async_with_exception_method(client):
class TestClass:
@weave.op()
async def async_with_exception(self):
raise ValueError("test")

test_inst = TestClass()
with pytest.raises(ValueError, match="test"):
await test_inst.async_with_exception()
res, call = await test_inst.async_with_exception.call(test_inst)
assert isinstance(call, Call)
assert call.exception is not None
assert res == None
2 changes: 1 addition & 1 deletion tests/trace/test_op_decorator_behaviour.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ def test_sync_method_call(client, weave_obj, py_obj):
weave_obj_method2 = weave_obj_method_ref.get()

with pytest.raises(errors.OpCallError):
res2, call2 = py_obj.amethod.call(1)
res2, call2 = py_obj.method.call(1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was a small but in the test suite actually. This should have been testing method not amethod



@pytest.mark.asyncio
Expand Down
Loading
Loading