-
Notifications
You must be signed in to change notification settings - Fork 111
Better handling of autorevert labels #7711
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
izaitsevfb
left a comment
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.
please fix the runtime issue with the set
ideally, add tests that should've been tripped by this issue
and PLEASE run it locally!
| DEFAULT_WORKFLOWS = ["Lint", "trunk", "pull", "inductor", "linux-aarch64", "slow"] | ||
| DEFAULT_WORKFLOW_RESTART_DAYS = 7 | ||
| # Users authorized to disable autorevert via circuit breaker issue | ||
| DEFAULT_CIRCUIT_BREAKER_APPROVED_USERS: set[str] = set( |
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.
this would fail with TypeError: set expected at most 1 argument, got 13 at import time,
please construct set as a literal, or from an iterable
| @@ -26,11 +37,29 @@ def check_autorevert_disabled(repo_full_name: str = "pytorch/pytorch") -> bool: | |||
| should_disable = False | |||
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.
this looks redundant, let's remove it?
|
With auto-revert working in auto-pilot mode, have we considered removing My thought process is:
|
|
@huydhn I guess you have a point. If everyone in the team is capable of going to AWS and disable the lambda schedulers this should do the trick. So, lets then remove all the shortcircuit logic, as this is extra complexity |
Summary
A user accidentally added the label ci: disable-autorevert to a PR, this is an honest mistake. But it silently disabled autorevert for the whole repo for 2 days. This is because we don't have validation on who can use this label, and the GitHub API considers PRs as issues, and we're only filtering by issues.
To address this, this PR:
Add guardrails to autorevert circuit breaker
Problem
The autorevert circuit breaker was activating on any open item (issue or PR) with the ci: disable-autorevert label. This created two issues:
Solution
Added two guardrails to the circuit breaker:
Only actual GitHub issues can activate the circuit breaker
PRs with the label are logged and skipped
Rationale: Circuit breaker is meant for explicit manual intervention via issues, not automatic PR labeling
Added optional approved_users parameter to restrict who can disable autorevert
defaults to:
Add validation for ci: disable-autorevert label on pull requests
Summary
Adds automatic validation to prevent users from accidentally using the ci: disable-autorevert label on pull requests. This label is intended for issues (to disable the entire autorevert system via circuit breaker) and is commonly confused with autorevert: disable (which prevents autorevert on a specific PR).
Changes
Behavior