fix(sdk): improve type safety in decorators #2867
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 9a80aa1 in 2 minutes and 47 seconds. Click for details.
- Reviewed
312lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:34
- Draft comment:
These type aliases are already defined in init.py. They should be imported rather than redefined. - Type Alias Definitions (P, R, F) (init.py) - Reason this comment was not posted:
Marked as duplicate.
2. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:163
- Draft comment:
Using **({"cls": cls} if cls else {}) in json.dumps calls slightly reduces readability. Consider extracting the kwargs to a variable. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:11
- Draft comment:
Good use of ParamSpec and generic types (P, R, F) to enhance the type-safety of decorators. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:227
- Draft comment:
Removal of the try/except block in the async wrapper (async_wrap) now omits explicit exception logging (span.set_status and span.record_exception). Confirm if losing this span error reporting is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code seems to be going through a refactor to unify sync and async handling. Both sync_wrap and async_wrap now have identical error handling approaches - they don't explicitly handle exceptions but rely on cleanup in finally blocks. This seems intentional for consistency. The generator functions still have explicit error handling because they have different requirements. I could be missing some OpenTelemetry best practices around error handling in spans. Maybe there's a reason why async functions specifically need explicit error handling. The consistent approach between sync and async code suggests this is an intentional design choice. If error handling was critical, it would likely be present in both sync and async versions. The comment should be deleted as the change appears intentional for consistency between sync and async code.
5. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:245
- Draft comment:
The sync wrapper (sync_wrap) no longer uses a try/except to record exceptions on the span. This change removes the explicit error reporting (span.set_status and span.record_exception); please verify if this behavior change is intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_jTuypfeKhSIFg4iF
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| entity_method, | ||
| ) | ||
|
|
||
| P = ParamSpec("P") |
Contributor
There was a problem hiding this comment.
These type aliases are already defined in base.py. Consider importing them instead of duplicating the definitions.
- Type Alias Definitions (P, R, F) (base.py)
| ) | ||
| _handle_span_input(span, args, kwargs, cls=JSONEncoder) | ||
|
|
||
| try: |
Contributor
There was a problem hiding this comment.
The async and sync wrappers in entity_method no longer catch exceptions to set span status and record exceptions. This change may alter telemetry behavior by not marking errors on spans.
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 1bc4845 in 1 minute and 1 seconds. Click for details.
- Reviewed
310lines of code in2files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:1
- Draft comment:
Good use of ParamSpec and TypeVar for improved type safety. This aligns well with modern type hinting practices. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:16
- Draft comment:
The return type annotation '-> Callable[[F], F]' on decorators ensures proper type safety for both sync and async functions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:206
- Draft comment:
The async generator and async function wrappers now consistently use explicit type annotations and cast(F, ...), which improves readability and type safety. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:21
- Draft comment:
Added return type annotations using generics (Callable[[F], F]) for decorators, which improves type safety. Consider documenting the Python version requirement for features like ParamSpec and the union operator (|). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:82
- Draft comment:
The deprecation warnings in decorators like @atask now include improved formatting with trailing commas. This consistent style is good; just ensure the deprecation strategy is clearly documented. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:206
- Draft comment:
Enhanced type annotations for async generator and async function wrappers in 'entity_method' improve the clarity of expected function signatures. The use of cast helps enforce type inference. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:163
- Draft comment:
The json.dumps calls in _handle_span_input have been refactored for better readability. Consider whether additional context should be logged on serialization errors. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
8. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:32
- Draft comment:
Using the PEP 604 union operator (|) and ParamSpec requires Python 3.10 or later. Ensure that this version requirement is documented in the project guidelines. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_eruhR2jRUuxKbn4j
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
galzilber
approved these changes
Apr 28, 2025
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed bd6594c in 1 minute and 43 seconds. Click for details.
- Reviewed
317lines of code in2files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:11
- Draft comment:
Good use of ParamSpec and improved type annotations for decorator functions. The return type Callable[[F], F] is clear, ensuring better type safety. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:16
- Draft comment:
Return type annotation (-> Callable[[F], F]) for decorators like task, workflow, agent, etc. improves consistency and type safety. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:4
- Draft comment:
Updated import block now includes ParamSpec and Awaitable. This is a beneficial upgrade for type safety. Ensure that the project uses Python 3.10+ if relying on union operator (|) syntax. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:207
- Draft comment:
Type annotations in async generator wrapper 'async_gen_wrap' are improved. The use of explicit *args: Any, **kwargs: Any is good for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:249
- Draft comment:
The sync_wrap function now checks if the result is a generator and delegates to _handle_generator appropriately. The finally block now consistently calls _cleanup_span, ensuring proper context cleanup. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:237
- Draft comment:
Using cast(F, ...) when returning the wrapped functions ensures the decorator preserves type signatures as expected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:86
- Draft comment:
Consider adding explicit return type annotations for the generator functions (_handle_generator and _ahandle_generator) to improve clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:223
- Draft comment:
Document the use of 'cast(F, ...)' in the decorators to clarify that it preserves the original function's signature. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The cast is part of the type system implementation and helps preserve function signatures. While documentation could be helpful, this seems more like a nice-to-have than a critical issue. The code is already well-typed and the cast usage is a standard Python typing pattern. The comment doesn't point out any actual problems or required changes. The cast usage could be non-obvious to developers unfamiliar with Python typing. Documenting it could help future maintainers understand the type system better. While documentation would be nice, we should only keep comments that require clear code changes. This is more of an optional suggestion than a required fix. Delete the comment as it's a nice-to-have documentation suggestion rather than a required code change.
9. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:163
- Draft comment:
The revised json.dumps formatting (using **({"cls": cls} if cls else {})) is clearer; ensure this style is applied consistently across similar calls. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_nsItA8udqCCwqd1p
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| @@ -1,4 +1,4 @@ | |||
| from typing import Optional | |||
| from typing import Optional, TypeVar, Callable, Any, Type, cast, ParamSpec, Awaitable | |||
Contributor
There was a problem hiding this comment.
Remove the unused import 'Type'—it isn’t used in this file.
Suggested change
| from typing import Optional, TypeVar, Callable, Any, Type, cast, ParamSpec, Awaitable | |
| from typing import Optional, TypeVar, Callable, Any, cast, ParamSpec, Awaitable |
bcec183 to
88c335e
Compare
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 88c335e in 1 minute and 58 seconds. Click for details.
- Reviewed
330lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:11
- Draft comment:
Excellent use of ParamSpec and TypeVar for function decorators. The return typeCallable[[F], F]clearly improves type safety. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:16
- Draft comment:
Return type annotation added for the decorator functions (e.g., task, workflow). This ensures the decorated function signature is preserved. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:4
- Draft comment:
Reorganized imports to include 'types' after type imports is fine. The explicit import helps prevent potential confusion on available types. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:206
- Draft comment:
The change from using 'yield from' to an explicit for loop in the _handle_generator function is clear and consistent. This improves clarity, especially when managing async generators. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:245
- Draft comment:
Good use of 'cast(F, ...)' on the decorated functions to maintain type consistency. This is a best practice when wrapping functions with decorators. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:16
- Draft comment:
Good improvement—adding ParamSpec, TypeVar, and explicit return types boosts type safety in these decorators. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_JeNLYCJ2vnMplCZD
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| span.record_exception(e) | ||
| _cleanup_span(span, ctx_token) | ||
| raise | ||
|
|
Contributor
There was a problem hiding this comment.
Bug: In the sync_wrap function inside entity_method, the result handling logic has been removed. Instead of processing and returning res (with _handle_span_output and _cleanup_span), the function returns cast(F, sync_wrap), which incorrectly returns the decorator itself rather than the intended value.
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a66c316 in 1 minute and 13 seconds. Click for details.
- Reviewed
324lines of code in2files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:21
- Draft comment:
Good improvement: the return type is now clearly defined as Callable[[F], F]. This boosts type safety. Make sure that the use of ParamSpec and type union (R | Awaitable[R]) is supported in all target environments. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:87
- Draft comment:
Consistent formatting for deprecation warnings: comma formatting (trailing commas) now improves readability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:207
- Draft comment:
The async generator wrapper (async_gen_wrap) now explicitly types its parameters and return. Using cast(F, async_gen_wrap) is a valid way to preserve type hints. Ensure that future modifications maintain this consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:248
- Draft comment:
The sync_wrap function now uses explicit type annotations for *args and **kwargs. This is a positive change for clarity and type safety. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:13
- Draft comment:
Ensure the use of the union operator ('R | Awaitable[R]') in the type bound for F is intentional; this requires Python 3.10+. Verify compatibility with your target Python version. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py:21
- Draft comment:
The decorator 'task' now explicitly returns a Callable[[F], F], which improves type safety by clearly preserving the input function signature. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:90
- Draft comment:
Replacing 'yield from res' with an explicit for-loop is functionally equivalent, but 'yield from' is more idiomatic. Consider using 'yield from res' for conciseness unless the explicit loop is required. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
8. packages/traceloop-sdk/traceloop/sdk/decorators/base.py:222
- Draft comment:
Using cast(F, ...) when returning the wrapped function ensures proper type inference. This is a good improvement for preserving the original function's signature. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_oYlhEYiXswIs19wO
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
1 task
nina-kollman
pushed a commit
that referenced
this pull request
Aug 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat(instrumentation): ...orfix(instrumentation): ....Important
Enhance type safety in decorators by using
TypeVar,Callable, andParamSpec, and update function signatures in__init__.pyandbase.py.TypeVar,Callable,ParamSpec, andAwaitablein__init__.pyandbase.pyfor improved type safety.task,workflow,agent,tool,atask,aworkflow,aagent,atool,entity_method, andentity_classtoCallable[[F], F].yield fromwithfor item in res: yield itemin_handle_generator()inbase.py.base.py.This description was created by
for a66c316. You can customize this summary. It will automatically update as commits are pushed.