chore: Test all coroutine patterns and refactor op call #2684
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=b8732059b14b34de573c7b8deaa3940a1ca27cef |
| @@ -715,6 +715,8 @@ def finish_call( | |||
| *, | |||
| op: Optional[Op] = None, | |||
| ) -> None: | |||
| ended_at = datetime.datetime.now(tz=datetime.timezone.utc) | |||
| call.ended_at = ended_at | |||
There was a problem hiding this comment.
small bug i noticed - we don't update ended at!
weave/trace/op.py
Outdated
| res, _ = await do_call_async( | ||
| cast(Op, wrapper), *args, __should_raise=True, **kwargs | ||
| ) |
There was a problem hiding this comment.
am I correct in reading that you extracted this into do_call_async? Is anything else happening here?
There was a problem hiding this comment.
yes, no logical changes. But the key now is that def call can dispatch to each of these
weave/trace/op.py
Outdated
| res, _ = do_call( | ||
| cast(Op, wrapper), *args, __should_raise=True, **kwargs | ||
| ) |
weave/trace/op.py
Outdated
| ) | ||
|
|
||
|
|
||
| def do_call( |
There was a problem hiding this comment.
These are private, so should be underscored
weave/trace/op.py
Outdated
| return res, call | ||
|
|
||
|
|
||
| async def do_call_async( |
There was a problem hiding this comment.
These are private, so should be underscored
| @@ -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.
this was a small but in the test suite actually. This should have been testing method not amethod
tests/trace/test_op_coroutines.py
Outdated
| from weave.trace.weave_client import Call | ||
|
|
||
|
|
||
| def test_non_async_non_coro(client): |
There was a problem hiding this comment.
I would just call this test_sync
tests/trace/test_op_coroutines.py
Outdated
| assert res == 1 | ||
|
|
||
|
|
||
| def test_non_async_non_coro_method(client): |
tests/trace/test_op_coroutines.py
Outdated
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_non_async_coro(client): |
There was a problem hiding this comment.
test_sync_in_coroutine
There was a problem hiding this comment.
opposite, but yeah - changed
tests/trace/test_op_coroutines.py
Outdated
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_non_async_coro_method(client): |
There was a problem hiding this comment.
test_sync_method_in_coroutine
There was a problem hiding this comment.
changed everything to test_[Op_type]_[return_type]
| @pytest.mark.asyncio | ||
| async def test_async_coro(client): | ||
| @weave.op() | ||
| async def async_coro(): |
There was a problem hiding this comment.
when would you write code like this?
There was a problem hiding this comment.
not normally, but better to test
tests/trace/test_op_coroutines.py
Outdated
| assert res == 1 | ||
|
|
||
|
|
||
| def test_non_async_with_exception(client): |
There was a problem hiding this comment.
yeah basically anything that is marked "non_async" should just be called "sync"
tests/trace/test_op_coroutines.py
Outdated
| 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.
all None checks should be res is None
I am working on another feature and needed to use the
Op.callpattern. I realized that the typing for this was not exactly correct AND we did not use the same code as the wrapper logic.So, this PR:
call,async wrapperandwrapper