-
Notifications
You must be signed in to change notification settings - Fork 67
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
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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): | ||
@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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed everything to |
||
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(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when would you write code like this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
|
||
@pytest.mark.asyncio | ||
|
There was a problem hiding this comment.
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