Don't merge to master in CircleCI #53652

samestep · 2021-03-09T21:32:39Z

Currently, most of our CircleCI jobs run directly on the tip of a PR, but a significant few instead merge the PR into master before running (the two top-level build jobs in this list do the actual merging, and their sub-items use the results of those builds, thus inheriting the merged repo state):

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

This is extremely surprising and confusing: for the past few months I've been unsure whether or not our CircleCI setup merges PRs into master before building/testing them, and I would not have guessed (until today, when I finally dug into .circleci/config.yml to figure this out, leading to this PR), that the answer is "both yes and no".

Beyond the principle of least astonishment, here is a snippet from an internal doc explaining a bit more of the motivation for not merging into master on PRs:

Currently, our CircleCI workflows (and some of our GitHub Actions) merge into master before building/testing/etc. ... we do not want to retain this behavior, since .... PyTorch gets many commits per day, and master is frequently broken. By merging with master before running CI, we deny people the benefit of basing their PRs off viable/strict and thus give them worse signal. See these PRs for some discussion of this behavior on the GHA side:

Always checkout github.head_ref #49578

add checkout PR tip step for quick checks #49590

For some additional context, here's some of the Git blame timeline for the existing behavior:

Add "merge to master" step before build in CircleCI #11443 (2018-09-11) added the first instance of this behavior that I can find, but it seems to have been removed sometime in the following year? (I'm not sure how to use Git blame to find when something was removed following a commit, as opposed to changed prior to a commit)
Rebase CircleCI to master if it is gcc5_4 #26321 (2019-09-17) added the behavior again, apparently in order to serve the backward-compatibility test job (Add a CI Job to Check BC Changes in Function Schemas #26329), only for gcc5_4
Allow unrelated histories when rebasing to master #29699 (2019-11-13) made the merging operation more permissible
[resubmit] Rebase xla job on top master before running CI build. #37085 (2020-04-22) expanded the scope of the automerging, so now it happens for xla in addition to gcc5_4
For jobs need a merge, merge with origin/master for ghstack PRs. #38745 (2020-05-20) fixed a bug with the automerging when used with ghstack PRs
Do not rebase select nighly builds on top of master #46038 (2020-10-08) narrowed the scope of the automerging, so now it no longer happens for nightly jobs

I've added the authors and reviewers of those PRs as reviewers of this one, to hopefully come to a consensus on what to do here. I'm guessing that this PR as initially written is not the best solution, since this automerging seems to perhaps be required for our backward-compatibility test job (and perhaps also our XLA job(s)?), so if those require automerging, we can't get rid of it entirely. However, in that case, I would much prefer to isolate the parts of our Circle build workflow that use automerging, and document exactly which parts of the workflow automerge and why.

Test plan:

PR #53639 serves as a testbed for this PR. Specifically:

this CircleCI workflow shows the two failing build jobs with the current automerging behavior when a merge conflict is introduced
this CircleCI workflow shows those jobs and their descendants succeeding when this PR is cherry-picked

facebook-github-bot · 2021-03-09T21:32:54Z

💊 CI failures summary and remediations

As of commit dec71a5 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

facebook-github-bot

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

samestep · 2021-03-09T21:42:09Z

Here's a specific example of automerging causing bad signal on a PR that was based on viable/strict and thus should not have been getting mypy failures:

https://app.circleci.com/pipelines/github/pytorch/pytorch/278093/workflows/8ea223c2-0121-4dcb-8b70-8a5974c513dc/jobs/11159713

janeyx99 · 2021-03-09T22:21:35Z

.circleci/verbatim-sources/commands.yml

