Skip to content

Conversation

harshit-anyscale
Copy link
Contributor

@harshit-anyscale harshit-anyscale commented Oct 13, 2025

  • default deployment name was changed to _TaskConsumerWrapper after async inference implementation, fixed it now

@harshit-anyscale harshit-anyscale requested a review from a team as a code owner October 13, 2025 04:07
@harshit-anyscale harshit-anyscale self-assigned this Oct 13, 2025
@harshit-anyscale harshit-anyscale added the go add ONLY when ready to merge, run all tests label Oct 13, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request fixes an issue where the default deployment name was being incorrectly set to _TaskConsumerWrapper for deployments using the @task_consumer decorator. The fix involves explicitly setting the __name__ of the wrapper class to that of the original class, which is a correct solution. A new test case is also added to verify this fix. I've suggested a small improvement to make the class wrapper more robust by copying additional attributes from the original class, which is a good practice for writing decorators.

Comment on lines +161 to +162
# Preserve the original class name
_TaskConsumerWrapper.__name__ = target_cls.__name__
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

It's good that you're preserving the class name. To make the wrapper class an even better proxy for the original, it's good practice to copy other dunder attributes as well, such as __module__ and __doc__. This is what functools.update_wrapper would do, and it improves the introspection capabilities and maintainability of the decorated class.

Suggested change
# Preserve the original class name
_TaskConsumerWrapper.__name__ = target_cls.__name__
# Preserve attributes from the original class
_TaskConsumerWrapper.__name__ = target_cls.__name__
_TaskConsumerWrapper.__module__ = target_cls.__module__
_TaskConsumerWrapper.__doc__ = target_cls.__doc__

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 ? how is this done with current deployment class?

@ray-gardener ray-gardener bot added the serve Ray Serve Related Issue label Oct 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go add ONLY when ready to merge, run all tests serve Ray Serve Related Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants