-
Notifications
You must be signed in to change notification settings - Fork 562
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.
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
base: master
Are you sure you want to change the base?
Conversation
Without "@functools.wraps" added, Ray exposes Prometheus metrics with all tasks named "new_func"
default=None, | ||
) | ||
) | ||
new_func.__signature__ = signature.replace(parameters=params) |
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.
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
.
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) | ||
|
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.
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 causeinspect.signature.replace()
to raise aValueError
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 theparams
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.
Follow-up to the CI run:
|
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.
Hey @svartalf, thanks for the PR! TL;DR, it all looks good to me, but see below for a couple of optional suggestions.
Re: LLM suggestions, we could rename the arg to _sentry_tracing
or something less generic to reduce the likelihood of collisions. FWIW I don't think it's necessary, so feel free to ignore.
If you can add a test or integrate a check for this into one of the tests in the existing test suite, that'd be awesome.
Approving already though since it looks good to me as is, pending the type: ignore
.
Description
Without "@functools.wraps" added, Ray exposes Prometheus metrics with all tasks named "new_func"
Issues
Reminders
tox -e linters
.feat:
,fix:
,ref:
,meta:
)