chore(weave): Add attributes to Call object and allow explicit root #1739
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=0ad4ca9cd57d7b2aa97586d274660196941b23ca |
…thub.com:wandb/weave into tim/add_support_for_explicit_root_and_attributes
weave/weave_client.py
Outdated
| op: Union[str, Op], | ||
| parent: Optional[Call], | ||
| inputs: dict, | ||
| attributes: dict = {}, |
There was a problem hiding this comment.
Should avoid using a mutable object as a default value
weave/weave_client.py
Outdated
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
moved parent to 3rd
| ) -> Call: | ||
| """Create, log, and push a call onto the runtime stack. |
There was a problem hiding this comment.
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.
weave/weave_client.py
Outdated
| """Create, log, and push a call onto the runtime stack. | ||
|
|
||
| Args: | ||
| op (Union[str, Op]): The operation to log. |
There was a problem hiding this comment.
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.
This PR supports the Langchain integration work and does two things: