-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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] Use DeploymentID.__str__
to standardize log format.
#43708
Conversation
Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
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.
LGTM!
python/ray/serve/_private/common.py
Outdated
@@ -28,6 +28,15 @@ class DeploymentID: | |||
name: str | |||
app_name: str = SERVE_DEFAULT_APP_NAME | |||
|
|||
def __repr__(self): | |||
s = f"Deployment(name='{self.name}'" | |||
if not self.app_name or self.app_name == SERVE_DEFAULT_APP_NAME: |
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.
Non-blocker: I kinda feel it's fine to print "default" as the app name just in case if the user doesn't know what's the default value we use. Also, in the case when the user explicitly set it to be "default", it won't be omitted and cause confusion.
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.
Agreed, I also think it would be more clear if we print "default" as the app name. Also, now that 1.x deployments are fully deprecated, I think app_name
should always be non-empty
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.
Ok, I will update it to include app='default'
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.
overall lgtm to me as well!
python/ray/serve/_private/common.py
Outdated
@@ -28,6 +28,15 @@ class DeploymentID: | |||
name: str | |||
app_name: str = SERVE_DEFAULT_APP_NAME | |||
|
|||
def __repr__(self): | |||
s = f"Deployment(name='{self.name}'" | |||
if not self.app_name or self.app_name == SERVE_DEFAULT_APP_NAME: |
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.
Agreed, I also think it would be more clear if we print "default" as the app name. Also, now that 1.x deployments are fully deprecated, I think app_name
should always be non-empty
Signed-off-by: Edward Oakes <[email protected]>
DeploymentID.__repr__
to standardize log format.DeploymentID.__str__
to standardize log format.
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.
c74d70a
to
3882d29
Compare
Signed-off-by: Edward Oakes <[email protected]>
…oject#43708) Uses a custom repr for DeploymentID instead of hand-writing long strings describing each deployment. Before: ``` (ServeController pid=47618) INFO 2024-03-05 09:06:46,000 controller 47618 deployment_state.py:1634 - Deploying new version of deployment A in application 'default'. Setting initial target number of replicas to 1. (ServeController pid=47618) INFO 2024-03-05 09:06:46,104 controller 47618 deployment_state.py:1944 - Adding 1 replica to deployment 'A' in application 'default'. ``` After: ``` (ServeController pid=52016) INFO 2024-03-05 09:39:17,679 controller 52016 deployment_state.py:1629 - Deploying new version of Deployment(name='A', app='default') (initial target replicas: 1). (ServeController pid=52016) INFO 2024-03-05 09:39:17,781 controller 52016 deployment_state.py:1936 - Adding 1 replica to Deployment(name='A', app='default'). ``` I will do the same thing for replicas in: ray-project#43557 --------- Signed-off-by: Edward Oakes <[email protected]>
Why are these changes needed?
Uses a custom repr for
DeploymentID
instead of hand-writing long strings describing each deployment.Before:
After:
I will do the same thing for replicas in: #43557
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.