Skip to content
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

[wip] [ci] doc-job-skip take #4 dry-run #8980

Merged
merged 41 commits into from
Dec 9, 2020

Conversation

stas00
Copy link
Contributor

@stas00 stas00 commented Dec 8, 2020

This is take 4 on attempting to find a reliable way to get a list of modified files of this PR.

Spent the whole day trying many different ideas, none worked. github API for
https://api.github.com/repos/${CIRCLE_USERNAME}/${CIRCLE_REPO_NAME}/pulls/${CIRCLE_PR_NUMER}"
is broken. It gives a bogus base.sha at times, e.g. when someone force-pushes into master and then you end up with a base.sha which has nothing to do with the fork. on github website everything is valid, but github API gives bogus info.

So after many attempts I give up on trying to get a reliable way via the SHA information provided via github or circleCI.

The only solution that seems to work is to replicate user's original branch on their fork. Cost is about 3secs.

To do that one has to clone user's repo, switch to their branch and find the branching point identical to what make fixup does. This is what the latest incarnation of this PR does.

This PR doesn't enable the skipping yet, just reporting what it would have done and dumps the list of modified files so that we could check if we get some edge cases again which this incarnation doesn't cover.

Please have a look and see if it looks safe to merge and then monitor for a while and if all seems in order then we can enable skipping.

This PR will not be able to handle PRs originating from github direct file edit as it can be seen from #8997 as CircleCI fails to pass PR number to the job in this situation :( The whole skip check is skipped in that case the job continues normally - we just don't get the saving on direct doc PRs. I'm still trying to see if CircleCI should be providing this data, since according to https://developer.github.com/webhooks/event-payloads/#pull_request this hook should be sending PR number to circleCI.

When I had the idea first little did I know that this trivial one-liner on user-side (we use it in make fixup) will turn out to be such a complicated and unreliable thing on CI.

@LysandreJik

@stas00 stas00 changed the title [wip] [ci] doc-job-skip take #4 [ci] doc-job-skip take #4 dry-run Dec 8, 2020
@stas00 stas00 changed the title [ci] doc-job-skip take #4 dry-run [wip] [ci] doc-job-skip take #4 dry-run Dec 8, 2020
@stas00 stas00 removed the request for review from LysandreJik December 8, 2020 22:28
@stas00
Copy link
Contributor Author

stas00 commented Dec 9, 2020

trying to self-heal PR

@stas00 stas00 closed this Dec 9, 2020
@stas00 stas00 reopened this Dec 9, 2020
@stas00 stas00 closed this Dec 9, 2020
@stas00
Copy link
Contributor Author

stas00 commented Dec 9, 2020

I guess continuing over here: #8997

@stas00 stas00 reopened this Dec 9, 2020
@stas00 stas00 closed this Dec 9, 2020
@stas00 stas00 reopened this Dec 9, 2020
@stas00 stas00 closed this Dec 9, 2020
@stas00
Copy link
Contributor Author

stas00 commented Dec 9, 2020

ok, succeeded at restoring this PR after force pushed mess up.

Thanks to this recipe: https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c

@stas00 stas00 reopened this Dec 9, 2020
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool workaround! Crazy that even the GitHub API is unreliable on that front.

@LysandreJik LysandreJik merged commit 5e637e6 into huggingface:master Dec 9, 2020
@stas00
Copy link
Contributor Author

stas00 commented Dec 9, 2020

let's please monitor new PRs closely, so that it doesn't somehow break the job while testing things. thank you.

@stas00
Copy link
Contributor Author

stas00 commented Dec 9, 2020

Crazy that even the GitHub API is unreliable on that front.

I haven't seen it with my own eyes (other than when force pushed history of master was rewritten yesterday), but I found more than one report of it being unreliable on various forums.

It's possible that they are referring to the situation when someone force pushes into the PR branch, thus changing the local history which could impact the forking/branching point (== base.sha), but github API continues to report the original base.sha for awhile - I think based on reports due to caching.

So this workaround going into user's branch and derives an up-to-date branching point from their branch - at a cost of needing to clone their forked repo.

@stas00 stas00 deleted the ci-doc-job-skip-take-4 branch December 9, 2020 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants