Skip to content
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

ensure deployment name does not contain # #51003

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

Conversation

abrarsheikh
Copy link

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]>
@abrarsheikh abrarsheikh requested review from edoakes and zcin February 28, 2025 21:58
@edoakes
Copy link
Contributor

edoakes commented Mar 1, 2025

@abrarsheikh we need to be careful making such breaking changes. I would suggest adding a clear warning that the character will be banned in a future release, leaving that in the code for 1-2 releases, then making the breaking change.

Signed-off-by: Abrar Sheikh <[email protected]>
Comment on lines +130 to +131
f"Name cannot contain '#' character, it will be removed in future releases. "
f"Current name: {name}."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f"Name cannot contain '#' character, it will be removed in future releases. "
f"Current name: {name}."
f"Deployment names cannot contain '#' character, this will raise an error in a future release. "
f"Current name: {name}."

Copy link
Contributor

Choose a reason for hiding this comment

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

just making it more precise (users might not know what "name" this is referring to)

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.

[Serve] Deployment names containing #s cause parse failure during checkpoint recovery.
2 participants