-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Description
🐛 Describe the bug
The problem
I notice a curious discrepancy between Dr.CI and trymerge in #104121 where Dr.CI correctly identified inductor_torchbench as a broken trunk failure but trymerge didn't. Taking a closer look, it turns out that Dr.CI and trymerge used 2 different commits as the merge base.
- Dr.CI got the correct commit
803c14490b189f9b755ecb9f2a969876088ea243(June 26th) coming frommerge_base_commitin compare commits call https://github.com/pytorch/test-infra/blob/main/torchci/pages/api/drci/drci.ts#L127-L135. This can be easily verified by looking at https://github.com/pytorch/pytorch/commits/1cb87771c1efef32df7009d75bed08249df8ecad or using the REST API:
curl -L \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer <TOKEN>"\
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/pytorch/pytorch/compare/1cb87771c1efef32df7009d75bed08249df8ecad...main
- trymerge got the wrong commit
de7b6e55eb0f87f8d822f69bad6b4189a857b460(June 27th) coming frombaseRefOidin the GraphQL call https://github.com/pytorch/pytorch/blob/main/.github/scripts/trymerge.py#L188 to get the PR info. After looking at the API documentation https://docs.github.com/en/graphql/reference/objects#pullrequest,baseRefOidreferred to the head commit of the PR target branch, i.e. PyTorch main, at the time the PR was created or was rebased. This is not the same as the merge base commit.
This can be easily verify by running @pytorchbot rebase on a PR, Dr.CI merge_base_commit would point to the head of viable/strict as it should be while baseRefOid would point to the head of PyTorch main, the PR target branch.
Yet more problem
I tried to switch trymerge to use the same comparison API as Dr.CI, however, there is another problem in the discrepancy between GitHub GraphQL and its REST API. https://docs.github.com/en/graphql/reference/objects#comparison was added quite recently https://docs.github.com/en/graphql/overview/changelog#schema-changes-for-2022-10-18-1 and it seems to lack the merge_base_commit information returned by the REST API
Conclusion
- Dr.CI gets the correct merge base
- Rebase works but it might lead to a discrepancy in detecting broken trunk jobs between Dr.CI and trymerge
- In the above example Enable fused foreach Adam compilation #104121, the initial merge (without
-f) failed while it should succeeded. Thus, the author needed to use force merge instead.
- In the above example Enable fused foreach Adam compilation #104121, the initial merge (without
Versions
PyTorch CI, trymerge
Metadata
Metadata
Assignees
Labels
Type
Projects
Status