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

Add driver logs to Jobs page for submission jobs #34514

Merged
merged 12 commits into from
Apr 20, 2023

Conversation

alanwguo
Copy link
Contributor

@alanwguo alanwguo commented Apr 18, 2023

Why are these changes needed?

  • Add driver logs to Jobs page for submission jobs
  • Adds a refresh button to the log viewer to reload the logs.
  • Refactors the log viewer from the logs page into its own component
  • Updates the look and feel of the jobs page to match the new IA style.
  • Adds User-provided metadata to the job detail page. (fixes [Core|Dashboard] Support custom tags for jobs. #34187 )
  • Updates the table icon
  • Change "Tasks" to "Tasks/actor overview"
  • Adds Node Count Card next to ray status cards

Job detail page
Screenshot 2023-04-17 at 11 40 53 PM

Other job pages (new look and feel)
Screenshot 2023-04-17 at 11 19 35 PM
Screenshot 2023-04-17 at 11 19 38 PM

Jobs user-provided metadata
Screenshot 2023-04-18 at 12 00 45 AM
Screenshot 2023-04-18 at 12 00 52 AM

Autoscaler section
Screenshot 2023-04-18 at 12 29 16 AM

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

Is there any way to add a refresh button? Until we make it print last N lines & that's probably the best way to use this effectively.

@scottsun94
Copy link
Contributor

scottsun94 commented Apr 18, 2023

  1. The position of the "Expand" button is a bit weird. Can we put it always next to the text? (8px away)

Screen Shot 2023-04-17 at 11 27 23 PM

Screen Shot 2023-04-17 at 11 28 37 PM

  1. Shall we say "User-provided metadata" since the metadata can be arbitrary data. "User metadata" feels like that they are related to the user.

  2. It seems that the metadata is only available in jobs SDK. Is it something we should support in CLI?

  3. for the title job id, the font size seems a bit too big. Can we change it to match this in the figma

Screen Shot 2023-04-17 at 11 40 55 PM

Signed-off-by: Alan Guo <[email protected]>
Signed-off-by: Alan Guo <[email protected]>
Signed-off-by: Alan Guo <[email protected]>
Signed-off-by: Alan Guo <[email protected]>
} else if (driver_info && ipLogMap[driver_info.node_ip_address]) {
return `${ipLogMap[driver_info.node_ip_address]}/`;
}
})();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we need to add another else to catch the case

!driver_agent_http_address || (!driver_info || !ipLogMap[driver_info.node_ip_address])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No in that case, we do not show any logs so the default behavior of return undefined is what we want.

const host = (() => {
if (driver_agent_http_address) {
return `${driver_agent_http_address}/logs/`;
} else if (driver_info && ipLogMap[driver_info.node_ip_address]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this possible that driver_info.node_ip_address is undefined or driver_info.node_ip_address not exists in ipLogMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no for driver_info.node_ip_address, yes for the second thing. That's why we check for the value from the dictionary.

@alanwguo
Copy link
Contributor Author

@scottsun94 , want to take another look? I think I added the autoscaler row to the job detail page since you last reviewed

@scottsun94
Copy link
Contributor

I cannot seem to see the "User-provided metadata" field. Is that only available when it's not empty? Shall we just always show it? It might raise the awareness of this feature.
Screen Shot 2023-04-18 at 10 30 41 PM

@alanwguo
Copy link
Contributor Author

It's hidden for driver jobs because it's not possible to set metadata.

For submission jobs, it shows up as a "-" if empty.

@scottsun94
Copy link
Contributor

scottsun94 commented Apr 19, 2023

oh. I see. This is the same for driver logs.
Just throwing a thought: shall we actually expose both of them and add message bars/tooltips whichever is appropriate to educate users why it's not available and the difference between driver jobs and submission jobs? Otherwise, the control logic here is just mystical to them because they may assume all ray jobs should work in the same way

NVM. Let's get this in first and wait for more info.

@scottsun94
Copy link
Contributor

Everything LGTM. Thanks!

@rkooo567 rkooo567 merged commit dbbd43a into ray-project:master Apr 20, 2023
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
Add driver logs to Jobs page for submission jobs
Adds a refresh button to the log viewer to reload the logs.
Refactors the log viewer from the logs page into its own component
Updates the look and feel of the jobs page to match the new IA style.
Adds User-provided metadata to the job detail page. (fixes [Core|Dashboard] Support custom tags for jobs. ray-project#34187 )
Updates the table icon
Change "Tasks" to "Tasks/actor overview"
Adds Node Count Card next to ray status cards

Signed-off-by: elliottower <[email protected]>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
Add driver logs to Jobs page for submission jobs
Adds a refresh button to the log viewer to reload the logs.
Refactors the log viewer from the logs page into its own component
Updates the look and feel of the jobs page to match the new IA style.
Adds User-provided metadata to the job detail page. (fixes [Core|Dashboard] Support custom tags for jobs. ray-project#34187 )
Updates the table icon
Change "Tasks" to "Tasks/actor overview"
Adds Node Count Card next to ray status cards

Signed-off-by: Jack He <[email protected]>
architkulkarni pushed a commit to architkulkarni/ray that referenced this pull request May 16, 2023
Add driver logs to Jobs page for submission jobs
Adds a refresh button to the log viewer to reload the logs.
Refactors the log viewer from the logs page into its own component
Updates the look and feel of the jobs page to match the new IA style.
Adds User-provided metadata to the job detail page. (fixes [Core|Dashboard] Support custom tags for jobs. ray-project#34187 )
Updates the table icon
Change "Tasks" to "Tasks/actor overview"
Adds Node Count Card next to ray status cards
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.

[Core|Dashboard] Support custom tags for jobs.
4 participants