-
Notifications
You must be signed in to change notification settings - Fork 6.2k
[Core][Autoscaler] Refactor v2 Log Formatting #49350
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
Merged
Merged
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
d36e447
Initial commit to refactor v2 format code
ryanaoleary 9418577
Merge branch 'master' into refactor-v2-logs
ryanaoleary 56b4be8
Update refactor
ryanaoleary a5750a1
Remove fixed TODO comment
ryanaoleary ff1368b
Remove fixed comments and now unused methods
ryanaoleary aed3f7a
Cover index -1 case
ryanaoleary 949a295
Change classmethods to staticmethods
ryanaoleary 64776ef
Remove StringIO
ryanaoleary e6488ce
Refactor and format to make code easier to review
ryanaoleary d07db55
Fix comments
ryanaoleary f047ca6
Merge branch 'master' into refactor-v2-logs
ryanaoleary a7e437c
Fix tests and refactor failure_lines sorting
ryanaoleary d498395
Merge branch 'master' into refactor-v2-logs
ryanaoleary f6db4ea
Merge branch 'master' into refactor-v2-logs
ryanaoleary 2084439
Fix missing newline causing 0 len seperator
ryanaoleary f73f023
Return separator length from header_info
ryanaoleary File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 remember that the instance ID for KubeRay is the Pod name. Could you check whether the
ray status
result also shows the Pod name so that we can map K8s Pods to Ray instances?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 don't think
ray status
currently shows the Pod name, this is from my manual testing: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.
Will the key in the key-value pairs in Pending be the Pod name? That's my expectation.
In addition, have you manually tested this PR? The test below shows that either "head node" or "worker node" is appended to the end of the
Node: ...
line. For example,However, the above output from your manual testing is:
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 misunderstood your initial comment, I thought you were asking whether
ray status
currently shows the Pod name in KubeRay. The above snippet was using the ray 2.41 image. I've been running into issues building an image lately to test the new changes with the following Dockerfile:where the RayCluster Pods will immediately crash and terminate after pulling the image. Describing the RayCluster just shows:
The head Pod keeps immediately crashing and re-creating, so I can't get any more useful logs from the container. I tried building an image using the latest changes from master (i.e. I didn't use any of my python changes) and it still had the same issue, is this a problem you've seen before? As soon as I have a working image I can run a manual test to check for Pod name in the key-value pairs in Pending.
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 was just able to manually test it with my changes, here is the output of
ray status --verbose
with a Pending node:it look like
instance_id
isn't set to the Pod name, but some other generated unique ID. Looking at the Autoscaler logs, if we wanted it to output Pod name here we should usecloud_instance_id
: