-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[BE] Refactor trymerge for readability #161637
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
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/161637
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ⏳ No Failures, 24 PendingAs of commit 5c4a1b9 with merge base ae8d319 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
.github/scripts/trymerge.py
Outdated
|
||
def last_commit_sha(self) -> str: | ||
# for commits, the oid is the sha | ||
return str(self.last_commit().get("oid", "")) |
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.
Don't you want to error out if oid
is not defined?
.github/scripts/trymerge.py
Outdated
return self.merge_base | ||
|
||
last_commit_oid = self.last_commit()["oid"] | ||
last_commit_oid = self.last_commit_sha() |
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.
Nit
last_commit_oid = self.last_commit_sha() | |
last_commit_sha = self.last_commit_sha() |
) | ||
|
||
def merge_changes( | ||
def merge_changes_locally( |
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 change feels unnecessary imo, but don't have a strong preference
The merge job was canceled or timed out. This most often happen if two merge requests were issued for the same PR, or if merge job was waiting for more than 6 hours for tests to finish. In later case, please do not hesitate to reissue the merge command |
@pytorchbot merge -f "Lint is green" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Pull Request resolved: #161560 Approved by: https://github.com/malfet ghstack dependencies: #161558, #161637
Two changes: - Extract getting the last_commit's sha into it's own function - Rename merge_changes to merge_changes_locally to better explain it's functionality Pull Request resolved: pytorch#161637 Approved by: https://github.com/seemethere, https://github.com/malfet ghstack dependencies: pytorch#161558
Pull Request resolved: pytorch#161560 Approved by: https://github.com/malfet ghstack dependencies: pytorch#161558, pytorch#161637
Stack from ghstack (oldest at bottom):
Two changes: