Skip to content

Commit

Permalink
Clarify and document commit choice for CI jobs (pytorch#54967)
Browse files Browse the repository at this point in the history
Summary:
PRs pytorch#53652 and pytorch#54693 attempted to increase the consistency of our choice of commit (head vs merge) for CI on PRs, and have so far been unsuccessful. This PR takes a less ambitious approach to the problem by clarifying the choice in one specific way (see the following paragraph) and documenting it in `CONTRIBUTING.md`.

In addition to documentation, this PR also removes the current behavior of our GHA jobs that checkout the PR tip instead of the merge commit. At first glance, this behavior seems to increase consistency (by eliminating the special-case for `ghstack` PRs), but in reality, it actually just means that for non-`ghstack` PRs, the question "Which commit is used in CI?" has *two* answers instead of just one; see the description of pytorch#53652 for more details.

Once merged, this PR will unblock other PRs that add modify our GHA workflows in breaking ways, such as pytorch#54737.

Pull Request resolved: pytorch#54967

Test Plan: None.

Reviewed By: walterddr, seemethere

Differential Revision: D27435913

Pulled By: samestep

fbshipit-source-id: 405fb419cf015cf88107d5eb2498cfb5bcb7ce33
  • Loading branch information
samestep authored and facebook-github-bot committed Mar 30, 2021
1 parent 18e61d1 commit eafa235
Show file tree
Hide file tree
Showing 7 changed files with 57 additions and 19 deletions.
2 changes: 0 additions & 2 deletions .github/workflows/build_linux_conda.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ jobs:
steps:
- name: Clone pytorch/pytorch
uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
- name: Generating build matrix
id: set-matrix
run: |
Expand Down
2 changes: 0 additions & 2 deletions .github/workflows/build_linux_libtorch.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ jobs:
steps:
- name: Clone pytorch/pytorch
uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
- name: Generating build matrix
id: set-matrix
run: |
Expand Down
2 changes: 0 additions & 2 deletions .github/workflows/build_linux_wheels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ jobs:
steps:
- name: Clone pytorch/pytorch
uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
- name: Generating build matrix
id: set-matrix
run: |
Expand Down
1 change: 0 additions & 1 deletion .github/workflows/clang_format.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ jobs:
- name: Fetch PyTorch
uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0 # deep clone, to allow us to use git merge-base
- name: Run clang-format
run: |
Expand Down
9 changes: 0 additions & 9 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ jobs:
architecture: x64
- name: Checkout PyTorch
uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
- name: Ensure consistent CircleCI YAML config
run: |
pip install -r requirements.txt
Expand Down Expand Up @@ -66,8 +64,6 @@ jobs:
architecture: x64
- name: Fetch PyTorch
uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
- name: Get HEAD commit SHA
run: echo ::set-output name=commit-sha::$(git rev-parse HEAD)
id: get-commit-sha
Expand Down Expand Up @@ -102,7 +98,6 @@ jobs:
- name: Checkout PyTorch
uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0 # deep clone, to allow us to use git merge-base
- name: Get HEAD commit SHA
run: echo ::set-output name=commit-sha::$(git rev-parse HEAD)
Expand Down Expand Up @@ -203,8 +198,6 @@ jobs:
architecture: x64
- name: Fetch PyTorch
uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
- name: Run cmakelint
run: |
set -eux
Expand All @@ -224,8 +217,6 @@ jobs:
architecture: x64
- name: Fetch PyTorch
uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
- name: Install dependencies
run: |
set -eux
Expand Down
1 change: 0 additions & 1 deletion .github/workflows/test_tools.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ jobs:
- name: Checkout PyTorch
uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0 # deep clone, to allow us to use git log
- name: Install dependencies
# mypy and boto3 versions copied from
Expand Down
59 changes: 57 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
- [Why no leak detection?](#why-no-leak-detection)
- [Caffe2 notes](#caffe2-notes)
- [CI failure tips](#ci-failure-tips)
- [Which commit is used in CI?](#which-commit-is-used-in-ci)

## Contributing to PyTorch

Expand Down Expand Up @@ -1137,8 +1138,9 @@ Once you submit a PR or push a new commit to a branch that is in
an active PR, CI jobs will be run automatically. Some of these may
fail and you will need to find out why, by looking at the logs.

Fairly often, a CI failure might be unrelated to your changes. In this
case, you can usually ignore the failure.
Fairly often, a CI failure might be unrelated to your changes. In this case, you
can usually ignore the failure. See [the following
subsection](#which-commit-is-used-in-ci) for more details.

Some failures might be related to specific hardware or environment
configurations. In this case, if the job is run by CircleCI, you can
Expand Down Expand Up @@ -1169,3 +1171,56 @@ following steps:
For certain Windows failures, it may be useful to have a full [Remote
Desktop](https://docs.microsoft.com/en-us/windows-server/remote/remote-desktop-services/clients/remote-desktop-clients) connection. See detailed instructions [here](https://github.com/pytorch/pytorch/wiki/Debugging-Windows-with-Remote-Desktop-or-CDB-(CLI-windbg)-on-CircleCI)
for how to set that up after rerunning the job.

### Which commit is used in CI?

For CI run on `master`, this repository is checked out for a given `master`
commit, and CI is run on that commit (there isn't really any other choice). For
PRs, however, it's a bit more complicated. Consider this commit graph, where
`master` is at commit `A`, and the branch for PR #42 (just a placeholder) is at
commit `B`:

```
o---o---B (refs/pull/42/head)
/ \
/ C (refs/pull/42/merge)
/ /
---o---o---o---A (refs/heads/master)
```

There are two possible choices for which commit to use:

1. Checkout commit `B`, the head of the PR (manually committed by the PR
author).
2. Checkout commit `C`, the hypothetical result of what would happen if the PR
were merged into `master` (automatically generated by GitHub).

This choice depends on several factors; here is the decision tree as of
2021-03-30:

- For CI jobs on CircleCI:
- If the name of the job (or one of its ancestors in the workflow DAG)
contains "xla" or "gcc5", choice **2** is used. This includes the following
jobs:
- pytorch_linux_xenial_py3_6_gcc5_4_build
- pytorch_cpp_doc_build
- pytorch_doc_test
- pytorch_linux_backward_compatibility_check_test
- pytorch_linux_xenial_py3_6_gcc5_4_jit_legacy_test
- pytorch_linux_xenial_py3_6_gcc5_4_test
- pytorch_python_doc_build
- pytorch_xla_linux_bionic_py3_6_clang9_build
- pytorch_xla_linux_bionic_py3_6_clang9_test
- Otherwise, choice **1** is used.
- For CI jobs on GitHub Actions:
- If the PR was created using [`ghstack`](https://github.com/ezyang/ghstack),
choice **1** is used.
- Otherwise, choice **2** is used.

This is important to be aware of, because if you see a CI failure on your PR and
choice **2** is being used for that CI job, it is possible that the failure is
nondeterministically caused by a commit that does not exist in the ancestry of
your PR branch. If you happen to have write access to this repo, you can choose
to use `ghstack` to eliminate this nondeterminism for GitHub Actions jobs on
your PRs, but it will still be present for the select CircleCI jobs listed
above.

0 comments on commit eafa235

Please sign in to comment.