Skip to content

Conversation

clee2000
Copy link
Contributor

@clee2000 clee2000 commented Jun 2, 2025

I'm open to other solutions, this is pretty crude

Things I'm not sure how to handle:

  • If a test was recently reenabled, we like to link the issue since issue closing + change landing can be out of sync and knowing there was a disable issue can be useful context. But I would also prefer to not reopen aggregate issues and I'm not sure how to discourage that
  • If there are multiple disable issues for a test

Alt solutions:

Copy link

vercel bot commented Jun 2, 2025

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

Name Status Preview Updated (UTC)
torchci ✅ Ready (Inspect) Visit Preview Jun 2, 2025 10:48pm

@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 Jun 2, 2025
@clee2000 clee2000 marked this pull request as ready for review June 2, 2025 22:48
@clee2000 clee2000 requested a review from a team June 2, 2025 22:48
Copy link
Contributor

@ZainRizvi ZainRizvi left a comment

Choose a reason for hiding this comment

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

Thoughts on your concerns:

If a test was recently reenabled, we like to link the issue since issue closing + change landing can be out of sync and knowing there was a disable issue can be useful context. But I would also prefer to not reopen aggregate issues and I'm not sure how to discourage that

I think it's okay to reopen an aggregate issue if it was recently closed and a test started failing. There's a high chance the error was related (and we can change course easily if we discover otherwise)

If there are multiple disable issues for a test

Two options for this:

  1. Link all issues (ideal). Could add a link for each matching issue
  2. Pick a winner.
    With winner picking, it would be better to prefer showing issues that are specifically for a given test before showing aggregate issues the test falls under.

But really, showing all issues would be ideal here

Parse body to see if it matches better - pros: more accurate, cons: parsing

What kind of parsing do you have in mind? The body is auto-gened, so more parsing isn't that bad

Copy link
Contributor

@huydhn huydhn left a comment

Choose a reason for hiding this comment

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

LGTM! There is a chance that the test name might be mentioned in another issue, but I guess it's really low

@clee2000
Copy link
Contributor Author

What kind of parsing do you have in mind? The body is auto-gened, so more parsing isn't that bad

there are functions to parse the issue body but it seems kind of excessive, although maybe this should happen anyways so we can get the platform

@clee2000 clee2000 merged commit 58b40fc into main Jun 11, 2025
6 checks passed
@clee2000 clee2000 deleted the csl/agg_disable_issue_joblinks branch June 11, 2025 16:58
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