-            if [[ -n "$CIRCLE_PULL_REQUEST" && "$CIRCLE_BRANCH" != "nightly" ]]; then
-              PR_NUM=$(basename $CIRCLE_PULL_REQUEST)
-              CIRCLE_PR_BASE_BRANCH=$(curl -s https://api.github.com/repos/$CIRCLE_PROJECT_USERNAME/$CIRCLE_PROJECT_REPONAME/pulls/$PR_NUM | jq -r '.base.ref')
-              if [[ "${BUILD_ENVIRONMENT}" == *"xla"* || "${BUILD_ENVIRONMENT}" == *"gcc5"* ]] ; then


I agree with the isolating the automerging idea to the configs that need it, though from the list above, it seems like more jobs than just backwards compatibility toggled on automerging. I'm a little confused as I'm not quite sure how pytorch_bazel_test whose build environment seems to be pytorch-linux-xenial-py3.6-gcc7-bazel-test also made it onto the automerge list (based on this if logic).

Ah you're right! My bad, I misread the graph and put pytorch_bazel_test in there by mistake. Good catch, thanks!

seemethere · 2021-03-09T22:39:29Z

I did attempt to do this at one point, #45281

seemethere · 2021-03-09T22:41:23Z

cc @zcain117, @ailzhang for pytorch/xla perspective

ezyang · 2021-03-10T15:00:21Z

XLA is explained by Ailing in this comment #45281 (review)

To elaborate on BC test, rebasing is needed for BC test because if nightly is NEWER than your base branch, it will look to the BC test like you have REMOVED operators (that were added between your merge base, and nightly) and BC test will spuriously fail.

janeyx99 · 2021-03-10T16:15:37Z

If you were thinking of splitting the build into a merge-with-master vs not, I did something similar with coverage back in the day. This PR might be helpful: #46656

walterddr · 2021-03-10T18:23:15Z

To elaborate on BC test, rebasing is needed for BC test because if nightly is NEWER than your base branch, it will look to the BC test like you have REMOVED operators (that were added between your merge base, and nightly) and BC test will spuriously fail.

question on the BC test, wouldn't it make more sense to compare against base commit of the PR instead of nightly, after all it is trying to check if the PR introduce any BC breakage right? I can see there're 2 potential problems not doing so:

base can be too old and some APIs were changed in newer master commits, but i think most likely this would also result in a merge conflict against master.
we need to rebuild a binary from base commit every PR that could cost lots of CI resources

samestep · 2021-03-10T18:27:47Z

compare against base commit of the PR

Minor note: this can be difficult to do for ghstack PRs, particularly with some of the recent unreleased work we've done in ghstack: ezyang/ghstack#44

ezyang · 2021-03-10T19:19:54Z

question on the BC test, wouldn't it make more sense to compare against base commit of the PR instead of nightly, after all it is trying to check if the PR introduce any BC breakage right?

The way the BC test works is it literally downloads the nightly wheel and then run some code that imports torch to dump the set of operators. For an arbitrary base commit, no nightly necessarily exists.

TBH, I've always thought this design was kind of hacky and maybe there are better ways to do it (e.g., maintain a separate database that maps commit id to set of supported operators) but you'll have to do implement that first before you can tackle this problem.

ezyang · 2021-03-18T21:51:40Z

@samestep btw, you don't have to close this issue, I think it is a legitimate complaint, just one that isn't easily fixable right now.

samestep · 2021-03-18T21:53:31Z

@ezyang Ah thanks; yeah I'm just closing it for my own sake (so that my only open PRs are the ones that I am actively working on), but I do definitely intend to come back to this.

Summary: PRs #53652 and #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 #53652 for more details. Once merged, this PR will unblock other PRs that add modify our GHA workflows in breaking ways, such as #54737. Pull Request resolved: #54967 Test Plan: None. Reviewed By: walterddr, seemethere Differential Revision: D27435913 Pulled By: samestep fbshipit-source-id: 405fb419cf015cf88107d5eb2498cfb5bcb7ce33

Don't merge to master in CircleCI

dec71a5

samestep requested review from a team, ailzhang, albanD, ezyang, houseroad, malfet, seemethere and yf225 March 9, 2021 21:32

facebook-github-bot added the cla signed label Mar 9, 2021

facebook-github-bot reviewed Mar 9, 2021

View reviewed changes

samestep requested a review from walterddr March 9, 2021 21:39

janeyx99 reviewed Mar 9, 2021

View reviewed changes

samestep closed this Mar 18, 2021

samestep mentioned this pull request Mar 30, 2021

Clarify and document commit choice for CI jobs #54967

Closed

samestep deleted the circleci-no-merge branch April 1, 2021 19:57

Don't merge to master in CircleCI #53652

Don't merge to master in CircleCI #53652

Uh oh!

Conversation

samestep commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

samestep commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janeyx99 Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

samestep Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

seemethere commented Mar 9, 2021

Uh oh!

seemethere commented Mar 9, 2021

Uh oh!

ezyang commented Mar 10, 2021

Uh oh!

janeyx99 commented Mar 10, 2021

Uh oh!

walterddr commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samestep commented Mar 10, 2021

Uh oh!

ezyang commented Mar 10, 2021

Uh oh!

ezyang commented Mar 18, 2021

Uh oh!

samestep commented Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

samestep commented Mar 9, 2021 •

edited

Loading

facebook-github-bot commented Mar 9, 2021 •

edited

Loading

samestep commented Mar 9, 2021 •

edited

Loading

walterddr commented Mar 10, 2021 •

edited

Loading