Rebase xla job on top master before running CI build. #36852

ailzhang · 2020-04-18T00:27:23Z

This PR tries to rebase on top of origin/master before building xla job.
I also saw a TODO in existing code which does a very similar thing (rebase on master for gcc5 jobs), so I just fixed the TODO by moving the logic into a separate step.
Currently the logic is:
For these gcc5 and xla jobs, we rebase on top of "target" branch before building.

This only happens on PRs.
"Target" branch is "origin/master" by default, but if it's trying to merge into a release branch, target branch will be the release branch.
I made the "target" branch a param mainly it's allow us to rebase on viable/strict if we want. But after a second thought, how quickly viable/strict moves forward is not controlled only by xla job, and it's hard to predict how long the breakage will last if it's not moving. But we do have control over how long a xla breakage lasts on origin/master (which should be short since we monitor it closely). So I currently want to keep origin/master and move to viable/strict when it's super stable.
There're jobs like pytorch_paralleltbb_linux_xenial_py3_6_gcc5_4_build which would fall into the rebase logic as well, but since those jobs doesn't run on PRs(so the old logic was essentially no-op), I didn't enabled the new logic on those jobs.

dr-ci · 2020-04-18T00:46:58Z

💊 Build failures summary and remediations

As of commit 0b7fe74 (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 on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 18 times.

houseroad · 2020-04-20T17:18:30Z

.circleci/config.yml

+              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')
+              echo "Rebase on top of $CIRCLE_PR_BASE_BRANCH branch before building in environment $BUILD_ENVIRONMENT"
+              if [[ "${CIRCLE_PR_BASE_BRANCH}" == "master" ]]; then


Why we would like to have separate handling for master based PR?

Hmmm so I was thinking about whether to make xla job rebase on top viable/strict as suggested by @albanD or origin/master so I opened a special case here that we could pass a param in.
I can indeed simplify the logic if we agree origin/master is a better option. cc: @ezyang @albanD wdyt?

master is not broken too often but we don't want chain failure where all CI fails as soon as master break I think.

It should be based off of the base branch that the PR is basing its changes off of.

Hardcoding origin/master should never be an option because that immediately breaks as soon as we do a release branch.

ezyang · 2020-04-20T19:39:25Z

.circleci/verbatim-sources/commands.yml

+              if [[ "${CIRCLE_PR_BASE_BRANCH}" == "master" ]]; then
+                git rebase "${REBASE_TARGET}"
+              else
+                git rebase "origin/${CIRCLE_PR_BASE_BRANCH}"


You should just merge with master here, there's no reason to actually do a rebase. The rebase is more likely to fail as each individual commit in the PR must merge cleanly.

We already have some existing logic for merging with master:

# TODO We may want to move the rebase logic to a separate step after checkout # Rebase to master only if in xenial_py3_6_gcc5_4 case if [[ "${CIRCLE_BRANCH}" != "master" && "${BUILD_ENVIRONMENT}" == *"gcc5"* ]]; then

is there any reason this couldn't have been reused in this case?

Hmmm no particular reason, I just thought rebase would be cleaner and easier than a merge :P
Yea I can revert it back to merging :D

Like I commented before, you should merge with whatever the base branch is for the PR.

If a base branch isn't available then it's a no-op.

@seemethere What do you mean by base branch for the PR? Do you mean CIRCLE_PR_BASE_BRANCH? It's indeed origin/master if you are merging into master, and it'll be release/1.5 if you want to merge into release/1.5. This is totally fine for release branches right?
Note this code block is also guarded by CIRCLE_PULL_REQUEST that it only runs on PRs not branches.

ezyang

Don't actually rebase?

seemethere

I'd highly recommend against hardcoding any merge / rebase logic against origin/master as it's likely to break whenever we do release branches.

We've already experienced this with 1.5.0 and we wouldn't like to repeat the exercise again for future releases.

seemethere · 2020-04-20T20:28:46Z

.circleci/config.yml

+              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')
+              echo "Rebase on top of $CIRCLE_PR_BASE_BRANCH branch before building in environment $BUILD_ENVIRONMENT"
+              if [[ "${CIRCLE_PR_BASE_BRANCH}" == "master" ]]; then


It should be based off of the base branch that the PR is basing its changes off of.

Hardcoding origin/master should never be an option because that immediately breaks as soon as we do a release branch.

seemethere · 2020-04-20T20:29:53Z

.circleci/verbatim-sources/commands.yml

+              if [[ "${CIRCLE_PR_BASE_BRANCH}" == "master" ]]; then
+                git rebase "${REBASE_TARGET}"
+              else
+                git rebase "origin/${CIRCLE_PR_BASE_BRANCH}"


Like I commented before, you should merge with whatever the base branch is for the PR.

If a base branch isn't available then it's a no-op.

facebook-github-bot

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

facebook-github-bot · 2020-04-22T16:20:32Z

@ailzhang merged this pull request in 191fa52.

facebook-github-bot · 2020-04-22T16:21:14Z

@ailzhang merged this pull request in 191fa52.

) Summary: Resubmit of #36852 Pull Request resolved: #37085 Differential Revision: D21180010 Pulled By: ailzhang fbshipit-source-id: c448b836e4c13b15860e0b69da76082bd644badd

ailzhang force-pushed the xla_rebase_master branch 7 times, most recently from 086231c to a99ffd8 Compare April 18, 2020 03:00

Rebase on top master before running CI build.

4988bf2

ailzhang force-pushed the xla_rebase_master branch from a99ffd8 to 4988bf2 Compare April 18, 2020 04:03

ailzhang requested review from ezyang and houseroad April 20, 2020 15:22

ailzhang changed the title ~~[TEST]Rebase xla job on top master before running CI build.~~ Rebase xla job on top master before running CI build. Apr 20, 2020

ailzhang requested a review from seemethere April 20, 2020 15:49

houseroad reviewed Apr 20, 2020

View reviewed changes

ezyang reviewed Apr 20, 2020

View reviewed changes

ezyang requested changes Apr 20, 2020

View reviewed changes

seemethere requested changes Apr 20, 2020

View reviewed changes

Rebase -> merge.

0b7fe74

ailzhang force-pushed the xla_rebase_master branch from 1fa4cf7 to 0b7fe74 Compare April 21, 2020 18:59

ailzhang requested review from ezyang, houseroad and seemethere April 21, 2020 21:46

ezyang approved these changes Apr 22, 2020

View reviewed changes

facebook-github-bot reviewed Apr 22, 2020

View reviewed changes

facebook-github-bot closed this in 191fa52 Apr 22, 2020

facebook-github-bot added the merged label Apr 22, 2020

ailzhang mentioned this pull request Apr 22, 2020

[resubmit] Rebase xla job on top master before running CI build. #37085

Closed

mruberry added the Merged label Oct 28, 2020

Rebase xla job on top master before running CI build. #36852

Rebase xla job on top master before running CI build. #36852

Uh oh!

Conversation

ailzhang commented Apr 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Apr 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 Build failures summary and remediations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ailzhang Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

seemethere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 22, 2020

Uh oh!

facebook-github-bot commented Apr 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ailzhang commented Apr 18, 2020 •

edited

Loading

dr-ci bot commented Apr 18, 2020 •

edited

Loading

ailzhang Apr 20, 2020 •

edited

Loading