Skip to content

fix(Ray): Retain the original function name when patching Ray tasks #4858

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(Ray): Retain the original function name when patching Ray tasks #4858

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion sentry_sdk/integrations/ray.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import inspect
import functools
import sys

import sentry_sdk
Expand All @@ -17,7 +18,6 @@
import ray # type: ignore[import-not-found]
except ImportError:
raise DidNotEnable("Ray not installed.")
import functools

from typing import TYPE_CHECKING

Expand Down Expand Up @@ -54,6 +54,7 @@ def new_remote(f=None, *args, **kwargs):

def wrapper(user_f):
# type: (Callable[..., Any]) -> Any
@functools.wraps(user_f)
def new_func(*f_args, _tracing=None, **f_kwargs):
# type: (Any, Optional[dict[str, Any]], Any) -> Any
_check_sentry_initialized()
Expand All @@ -78,6 +79,19 @@ def new_func(*f_args, _tracing=None, **f_kwargs):

return result

# Patching new_func signature to add the _tracing parameter to it
# Ray later inspects the signature and finds the unexpected parameter otherwise
signature = inspect.signature(new_func)
params = list(signature.parameters.values())
params.append(
inspect.Parameter(
"_tracing",
kind=inspect.Parameter.KEYWORD_ONLY,
default=None,
)
)
new_func.__signature__ = signature.replace(parameters=params)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Tracing Parameter Patching Causes Signature Errors

The _tracing parameter patching logic unconditionally appends the parameter, which can lead to two issues: a ValueError if user_f already defines _tracing, or an invalid signature if **kwargs is present, as keyword-only parameters must precede **kwargs.

Fix in Cursor Fix in Web

Comment on lines +84 to +94
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential bug: Adding the _tracing parameter without checking for its existence can cause a ValueError if the user's function already has a parameter with that name, crashing the application.

  • Description: The Ray integration modifies the signature of a user's function to add a _tracing parameter. However, it does not first check if a parameter with that name already exists. If a user decorates a function that already has a _tracing parameter, the code attempts to add a duplicate. This will cause inspect.signature.replace() to raise a ValueError because duplicate parameter names are not allowed. This error is unhandled and will crash the application when the function is decorated, preventing the application from starting.

  • Suggested fix: Before appending the _tracing parameter to the params list, iterate through the existing parameters to check if one with the name _tracing already exists. If it does, do not append the new parameter.
    severity: 0.7, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.
if f:
rv = old_remote(new_func)
else:
Expand Down
Loading