-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-35048][INFRA] Distribute GitHub Actions workflows to fork repositories to share the resources #32092
Conversation
cc @dongjoon-hyun, @MaxGekk, @maropu, @srowen, @gengliangwang FYI cc @potiuk too FYI who might be interested in this approach. |
Kubernetes integration test starting |
Kubernetes integration test status failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, seems like a good way to reduce load if it works!
Test build #137066 has finished for PR 32092 at commit
|
I was surprised that there is such a solution for the resource-shortage issue, nice. |
@HyukjinKwon Thanks for the great effort in testing infra! |
It's great. Thank you, @HyukjinKwon ! |
This is an excellent idea ! I ❤️ it. Are all the pieces already working :) ? |
One thing that is missing is how to get back the information about the status of the build in the forked repository. I do not think with the permissions model of GH it is possible to do directly, but I can think of another 'scheduled' workflow that could run in the original repository that could read status from all the PRs that had "forked" builds running. That would be quite possible. We could even try to utilize "GitHub Check" for that https://docs.github.com/en/rest/reference/checks - I am already using it in Airflow to notify status betwen workflows (I have a 'build image` workflow that notifies the PR that runs a CI workflow this way. This looks like the regular "yellow/green/red" build status indicator, and it impacts the "mergable" status of the PR. This way we could have a complete solution, when the PR would become "green" when the forked build succeeds. |
I think it needs a bit complex logic, but I could possibly write a custom GitHub Action for that similar to what I've done with https://github.com/potiuk/cancel-workflow-runs |
Thanks @potiuk. Yeah, I think it makes sense to do that seamlessly with updating the status checks too. Borrowing the idea from cancel workflow is a good idea. If we're going this way, I will take a separate look for that. FWIW, I do think this is still a workaround that disables GitHub Actions to work out of the box. In the mailing list, the permanent solution is being discussed, which I think makes sense and reasonable. |
Agree it's a nasty hack and should be out-of-the-box built in GitHub. |
I did not have time to take a look yet, and during the week I have a bit limited time but in a week or so I hope to have some POC with the checks in place. |
Definitely seems like a possible way to distribute the load and cost. Are there limitations on the personal repo GitHub actions?
How many action minutes would an average spark pr take? the test build took 1 hours 27 minutes duration but is that action minutes (https://github.com/MaxGekk/spark/actions/runs/728916852) or do you have to add up each of the individual build modules, java 11, etc? Similar with storage are log files still available, how long or is there any others differences to deal with? just wondering some of these because say a user runs out of action minutes or there are problems, then what do we do? |
@tgravescs : Github Actions and storage are FREE for public repositories. If you look for the smaller footprint in the pricing page (bold is mine):
There are some limitations when it comes to number of runners available (so if you have more builds they will queue in your personal queue) |
6dabe60
to
74a7674
Compare
This is ready for a review. I will take a look for #32092 (comment) separately. |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #137286 has finished for PR 32092 at commit
|
Thanks @dongjoon-hyun! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! LGTM
Thanks @gengliangwang! (I just fixed a indentation nit) |
@@ -656,16 +656,10 @@ def main(): | |||
# If we're running the tests in GitHub Actions, attempt to detect and test | |||
# only the affected modules. | |||
if test_env == "github_actions": | |||
if os.environ["GITHUB_INPUT_BRANCH"] != "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maropu FYI. I think we should update https://spark.apache.org/developer-tools.html because now we always run the tests on each commit in each branch in forked repositories.. I will take a look and fix it soon.
I am merging it into master. It's a daytime in my time .. so would likely be able to track it if anything goes wrong. |
Currently contribution guide does not reflect actual flow to raise a new PR and hence it's not clear (for a new contributors) what exactly needs to be done to make a PR for Spark repository and test it as per expectation. This PR addresses that by following: - It describes in the Pull request section of the Contributing page the actual procedure and takes a contributor through a step by step process. - It removes optional "Running tests in your forked repository" section on Developer Tools page which is obsolete now and doesn't reflect reality anymore i.e. it says we can test by clicking “Run workflow” button which is not available anymore as workflow does not use "workflow_dispatch" event trigger anymore and was removed in apache/spark#32092 - Instead it documents the new procedure that above PR introduced i.e. contributors needs to use their own GitHub free workflow credits to test new changes they are purposing and a Spark Actions workflow will expect that to be completed before marking PR to be ready for a review. - Some general wording was copied from "Running tests in your forked repository" section on Developer Tools page but main content was rewritten to meet objective - Also fixed URL to developer-tools.html to be resolved by parser (that converted it into relative URI) instead of using hard coded absolute URL. Tested imperically with `bundle exec jekyll serve` and static files were generated with `bundle exec jekyll build` commands This closes https://issues.apache.org/jira/browse/SPARK-37996 Author: khalidmammadov <xmamedov@hotmail.com> Closes #378 from khalidmammadov/fix_contribution_workflow_guide.
What changes were proposed in this pull request?
This PR proposes to leverage the GitHub Actions resources from the forked repositories instead of using the resources in ASF organisation at GitHub.
This is how it works:
build_and_test.yml
) triggers a build on any commit on any branch (exceptbranch-*.*
), which roughly means:master
branchmaster
branch locally, and merge the branch from the forked repository into the original repository'smaster
branch locally.Therefore, the tests in the forked repository will run after being sync'ed with the original repository's
master
branch.In short, please see this example HyukjinKwon#34
NOTE that we will still run the tests in the original repository for each commit pushed to
master
branch. This distributes the workflows only in PRs.Why are the changes needed?
ASF shares the resources across all the ASF projects, which makes the development slow down.
Please see also:
By distributing the workflows to use author's resources, we can get around this issue.
Does this PR introduce any user-facing change?
No, this is a dev-only change.
How was this patch tested?
Manually tested at HyukjinKwon#34 and HyukjinKwon#33.