-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[GHF] Small cleanup #77004
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
[GHF] Small cleanup #77004
Conversation
- Use double quotes for Python string literals (per https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#strings ) - Do not post comment of merge_on_green called with dry_run argument - Dismantle pyramid of doom (and avoid infinite while) by specifying timeout as exit criteria - Use `handle_exception` for revert workflow as well [ghstack-poisoned]
🔗 Helpful links
❌ 6 New FailuresAs of commit 4e4b181 (more details on the Dr. CI page): Expand to see more
🕵️ 6 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages
|
- Use double quotes for Python string literals (per https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#strings ) - Do not post comment of merge_on_green called with dry_run argument - Dismantle pyramid of doom (and avoid infinite while) by specifying timeout as exit criteria - Use `handle_exception` for revert workflow as well [ghstack-poisoned]
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.
LGTM. some minor comments.
last_exception = '' | ||
while True: | ||
elapsed_time = 0.0 | ||
while elapsed_time < 400 * 60: |
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.
curious why we have such long (6+ hours) timeout. Does merging PR also run additional CI jobs?
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.
No idea, 6h limits sounds wrong to me (and the job would probably be killed earlier)
@zengk95 care to explain?
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.
I was thinking the scenario where
Person creates PR > Triggers CI > Immediately after, they write merge this > CI can potentially take 4-5 hours to run so this script spins for around 6 hours (once we make those tests mandatory)? which only gives us a 1 or 2 hour overhead. I chose 6 hours cause I think that's the default GHA timeout.
Or the scenario where they retrigger CI by pushing another commit after like an hour or so and still want to land it when green.
- Use double quotes for Python string literals (per https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#strings ) - Do not post comment of merge_on_green called with dry_run argument - Dismantle pyramid of doom (and avoid infinite while) by specifying timeout as exit criteria - Use `handle_exception` for revert workflow as well [ghstack-poisoned]
@pytorchbot merge on green this |
Summary: - Use double quotes for Python string literals (per https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#strings ) - Do not add label if merge_on_green is called with dry_run argument - Dismantle pyramid of doom (and avoid infinite while) by specifying timeout as exit criteria - Use `handle_exception` for revert workflow as well - Do not double post comment if merge_on_green times out Pull Request resolved: #77004 Approved by: https://github.com/mehtanirav Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/9f3a497c62bac3f0cc47fe12d30430b2281f0d00 Reviewed By: malfet Differential Revision: D36250438 fbshipit-source-id: eb2165597b96889907e1a88bfa31fc7cdd72844e
Stack from ghstack:
timeout as exit criteria
handle_exception
for revert workflow as well