- 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][CLI] fix ray status long decimal numbers#43480
base:master
Are you sure you want to change the base?
Conversation
Could you add some tests? |
@jjyao Sure. Will do. |
244dc31 toc90827eCompareDone! This code path is test covered too. |
cf8cceb to8f62a81CompareSigned-off-by: Hongchao Deng <hongchaodeng1@gmail.com>Signed-off-by: hongchaodeng <hongchaodeng1@gmail.com>
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.
Approach looks good to me!
Just not sure what's the best UX here - pinging Huaiwei for some expertise here.
| @@ -650,6 +650,17 @@ def parse_usage(usage: Usage, verbose: bool) -> List[str]: | |||
| return usage_lines | |||
| def format_resource(number): | |||
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.
if we do with 2 decimal places - we should rename the function.
| @@ -563,8 +563,8 @@ def test_cluster_status_formatter(): | |||
| Resources | |||
| -------------------------------------------------------- | |||
| Total Usage: | |||
| 0.5/3.0 CPU | |||
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 old one actually looks better to me.
@scottsun94 could you maybe chime in here - what's the best way to show decimal places in your opinion?
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.
Hmm. It does feel that something is broken when I see something like0.01/4 GPU
I don't know what's the best solution here. What if we only keep the decimal places the same/consistent before and after/ on the same row?
Something like?
1/3 CPU1.01/4.00 GPU0.5/3.0 CPU0.01/2.00 GPU2/4 CPU0.51/1.00 GPU0/1 CPU0/2 GPUbtw - we use assignees for PR reviews. |
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.
Isn't this more likely a bug from floating point calculation in the scheduler layer? IIUC this approach won't work if there are resources that are smaller than 0.01?
Why are these changes needed?
Related issue number
Closes#42491
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.