Skip to content

fix(integrations/ray): Correctly pass keyword arguments to ray.remote function #4430

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

svartalf
Copy link

Monkey-patched implementation was passing the provided keyword arguments incorrectly due to a typo - "*kwargs" was used instead of "**kwargs" twice.

Fixed integration started hitting an assert in the Ray codebase that requires for users to use "@ray.remote" decorator either with no arguments and no parentheses, or with some of the arguments provided.

An additional wrapper function was added to support both scenarios.


Thank you for contributing to sentry-python! Please add tests to validate your changes, and lint your code using tox -e linters.

Running the test suite on your PR might require maintainer approval.

… function

Monkey-patched implementation was passing the provided keyword arguments incorrectly
due to a typo - "*kwargs" was used instead of "**kwargs" twice.

Fixed integration started hitting an assert in the Ray codebase that
requires for users to use "@ray.remote" decorator either with no arguments and no parentheses,
or with some of the arguments provided.

An additional wrapper function was added to support both scenarios.
@svartalf svartalf requested a review from a team as a code owner May 30, 2025 13:17
Copy link

codecov bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 56.41026% with 17 lines in your changes missing coverage. Please review.

Project coverage is 80.68%. Comparing base (c2d5a76) to head (c4b9702).

Files with missing lines Patch % Lines
sentry_sdk/integrations/ray.py 56.41% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4430      +/-   ##
==========================================
+ Coverage   80.67%   80.68%   +0.01%     
==========================================
  Files         142      142              
  Lines       15982    15988       +6     
  Branches     2729     2731       +2     
==========================================
+ Hits        12893    12900       +7     
  Misses       2232     2232              
+ Partials      857      856       -1     
Files with missing lines Coverage Δ
sentry_sdk/integrations/ray.py 61.33% <56.41%> (+3.36%) ⬆️

... and 2 files with indirect coverage changes

@svartalf
Copy link
Author

svartalf commented Jun 2, 2025

@sentrivana thank you for merging the master branch!

I fixed the mypy errors (and added a missing functools.wraps() call for better dev UX), should we do anything about the failing Codecov workflow?

@sentrivana
Copy link
Contributor

Hey @svartalf, thanks for the PR!

We still have to properly review this but until then could I ask you to please:

  • take a look at the failing Tasks tests -- looks like the last change introduced some breakage
  • fix the remaining mypy error (sentry_sdk/integrations/ray.py:122: error: Unused "type: ignore" comment [unused-ignore])

Re: the codecov warning, we can reevaluate if we need more test coverage afterwards too, but I think this should already be properly covered, will check when reviewing.

@svartalf
Copy link
Author

svartalf commented Jun 2, 2025

take a look at the failing Tasks tests -- looks like the last change introduced some breakage

Done! It looks like adding the @functools.wraps() decorator was a mistake, as we injecting an additional _tracing parameter to the user function, but wraps omits it somehow (?). Not sure about its behavior, it would be nice to explore it later.

fix the remaining mypy error (sentry_sdk/integrations/ray.py:122: error: Unused "type: ignore" comment [unused-ignore])

I removed this type: comment, but I'm still puzzled why mypy is unhappy about it:

  • ray package is not installed by default, so I would expect mypy to correctly raise an import-not-found error
  • And the comment itself was introduced 10 months ago in the original implementation, and it hasn't triggered any errors until this PR

Anyway, I guess, as long as mypy is happy… :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants