-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[serve] Support setting deployment options via kwargs #14935
Conversation
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.
I agree, the new API is more concise and Ray-like. I commented about one possible nit but otherwise looks great!
python/ray/serve/api.py
Outdated
ray_actor_options: Optional[Dict] = None, | ||
config: Optional[BackendConfig] = None) -> "Deployment": | ||
def options( | ||
self, |
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.
Hadn't seen @classmethod
before so I looked it up--it looks like it's idiomatic to for the first argument to be cls
instead of self
and to use cls.<...>
instead of Deployment.<...>
in the body of the method. I'm not sure if there's any concrete benefit to doing so though, unless we plan to subclass Deployment or change its name in the future.
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.
Ah yeah really good point, that would be much cleaner (it's weird to reference the class from within its own definition...)
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.
updated
Why are these changes needed?
See updated tests for details.
Less verbose UX, more closely matches Ray API.
Also clarifies the
init_args
behavior.Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.