-
Notifications
You must be signed in to change notification settings - Fork 84
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): Better organized op func methods #3898
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=2bf10c1d436ea1a43f1bb493ef26699e4e011775 |
WalkthroughThis pull request updates documentation and refactors accumulator handling throughout the codebase. The README is adjusted to use a neutral tone while a note is removed from the PII redaction guide. A new dependency is added in the project configuration. In tests and various integration modules, calls to the deprecated accumulator function are replaced with a new implementation. In addition, the core tracing module is overhauled by removing the old execution function and introducing a set of new helper functions and an iterator wrapper, with the obsolete accumulator extension file removed. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant Call as call()
participant Sync as _call_sync_func
participant Async as _call_async_func
participant Finish as finish/on_output
C->>Call: Invoke operation
alt Operation is synchronous
Call->>Sync: Delegate to _call_sync_func
Sync->>Finish: Process & complete operation
else Operation is asynchronous
Call->>Async: Delegate to _call_async_func
Async->>Finish: Process & complete operation
end
Finish->>C: Return result
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
weave/integrations/mistral/v0/mistral.py (1)
84-84
: Function call updated to use internal accumulator APIThe function call has been updated to use
_add_accumulator
from the core module. Note that while the type ignore comment is present, it would be better to properly type this function call to avoid potential type-checking issues.Consider adding proper type annotations instead of using
# type: ignore
to improve code maintainability:- acc_op = _add_accumulator(op, lambda inputs: mistral_accumulator) # type: ignore + acc_op = _add_accumulator(op, lambda inputs: mistral_accumulator)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
README.md
(1 hunks)docs/docs/guides/tracking/redact-pii.md
(0 hunks)pyproject.toml
(1 hunks)tests/trace/test_op_return_forms.py
(17 hunks)tests/trace/test_tracing_resilience.py
(11 hunks)weave/integrations/anthropic/anthropic_sdk.py
(4 hunks)weave/integrations/bedrock/bedrock_sdk.py
(2 hunks)weave/integrations/cohere/cohere_sdk.py
(3 hunks)weave/integrations/google_ai_studio/google_ai_studio_sdk.py
(3 hunks)weave/integrations/groq/groq_sdk.py
(2 hunks)weave/integrations/huggingface/huggingface_inference_client_sdk.py
(3 hunks)weave/integrations/instructor/instructor_iterable_utils.py
(3 hunks)weave/integrations/instructor/instructor_partial_utils.py
(2 hunks)weave/integrations/langchain_nvidia_ai_endpoints/langchain_nv_ai_endpoints.py
(2 hunks)weave/integrations/litellm/litellm.py
(2 hunks)weave/integrations/mistral/v0/mistral.py
(2 hunks)weave/integrations/mistral/v1/mistral.py
(2 hunks)weave/integrations/openai/openai_sdk.py
(3 hunks)weave/integrations/vertexai/vertexai_sdk.py
(3 hunks)weave/trace/op.py
(12 hunks)weave/trace/op_extensions/accumulator.py
(0 hunks)
💤 Files with no reviewable changes (2)
- docs/docs/guides/tracking/redact-pii.md
- weave/trace/op_extensions/accumulator.py
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.{md,mdx}`: Focus on technical accuracy. Check for broken links. Verify code examples are up-to-date. Look for clarity and completeness. Don't focus on grammar/spelling unless...
**/*.{md,mdx}
: Focus on technical accuracy.
Check for broken links.
Verify code examples are up-to-date.
Look for clarity and completeness.
Don't focus on grammar/spelling unless significant.
README.md
`**/*.py`: Focus on pythonic code patterns. Check for proper exception handling. Verify type hints usage where applicable. Look for potential performance improvements. Don't commen...
**/*.py
: Focus on pythonic code patterns.
Check for proper exception handling.
Verify type hints usage where applicable.
Look for potential performance improvements.
Don't comment on formatting if black/isort is configured.
Check for proper dependency injection patterns.
Verify proper async handling if applicable.
weave/integrations/instructor/instructor_partial_utils.py
weave/integrations/cohere/cohere_sdk.py
weave/integrations/openai/openai_sdk.py
weave/integrations/groq/groq_sdk.py
weave/integrations/instructor/instructor_iterable_utils.py
tests/trace/test_tracing_resilience.py
weave/integrations/litellm/litellm.py
weave/integrations/bedrock/bedrock_sdk.py
weave/integrations/langchain_nvidia_ai_endpoints/langchain_nv_ai_endpoints.py
tests/trace/test_op_return_forms.py
weave/integrations/huggingface/huggingface_inference_client_sdk.py
weave/integrations/anthropic/anthropic_sdk.py
weave/integrations/google_ai_studio/google_ai_studio_sdk.py
weave/integrations/mistral/v1/mistral.py
weave/trace/op.py
weave/integrations/mistral/v0/mistral.py
weave/integrations/vertexai/vertexai_sdk.py
🧬 Code Definitions (12)
weave/integrations/instructor/instructor_partial_utils.py (1)
weave/trace/op.py (4) (4)
op
(576:583)op
(587:593)op
(596:696)_add_accumulator
(1006:1062)
weave/integrations/cohere/cohere_sdk.py (1)
weave/trace/op.py (4) (4)
op
(576:583)op
(587:593)op
(596:696)_add_accumulator
(1006:1062)
weave/integrations/openai/openai_sdk.py (1)
weave/trace/op.py (6) (6)
op
(576:583)op
(587:593)op
(596:696)Op
(156:206)ProcessedInputs
(103:113)_add_accumulator
(1006:1062)
weave/integrations/groq/groq_sdk.py (1)
weave/trace/op.py (4) (4)
op
(576:583)op
(587:593)op
(596:696)_add_accumulator
(1006:1062)
weave/integrations/instructor/instructor_iterable_utils.py (1)
weave/trace/op.py (4) (4)
op
(576:583)op
(587:593)op
(596:696)_add_accumulator
(1006:1062)
weave/integrations/litellm/litellm.py (1)
weave/trace/op.py (4) (4)
op
(576:583)op
(587:593)op
(596:696)_add_accumulator
(1006:1062)
weave/integrations/langchain_nvidia_ai_endpoints/langchain_nv_ai_endpoints.py (1)
weave/trace/op.py (6) (6)
op
(576:583)op
(587:593)op
(596:696)Op
(156:206)ProcessedInputs
(103:113)_add_accumulator
(1006:1062)
weave/integrations/huggingface/huggingface_inference_client_sdk.py (1)
weave/trace/op.py (4) (4)
op
(576:583)op
(587:593)op
(596:696)_add_accumulator
(1006:1062)
weave/integrations/google_ai_studio/google_ai_studio_sdk.py (1)
weave/trace/op.py (4) (4)
op
(576:583)op
(587:593)op
(596:696)_add_accumulator
(1006:1062)
weave/trace/op.py (3)
weave/trace/settings.py (1) (1)
should_disable_weave
(173:174)weave/trace/context/weave_client_context.py (2) (2)
get_weave_client
(33:36)require_weave_client
(42:45)weave/trace/weave_client.py (4) (4)
call
(1054:1059)finish
(2002:2031)finish_call
(1212:1315)Call
(499:725)
weave/integrations/mistral/v0/mistral.py (2)
weave/trace/op.py (4) (4)
op
(576:583)op
(587:593)op
(596:696)_add_accumulator
(1006:1062)weave/integrations/mistral/v1/mistral.py (1) (1)
mistral_accumulator
(20:84)
weave/integrations/vertexai/vertexai_sdk.py (1)
weave/trace/op.py (4) (4)
op
(576:583)op
(587:593)op
(596:696)_add_accumulator
(1006:1062)
⏰ Context from checks skipped due to timeout of 90000ms (93)
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
🔇 Additional comments (53)
pyproject.toml (1)
77-80
: LGTM: Addition of google-genai dependency for instructor integration.The addition of
google-genai>=1.5.0
as a dependency for the instructor optional features makes sense, especially considering the broader refactoring of accumulator functionality across the codebase. This ensures that Google AI integrations will work properly with the instructor features.README.md (1)
9-9
: LGTM: Tone adjustment in project description.Changed from an exclamation mark to a period, resulting in a more neutral and professional tone. This doesn't affect the technical content or accuracy of the documentation.
weave/integrations/bedrock/bedrock_sdk.py (2)
4-5
: LGTM: Updated import to use internal accumulator function.Import now references
_add_accumulator
directly fromweave.trace.op
instead of from the extension module, aligning with the codebase refactoring effort to better organize operational function methods.
136-141
: LGTM: Updated function call to use the new internal accumulator function.The function call has been updated to use
_add_accumulator
instead ofadd_accumulator
while keeping the same parameters and functionality, ensuring consistent behavior.weave/integrations/instructor/instructor_partial_utils.py (2)
7-7
: LGTM: Updated import to use internal accumulator function.Changed import to reference
_add_accumulator
directly fromweave.trace.op
instead of from the extension module, consistent with the PR's goal of better organizing operational function methods.
22-26
: LGTM: Updated function call to use the new internal accumulator function.The function call has been updated to use
_add_accumulator
while maintaining the same parameters and behavior. This change is part of the refactoring effort to centralize accumulator functionality in theweave.trace.op
module.weave/integrations/litellm/litellm.py (2)
9-9
: Import updated to use internal accumulator APIThe import has been updated to use
_add_accumulator
fromweave.trace.op
instead of the previous accumulator import. This aligns with the function's new location in the codebase.
94-99
: Updated to use internal accumulator APIThe function call has been updated to use
_add_accumulator
instead ofadd_accumulator
, maintaining the same parameters and functionality. This properly implements the refactoring of the accumulator functionality.weave/integrations/instructor/instructor_iterable_utils.py (3)
8-8
: Import updated to use internal accumulator APIThe import has been updated to use
_add_accumulator
fromweave.trace.op
instead of the previous accumulator extension import. This change is part of the refactoring to better organize op function methods.
35-39
: Updated to use internal accumulator API in sync wrapperThe function call has been updated to use
_add_accumulator
instead ofadd_accumulator
in theinstructor_wrapper_sync
function, maintaining the same parameters and functionality.
55-59
: Updated to use internal accumulator API in async wrapperThe function call has been updated to use
_add_accumulator
instead ofadd_accumulator
in theinstructor_wrapper_async
function, maintaining the same parameters and functionality.weave/integrations/cohere/cohere_sdk.py (3)
10-10
: Import updated to use internal accumulator APIThe import has been updated to use
_add_accumulator
fromweave.trace.op
instead of the previous accumulator extension import, aligning with the organizational changes in the codebase.
170-170
: Updated to use internal accumulator API in stream wrapperThe function call has been updated to use
_add_accumulator
instead ofadd_accumulator
in thecohere_stream_wrapper
function, maintaining the same parameters and functionality.
179-179
: Updated to use internal accumulator API in v2 stream wrapperThe function call has been updated to use
_add_accumulator
instead ofadd_accumulator
in thecohere_stream_wrapper_v2
function, maintaining the same parameters and functionality.tests/trace/test_tracing_resilience.py (2)
17-17
: Import updated to use internal accumulator APIThe import has been updated to use
_add_accumulator
fromweave.trace.op
instead of the previous accumulator extension import. This aligns with the function's new location in the core module.
146-146
: Tests updated to use internal accumulator APIAll test calls to
add_accumulator
have been properly replaced with_add_accumulator
while maintaining the same functionality and test coverage. The tests still verify the resilience of the accumulator functionality under various error conditions.Also applies to: 181-181, 214-214, 254-254, 293-297, 338-342, 385-389, 431-435, 470-470, 500-500
weave/integrations/openai/openai_sdk.py (3)
10-10
: Updated import to use the new internal accumulator function.The import statement now uses
_add_accumulator
fromweave.trace.op
instead of the previousadd_accumulator
fromweave.trace.op_extensions.accumulator
, reflecting the refactoring of the accumulator functionality into the coreop
module.
334-341
: Updated to use internal _add_accumulator function.This change replaces the previous
add_accumulator
with_add_accumulator
while maintaining the same functionality. The leading underscore in the function name indicates this is now intended for internal use only.
370-377
: Updated to use internal _add_accumulator function for async wrapper.Similar to the sync wrapper, this function now uses
_add_accumulator
instead ofadd_accumulator
with the same parameter structure, ensuring consistent behavior across sync and async implementations.weave/integrations/vertexai/vertexai_sdk.py (3)
10-10
: Import updated to use internal accumulator function.The import statement now correctly pulls
_add_accumulator
fromweave.trace.op
module rather than the previous location, aligning with the internal API changes.
98-103
: Updated sync wrapper to use _add_accumulator.The function call to
add_accumulator
has been replaced with_add_accumulator
while keeping the same functionality. This change is part of a broader refactoring to standardize accumulator implementation across the codebase.
123-128
: Updated async wrapper to use _add_accumulator.Consistent with the changes to the sync wrapper, the async implementation now uses
_add_accumulator
with identical parameters, ensuring consistent behavior between both implementations.weave/integrations/groq/groq_sdk.py (2)
9-9
: Import updated to use internal accumulator function.The import statement now uses
_add_accumulator
fromweave.trace.op
instead of the previous location, aligning with the refactored internal API.
96-100
: Updated to use internal _add_accumulator function.The groq wrapper now uses the refactored
_add_accumulator
function while maintaining the same parameter pattern, ensuring consistent behavior with the previous implementation.weave/integrations/google_ai_studio/google_ai_studio_sdk.py (3)
10-10
: Import updated to use internal accumulator function.The import statement now references
_add_accumulator
fromweave.trace.op
instead of the previous import, aligning with the refactored implementation.
103-108
: Updated sync wrapper to use _add_accumulator.The gemini sync wrapper now correctly uses the refactored
_add_accumulator
function while maintaining the same parameters and behavior, ensuring consistency with the broader API changes.
128-133
: Updated async wrapper to use _add_accumulator.The async implementation has been properly updated to use
_add_accumulator
with identical parameters as the sync version, maintaining consistent behavior between both implementations.weave/integrations/langchain_nvidia_ai_endpoints/langchain_nv_ai_endpoints.py (2)
17-17
: Import changed from extension to core moduleThe import has been updated to use
_add_accumulator
from the coreweave.trace.op
module instead of the previous import from an extensions module. This aligns with internal refactoring to consolidate accumulator functionality.
164-169
: Function call updated to use internal accumulator APIThe function call has been updated to use
_add_accumulator
, which is an internal API designed specifically for integrations with streaming libraries. The parameters remain unchanged, maintaining the same functionality.weave/integrations/mistral/v0/mistral.py (1)
9-9
: Import changed from extension to core moduleThe import has been updated to use
_add_accumulator
from the coreweave.trace.op
module instead of importing from an extensions module. This aligns with the code organization changes across the project.weave/integrations/huggingface/huggingface_inference_client_sdk.py (3)
8-8
: Import changed from extension to core moduleThe import has been updated to use
_add_accumulator
from the coreweave.trace.op
module instead of importing from an extensions module. This is part of the codebase reorganization to better structure the op function methods.
82-87
: Updated sync wrapper to use internal accumulator APIThe function call in the sync wrapper has been updated to use
_add_accumulator
with the same parameters, preserving the original behavior while using the new internal API.
106-111
: Updated async wrapper to use internal accumulator APIThe function call in the async wrapper has been updated to use
_add_accumulator
with the same parameters, maintaining consistency between sync and async implementations.weave/integrations/anthropic/anthropic_sdk.py (4)
11-11
: Import updated to include both accumulator and iterator wrapperThe import has been updated to use
_add_accumulator
and_IteratorWrapper
from the coreweave.trace.op
module. This change aligns with the refactoring across the codebase to consolidate related functionality.
80-84
: Updated sync wrapper to use internal accumulator APIThe function call in the sync wrapper has been updated to use
_add_accumulator
while maintaining the same parameters and behavior.
104-108
: Updated async wrapper to use internal accumulator APIThe function call in the async wrapper has been updated to use
_add_accumulator
with the same parameters, maintaining consistent behavior between sync and async implementations.
173-178
: Updated stream wrapper to use internal accumulator API with custom iteratorThe function call has been updated to use
_add_accumulator
while also specifying a customAnthropicIteratorWrapper
. This maintains the specialized stream handling functionality while adapting to the new API.weave/integrations/mistral/v1/mistral.py (2)
9-9
: Use of the private_add_accumulator
import looks intentional.
No immediate issues. If this function is stable enough for public usage, consider renaming it to remove the underscore.
91-91
: Accumulator function usage appears correct.
The updated code properly referencesmistral_accumulator
. Confirm that all necessary tests or calls now rely on_add_accumulator
rather than the removedadd_accumulator
.tests/trace/test_op_return_forms.py (2)
4-4
: Updated import reference is consistent with the refactor.
No concerns here; the new import should integrate correctly with_add_accumulator
.
113-113
: Uniform replacement ofadd_accumulator
with_add_accumulator
.
All these lines exhibit the same refactor pattern. Ensure the accumulator logic remains unchanged.Also applies to: 140-140, 175-175, 211-211, 237-237, 263-263, 297-297, 313-313, 337-337, 385-385, 421-421, 458-458, 487-487, 521-521, 561-561, 603-603
weave/trace/op.py (12)
5-5
: New imports (atexit
,weakref
, and extended collections) are valid.
They align with the new functionality managing iterator state and references.Also applies to: 11-12
20-20
: Additional type parameters (Generic
,TypeVar
)
They improve code clarity and type safety. No issues found.Also applies to: 24-24
71-85
: Backport-like definitions foraiter
andanext
.
These ensure compatibility with Python <3.10. The approach is standard and looks correct.
86-86
: Logger instantiation.
Initializinglogger
at module scope is acceptable for capturing debug info. No further concerns.
299-309
:_should_skip_tracing
solid use-case checks.
This function logically covers all “skip tracing” conditions: disabled setting, missing client, or manually disabled op.
311-319
:_should_sample_traces
handles parent-child calls and sampling.
This correctly bypasses child calls for sampling. The random check for sampling rate appears correct.
334-420
:_call_sync_func
refactor
- Skips tracing early when needed.
- Captures output and exceptions with inline
finish
.- Cleanly pops call context.
Logic is sound and well-structured.
422-506
:_call_async_func
mirrors_call_sync_func
for async.
Flow is nearly identical, with error/finish handling in line. Appears consistent and correct.
508-547
:call
function orchestration.
Dispatching to sync or async call paths is straightforward and captures exceptions in theCall
object.
774-782
: New callback type aliases.
Facilitate typed hooks for iterators. Straightforward definitions with descriptive naming.
784-920
:_IteratorWrapper
class for hooking into iteration lifecycle.
- Good handling of
__enter__
,__exit__
, async contexts, and final close.- Defensive error capturing in
_call_on_close_once
and_call_on_error_once
.- Potential concurrency note: current approach is single-threaded, which is fine unless concurrency is introduced.
961-1063
: Accumulator utility and_add_accumulator
refactor.
- Accumulator state management fits streaming scenarios.
- Code is clear and well encapsulated.
- Provides an extensible pattern for custom accumulations.
c874c9b
to
693d805
Compare
3e3481a
to
22918dc
Compare
re-opening: #3878
Summary by CodeRabbit