Skip to content

Commit

Permalink
ensure deployment name does not contain # (#51003)
Browse files Browse the repository at this point in the history
## Why are these changes needed?

Currently, `#` is allowed in deployment names, but it is also used as a
delimiter when recovering from a checkpoint to infer the app,
deployment, and replica. This leads to ambiguity and potential issues
during recovery.

To resolve this, we are now prohibiting `#` in deployment names.

This change introduces an incompatibility for users who have already
used `#` in their deployment names but do not use the checkpointing
feature. I suspect this group is mostly tinkerers rather than production
users, but I'm interested in hearing the reviewers' thoughts.

An alternative approach would be to escape `#` during checkpoint
serialization and unescape it during deserialization. However, this felt
like unnecessary complexity for limited benefit. That said, I'm open to
revisiting this if reviewers prefer that approach.

## Related Issue

Fixes #48260  

## Checks

- Added unit tests.

---------

Signed-off-by: Abrar Sheikh <[email protected]>
  • Loading branch information
abrarsheikh authored Mar 4, 2025
1 parent ef041cd commit c6b06e1
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 2 deletions.
15 changes: 13 additions & 2 deletions python/ray/serve/deployment.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import logging
from copy import deepcopy
from typing import Any, Callable, Dict, List, Optional, Tuple, Union
import warnings

from ray.serve._private.config import (
DeploymentConfig,
Expand Down Expand Up @@ -101,8 +102,7 @@ def __init__(
"The Deployment constructor should not be called "
"directly. Use `@serve.deployment` instead."
)
if not isinstance(name, str):
raise TypeError("name must be a string.")
self._validate_name(name)
if not (version is None or isinstance(version, str)):
raise TypeError("version must be a string.")
docs_path = None
Expand All @@ -120,6 +120,17 @@ def __init__(
self._replica_config = replica_config
self._docs_path = docs_path

def _validate_name(self, name: str):
if not isinstance(name, str):
raise TypeError("name must be a string.")

# name does not contain #
if "#" in name:
warnings.warn(
f"Deployment names should not contain the '#' character, this will raise an error starting in Ray 2.46.0. "
f"Current name: {name}."
)

@property
def name(self) -> str:
"""Unique name of this deployment."""
Expand Down
15 changes: 15 additions & 0 deletions python/ray/serve/tests/unit/test_build_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,21 @@ class D:
],
)

# Change to `with pytest.raises(ValueError)` when the warning is removed.
with pytest.warns(UserWarning):

@serve.deployment(name="test#deployment")
def my_deployment():
return "Hello!"

with pytest.warns(UserWarning):

@serve.deployment()
def my_deployment():
return "Hello!"

my_deployment.options(name="test#deployment")


def test_multi_deployment_basic():
@serve.deployment(num_replicas=3)
Expand Down

0 comments on commit c6b06e1

Please sign in to comment.