- Notifications
You must be signed in to change notification settings - Fork6.2k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Core][Label scheduling 8/n]Add length and illegal letters validation to the node labels#38824
base:master
Are you sure you want to change the base?
Conversation
Why do we need to have such limitations? |
But i see our node_id length is 56, eg: |
I feel in general Ray doesn't enforce any limitations (not sure if it's a good thing or not): for example, we don't have limitations on custom resource name. So not sure what the limitations we should put on labels. |
I can understand your meaning. However, I think it would be better to add parameter limitations at the beginning rather than having no restrictions. |
c3b3446 to61ba740CompareThis pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.
|
@jjyao Can you help to review this pr ? |
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.
LG and very sorry for the late view!
python/ray/_private/utils.py Outdated
| len(key) >= ray_constants.RAY_LABEL_MAX_LENGTH | ||
| or len(key) == 0 | ||
| or len(value) >= ray_constants.RAY_LABEL_MAX_LENGTH | ||
| or len(value) == 0 |
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 can understand key cannot be empty but I guess value can be empty? There are cases where you only care if a key exists or not. (k8s label value can be 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.
It's ok， The value can be empty. I'll fix.
61ba740 tof5bb6dcComparef5bb6dc to23c2ee0Compare… to the node labelsSigned-off-by: LarryLian <554538252@qq.com>
23c2ee0 to90a9f0aCompare| f"This is reserved for Ray defined labels." | ||
| ) | ||
| if len(key) >= ray._raylet.RAY_LABEL_MAX_LENGTH or len(key) == 0: | ||
| raise ValueError( | ||
| "The keys length of custom lables " |
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.
| "Thekeys lengthofcustom lables " | |
| "Thekeyofnode labels " |
| raise ValueError( | ||
| "The keys length of custom lables " | ||
| "cannot be empty or exceed " | ||
| f"{ray._raylet.RAY_LABEL_MAX_LENGTH - 1} characters, " |
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.
It's a bit confusing. RAY_LABEL_MAX_LENGTH means it can be exactly that number of characters but not more.
| f"{ray._raylet.RAY_LABEL_MAX_LENGTH - 1} characters, " | ||
| f"invalid label key: {key}." | ||
| ) | ||
| if len(value) >= ray._raylet.RAY_LABEL_MAX_LENGTH: |
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.
Same here, instead of >=, it should be >
| ) | ||
| if len(value) >= ray._raylet.RAY_LABEL_MAX_LENGTH: | ||
| raise ValueError( | ||
| "The values length of custom lables " |
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.
| "Thevalues lengthofcustom lables " | |
| "Thevalueofnode labels " |
| if len(value) >= ray._raylet.RAY_LABEL_MAX_LENGTH: | ||
| raise ValueError( | ||
| "The values length of custom lables " | ||
| "cannot be exceed " |
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.
| "cannotbeexceed " | |
| "cannot exceed " |
| "The values length of custom lables " | ||
| "cannot be exceed " | ||
| f"{ray._raylet.RAY_LABEL_MAX_LENGTH - 1} characters, " | ||
| f"invalid label: {key}:{value}." |
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.
| f"invalid label:{key}:{value}." | |
| f"invalid label value:{value} for key{key}." |
| "The keys and values of custom lables must " | ||
| "consist of letters from `a-z0-9A-Z` or `-` or `_` or `.` or `/`, " | ||
| f"invalid label: {key}:{value}." |
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.
| "The keys and values ofcustom lables must " | |
| "consist of letters from`a-z0-9A-Z` or `-` or `_` or `.` or `/`," | |
| f"invalid label:{key}:{value}." | |
| "The keys and values ofnode labels can " | |
| "only contain`a-zA-Z0-9\-_./`" | |
| f"invalid label:{key}:{value}." |
| def is_valid_label_string(s): | ||
| if len(s) == 0: | ||
| return True | ||
| pattern = r"^[a-zA-Z0-9\-_./]+$" |
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.
Actually after some thoughts. I feel having a limitation on the length makes sense. But for valid characters, it seems a bit arbitrary. Why don't we allow: or+? We are also not following k8s since k8s doesn't allow\. Also I don't think we should strictly follow k8s.
Should we just have length limitations?
Why are these changes needed?
Add length and illegal letters validation to the node labels:
The keys and values of node labels:
a-z0-9A-Zor-or_or.or/Why?
cannot exceed 127 characters ?
The excessive length negatively impacts performance and can cause unknown code crashes. And using a length of 127 is completely sufficient to include the information of node ID and actor ID.
must consist of letters from
a-z0-9A-Zor-or_or.or/To avoid unknown issues caused by users inputting special characters,
Example:
Related issue number
#34894
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.