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

[HUD] Add job-status "QUEUED" #5859

Merged
merged 15 commits into from
Nov 5, 2024
Merged

[HUD] Add job-status "QUEUED" #5859

merged 15 commits into from
Nov 5, 2024

Conversation

yangw-dev
Copy link
Contributor

@yangw-dev yangw-dev commented Nov 4, 2024

Overview

#5822
Add Queued status

Details

  • add Queued value in conclusion based on job.status
  • add the UI changes in both the commit view, and the main trunk view.
  • replace all string such as 'pending` to enum value JobStatus

Notice

there are other sqls are using 'pending' as default conclusion for empty conclusion value , may create followup issues to address those.

Next Steps

replace the text to react-icons for rendering.

Copy link

vercel bot commented Nov 4, 2024

@yangw-dev is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 4, 2024
Copy link

vercel bot commented Nov 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
torchci ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 5, 2024 10:12pm

@yangw-dev yangw-dev changed the title [UI] Add Pending and InProgress status [HUB] Add job-status "QUEUED" Nov 5, 2024
@huydhn
Copy link
Contributor

huydhn commented Nov 5, 2024

At first glance, I think we should keep the same familiar yellow ? for pending job as the majority of them will be in pending state (screenshot). IMO, this helps me keep the familiar context when I see them and only focus on the queueing part that stands out (dark/black Q):

Screenshot 2024-11-04 at 21 28 24

@huydhn
Copy link
Contributor

huydhn commented Nov 5, 2024

I think you do have the information about the total number of jobs in the queue. An interesting feature here would be to use the same gray color as the ~ character for queued job as long as the their number is lower than a threshold, i.e. 50 jobs in the queue. This signifies that everything operates normally.

On the other hand, if the number of queue jobs is higher than that threshold, meaning something is not right, we can switch the color to orange, and it would surely catch our attention.

This can be done in a follow-up PR though I think

@huydhn huydhn marked this pull request as ready for review November 5, 2024 05:42
@huydhn
Copy link
Contributor

huydhn commented Nov 5, 2024

Nit: Could we get rid of the show log part when the job is pending or in queue as there is nothing to show there yet except and S3 error that the key doesn't exist?

Screenshot 2024-11-04 at 21 44 20

@yangw-dev
Copy link
Contributor Author

yangw-dev commented Nov 5, 2024

character

Hi Huy, I will replace all of those text (S,Q,P) with react-icon in next PR, so this is temporary, we can def discuss the color change in next PR

@yangw-dev
Copy link
Contributor Author

Nit: Could we get rid of the show log part when the job is pending or in queue as there is nothing to show there yet except and S3 error that the key doesn't exist?

Screenshot 2024-11-04 at 21 44 20

I agree the log is not very useful here, @clee2000 , thoughts?

@yangw-dev
Copy link
Contributor Author

yangw-dev commented Nov 5, 2024

I think you do have the information about the total number of jobs in the queue. An interesting feature here would be to use the same gray color as the ~ character for queued job as long as the their number is lower than a threshold, i.e. 50 jobs in the queue. This signifies that everything operates normally.

On the other hand, if the number of queue jobs is higher than that threshold, meaning something is not right, we can switch the color to orange, and it would surely catch our attention.

This can be done in a follow-up PR though I think

I changed the queued signal to grey for general purpose now

@clee2000 clee2000 changed the title [HUB] Add job-status "QUEUED" [HUD] Add job-status "QUEUED" Nov 5, 2024
Copy link
Contributor

@clee2000 clee2000 left a comment

Choose a reason for hiding this comment

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

Personally I prefer yellow over gray for queued, but given you're going to change the icons in a different PR I don't think it matters much

@yangw-dev yangw-dev merged commit 37358dc into pytorch:main Nov 5, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants