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 grouping for HUD View #198

Merged
merged 9 commits into from
Feb 14, 2022
Merged

Add grouping for HUD View #198

merged 9 commits into from
Feb 14, 2022

Conversation

zengk95
Copy link
Contributor

@zengk95 zengk95 commented Feb 14, 2022

See title

Test Plan:
Manually verified that it works.

Screen.Recording.2022-02-14.at.10.55.53.AM.mov

Main questionable things:

  1. Clicking on a grouped cell doesn't expand it, it just opens up a hover tip card that tells you which jobs are failed or pending.
  2. The way that we classified a failing grouping (in this one, we considered a cancelled job as failed group)
  3. Probably should have moved the grouping logic/parsing into the API
  4. Filtering doesn't work very well. You need to expand it to filter out specific jobs or uncheck grouped view
  5. Group view check box. I think most people tend to just view it as a group anyways so we don't necessarily need to give users this type of flexibility.

@vercel
Copy link

vercel bot commented Feb 14, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/torchci/torchci/2WiC4oLteFXgZcN3SzViNy4irXHi
✅ Preview: https://torchci-git-add-groups-torchci.vercel.app

@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 Feb 14, 2022
@janeyx99
Copy link
Contributor

janeyx99 commented Feb 14, 2022

  1. Clicking on a grouped cell doesn't expand it, it just opens up a hover tip card that tells you which jobs are failed or pending.

This is totally fine; I prefer this anyway.

  1. The way that we classified a failing grouping (in this one, we considered a cancelled job as failed group)

This is 100% correct since nothing should be canceled on trunk/release/nightly.

  1. Probably should have moved the grouping logic/parsing into the API

lol

  1. Filtering doesn't work very well. You need to expand it to filter out specific jobs or uncheck grouped view

Oh hm. Will this be improved in a later PR?

  1. Group view check box. I think most people tend to just view it as a group anyways so we don't necessarily need to give users this type of flexibility.

Yea, though doesn't hurt to have?

@zengk95
Copy link
Contributor Author

zengk95 commented Feb 14, 2022

  1. Clicking on a grouped cell doesn't expand it, it just opens up a hover tip card that tells you which jobs are failed or pending.

This is totally fine; I prefer this anyway.

  1. The way that we classified a failing grouping (in this one, we considered a cancelled job as failed group)

This is 100% correct since nothing should be canceled on trunk/release/nightly.

  1. Probably should have moved the grouping logic/parsing into the API

lol

  1. Filtering doesn't work very well. You need to expand it to filter out specific jobs or uncheck grouped view

Oh hm. Will this be improved in a later PR?

  1. Group view check box. I think most people tend to just view it as a group anyways so we don't necessarily need to give users this type of flexibility.

Yea, though doesn't hurt to have?

Yea I will address them in followup PRs. I think this works OK for now though.

  1. It doesn't hurt to have but just has a bunch of unnecessary code to differentiate between the two views.

@suo
Copy link
Member

suo commented Feb 14, 2022

Group view check box. I think most people tend to just view it as a group anyways so we don't necessarily need to give users this type of flexibility.

@mruberry, @ngimel, @albanD are we okay with landing this without a "don't use groups" view, or do you prefer that we add one first.

@zengk95
Copy link
Contributor Author

zengk95 commented Feb 14, 2022

9.

I might not have been clear, but there is a "dont use group" view checkbox at the top, near search bar that shows you basically the old version.

@albanD
Copy link

albanD commented Feb 14, 2022

I think it's fine yes. Unless you have a crazily wide screen, it is pretty unreadable without it :D

@suo
Copy link
Member

suo commented Feb 14, 2022

Can we get less horizontal space between grid columns? Right now, if I expand "linux" jobs, I can't see them all, even on my relatively large external monitor (which previously could fit all jobs)

@zengk95
Copy link
Contributor Author

zengk95 commented Feb 14, 2022

Can we get less horizontal space between grid columns? Right now, if I expand "linux" jobs, I can't see them all, even on my relatively large external monitor (which previously could fit all jobs)

Sounds good. I added spacing because I felt that the jobs names were overlapping with each other.

@@ -0,0 +1,196 @@
import { GroupData, RowData } from "./types";

const groups = [
Copy link
Member

Choose a reason for hiding this comment

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

I think we should come up with a more maintainable way of grouping jobs; a collection of regexes is likely to break over time and requires changes to the HUD to change the groups.

Should we enforce some convention on CI names and group on that basis? For example, we could do: - separated, from least specific grouping to most specific. e.g. linux-py3.7-cuda10.3, linux-py3.7-cpu-clang12, linux-py3.7-cpu-gcc7.3. This code could then be turned into name.split("-")[0], and eventually we can build a tree-ish structure similar to https://ci.chromium.org/p/chromium/g/main/console. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this from the old HUD just to keep parity between the two. I think changing the names is a good idea since some of the workflows and stuff have different naming standards like some of the builds vs publish to s3 or something. It might also allow for better classification/parsing and stuff like how you described but it seems a bit out of scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just made it
pytorch/pytorch#72814

One more followup: right now the logic for collapsing groups is wrong; if I have multiple groups open and collapse one, they will all collapse. Clicking to collapse a group should only collapse that group.

@suo
Copy link
Member

suo commented Feb 14, 2022

Sounds good. I added spacing because I felt that the jobs names were overlapping with each other.

yeah, I do agree that the group names look a little crowded. Ideally we can have different spacing for the group columns vs. job columns, maybe as a followup

re: overall naming/regex thing. I agree it shouldn't be blocking. Can you file an issue so we track the followup?

@suo
Copy link
Member

suo commented Feb 14, 2022

One more followup: right now the logic for collapsing groups is wrong; if I have multiple groups open and collapse one, they will all collapse. Clicking to collapse a group should only collapse that group.

@mruberry
Copy link

As long as we can expand the tabs and they enumerate the jobs in their own columns this seems like a fine default

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.

None yet

6 participants