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(weave): Add attributes to Call object and allow explicit root #1739

Merged
merged 15 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from 11 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
1 change: 1 addition & 0 deletions weave/tests/test_client_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ def my_op(a: int) -> int:
exception=None,
output=6,
summary={},
attributes={},
)


Expand Down
3 changes: 3 additions & 0 deletions weave/tests/test_weave_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ def test_call_create(client):
exception=None,
summary={},
_children=[],
attributes={},
)
assert dataclasses.asdict(result._val) == dataclasses.asdict(expected)

Expand All @@ -228,6 +229,7 @@ def test_calls_query(client):
parent_id=None,
inputs={"a": 5, "b": 10},
id=call0.id,
attributes={},
)
assert result[1] == weave_client.Call(
op_name="weave:///shawn/test-project/op/x:tzUhDyzVm5bqQsuqh5RT4axEXSosyLIYZn9zbRyenaw",
Expand All @@ -236,6 +238,7 @@ def test_calls_query(client):
parent_id=call0.id,
inputs={"a": 6, "b": 11},
id=call1.id,
attributes={},
)
client.finish_call(call2, None)
client.finish_call(call1, None)
Expand Down
28 changes: 25 additions & 3 deletions weave/weave_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ class Call:
exception: Optional[str] = None
summary: Optional[dict] = None
display_name: Optional[str] = None
attributes: Optional[dict] = None
# These are the live children during logging
_children: list["Call"] = dataclasses.field(default_factory=list)

Expand Down Expand Up @@ -244,6 +245,7 @@ def make_client_call(
output=output,
summary=server_call.summary,
display_name=server_call.display_name,
attributes=server_call.attributes,
)
if call.id is None:
raise ValueError("Call ID is None")
Expand Down Expand Up @@ -457,9 +459,21 @@ def create_call(
op: Union[str, Op],
parent: Optional[Call],
inputs: dict,
attributes: dict = {},
attributes: Optional[dict] = None,
display_name: Optional[str] = None,
*,
_use_stack: bool = True,
) -> Call:
"""Create, log, and push a call onto the runtime stack.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're adding the docstring, I'm going to review it.

Style: Please use the style in api.py for publish. My little doc generator script expects that style and I made some decisions in choosing it. In particular, don't put the type next to the parameter. The type information is already annotated in the function signature, no need to reproduce it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed


Args:
op (Union[str, Op]): The operation to log.
Copy link
Contributor

Choose a reason for hiding this comment

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

The Op that this is a call for. If a string is passed, a code-less op will be created using "op" as the id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

parent (Optional[Call]): The parent call. If parent is not provided, the current run is used as the parent.
inputs (dict): The inputs to the operation.
display_name (Optional[str], optional): The display name for the call. Defaults to None.
attributes (Optional[dict], optional): The attributes for the call. Defaults to None.
_use_stack (bool, optional): Whether to push the call onto the runtime stack. Defaults to True.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why prefix with "_"? I think its just as obscure as "parent". I'd just call it "use_stack".

I think the docstring could cover both cases of stack use: If True, read parent from call_context, and push call onto call_context.

Should parent and use_stack be mutually exclusive? I mean should we throw an error if both are passed?

Ideally parent would have a default value of None and be moved to after inputs. This would mean we need to update positional callers, but that's probably not too bad right now since they're all in our codebase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed underscore. I like to use that when the param is not something we want to be used externally and therefore have more liberties to change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we don't want mutual exclusivity. We want to be able to specify a parent and to not use the stack (this is the exact case in Langchain integration)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved parent to 3rd

"""
if isinstance(op, str):
if op not in self._anonymous_ops:
self._anonymous_ops[op] = _build_anonymous_op(op)
Expand All @@ -474,7 +488,7 @@ def create_call(
inputs_with_refs = map_to_refs(inputs)
call_id = generate_id()

if parent is None:
if parent is None and _use_stack:
parent = run_context.get_current_run()

if parent:
Expand All @@ -483,6 +497,10 @@ def create_call(
else:
trace_id = generate_id()
parent_id = None

if attributes is None:
attributes = {}

call = Call(
op_name=op_str,
project_id=self._project_id(),
Expand All @@ -491,6 +509,7 @@ def create_call(
id=call_id,
inputs=inputs_with_refs,
display_name=display_name,
attributes=attributes,
)
if parent is not None:
parent._children.append(call)
Expand All @@ -510,7 +529,10 @@ def create_call(
wb_run_id=current_wb_run_id,
)
self.server.call_start(CallStartReq(start=start))
run_context.push_call(call)

if _use_stack:
run_context.push_call(call)

return call

@trace_sentry.global_trace_sentry.watch()
Expand Down
Loading