chore(weave): backend to allow call renaming #1699
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=5e758df8d6b81e65c728870b56220c68870e6fe0 |
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=64ba97e6fb6802bfe1ab6165220f5cce6f64e5be |
| project_id: str | ||
| call_id: str | ||
| # wb_user_id gets generated from auth params | ||
| wb_user_id: typing.Optional[str] = None |
There was a problem hiding this comment.
When is this None? If it is only expected to be None over the wire, then you should consider a specific Req class inside the server definition itself.
There was a problem hiding this comment.
ah okay, this is the same as Delete so I can do both that way...
There was a problem hiding this comment.
I realize this is a bit lame but im not 100% sure what you mean, or where the best place for this type to live...
There was a problem hiding this comment.
ok let's chat... basically:
We should have class CallRenameReq(BaseModel): here require the wb_user_id and the trace server itself should have a class with such field omitted
There was a problem hiding this comment.
honestly, it is a small nit, so maybe not worth it. In Jamie's https://github.com/wandb/weave/pull/1718/files#diff-2628396670e408d29134ac19cc8f01d7d12e9e60821a514e02b0139a03e05d8c, the id is required actually
There was a problem hiding this comment.
Right, but doesn't call_rename have to be identical in:
remote_http_trace_server.pyclickhouse_trace_server_batched.pyTraceServerInterfaceintrace_server_interface.py?
Over the wire we don't need wb_run_id, but in the other places we do. @tssweeney
weave/weave_client.py
Outdated
| @@ -561,6 +572,16 @@ def delete_call(self, call: Call) -> None: | |||
| ) | |||
| ) | |||
|
|
|||
| @trace_sentry.global_trace_sentry.watch() | |||
| def rename_call(self, call: Call, display_name: str) -> None: | |||
There was a problem hiding this comment.
Seeing this pattern makes me wonder, "should all operations follow this pattern?:"
- LOWEST LEVEL:
trace_server_interfacehas capability directly or indirectly via other capabilities remote_http_trace_server-> exposes http interface in language specific binding (could be generated someday)weave_client: contains some[action]_[class](self, obj: Class, params)to do the action operating on objects- HIGHEST LEVEL
[Class].action(params)- OO style of action
Maybe so? I like that clarity in design.
gtarpenning
force-pushed
the
griffin/rename-traces
branch
from
7ccf89c
to
a41d3c7
Compare
requires using
argMax, which is not asimpleAggregationFunction.Depends on: https://github.com/wandb/core/pull/21850