feat(weave): Add 'tracing_sample_rate' param to weave.op #3195
Conversation
tests/trace/test_client_trace.py
Outdated
| @@ -269,7 +269,7 @@ def adder(a: Number) -> Number: | |||
|
|
|||
| adder_v0 = adder | |||
|
|
|||
| @weave.op() | |||
| @weave.op() # type: ignore | |||
weave/trace/op.py
Outdated
| skip_tracing = ( | ||
| settings.should_disable_weave() | ||
| or weave_client_context.get_weave_client() is None | ||
| or not op._tracing_enabled | ||
| or not get_tracing_enabled() | ||
| ) | ||
|
|
||
| if skip_tracing: |
There was a problem hiding this comment.
I try to avoid this. The former is easier to debug which branch you're in if you're unexpectedly skipping tracing
|
We should update the docs for this feature too |
… github.com:wandb/weave into adrian/trace_sampling
| def adder(a: Number) -> Number: | ||
| return Number(value=a.value + a.value) | ||
|
|
||
| adder_v0 = adder | ||
|
|
||
| @weave.op() | ||
| @weave.op # type: ignore |
There was a problem hiding this comment.
why is this the only one with type ignore?
There was a problem hiding this comment.
linter throws a warning about redefining "adder" without calling it an overload (it's already defined above)
tests/trace/test_client_trace.py
Outdated
| for i in range(10): | ||
| never_traced(i) | ||
| assert never_traced_calls == 10 # Function was called | ||
| # NOTE: We can't assert here that never_traced.calls() is empty because that call requires |
There was a problem hiding this comment.
you could just publish the op
There was a problem hiding this comment.
you mean just in this test?
There was a problem hiding this comment.
well it would be nice to have a consistent assertion for # of calls. so anywhere you did not to .calls could have explicit publishing
| return res, call | ||
|
|
||
| current_call = call_context.get_current_call() | ||
| if current_call is None: |
There was a problem hiding this comment.
might be nice to say: if your parent is traced, then you are (as a comment)
There was a problem hiding this comment.
Note: this specific case is not tested by your unit tests (setting a child sample rate to 0, and a parent to 1)
Done |
Description
Introduces a new parameter, "tracing_sample_rate", to weave.op. This allows users to specify how frequently the op should actually be traced.
The sample rate applies to the traced op as well as all of its descendants. If a root op is 'sampled out', then none of its descendant calls in that 'tree' will be traced either, regardless of the sampling rate set in those descendant ops.
Testing
Tests were added.