-
Notifications
You must be signed in to change notification settings - Fork 4.5k
GA Migration Python PreCommit Sharding #23451
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
Conversation
f787160 to
b336558
Compare
|
R: @damccorm |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
b336558 to
2bed71b
Compare
| - 'sdks/python/apache_beam/runners/portability/local_job_service_test.py' | ||
| - 'sdks/python/apache_beam/runners/portability/portable_runner_test.py' | ||
| - 'sdks/python/apache_beam/runners/portability/sdk_container_builder_test.py' | ||
| - 'sdks/python/apache_beam/io/hadoopfilesystem_test.py' |
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.
Could you please sort these paths alphabetically? It will make it much easier to read/edit
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.
Done 👍
| uses: ./.github/actions/gradle-command-self-hosted-action | ||
| with: | ||
| gradle-command: :sdks:python:test-suites:tox:py${{env.PYTHON_VERSION}}:testPy${{env.PYTHON_VERSION}}${{matrix.tox-env}} | ||
| arguments: "-Pposargs=apache_beam/" |
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.
I was revisiting this and realized I don't think its actually working the way we expect it to. A couple notes:
- This runs the tests for all of the apache_beam directory. This means that subsequent steps are just trying to retest things tested here. Since that gets cached, subsequent steps will do nothing. See https://github.com/roger-mike/beam/actions/runs/3161439176/jobs/5147285589 which takes 10 minutes for this step and 10 seconds for each subsequent step.
- As best I can tell
gradlew :sdks:python:test-suites:tox:py${{env.PYTHON_VERSION}}:testPy${{env.PYTHON_VERSION}}${{matrix.tox-env}} <path/to/tests>doesn't actually limit runs to tests in that directory. So I don't think this approach is generally valid. - (least important, but still matters) Sharding is most helpful when things are split into different jobs, not just different steps. This kind of sharding doesn't really buy us much. Sharding into jobs would also make some of these problems more obvious.
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.
Got it, thanks for the feedback Danny. I just have a question, do you think is a good idea to split this tests into different workflows for each module? For example, a workflow for each one of these:
apache_beam/
apache_beam/coders
apache_beam/internal
apache_beam/io
apache_beam/metrics
apache_beam/options
apache_beam/runners
...etc.
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.
Yeah, I think that's a good idea
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.
As best I can tell gradlew :sdks:python:test-suites:tox:py${{env.PYTHON_VERSION}}:testPy${{env.PYTHON_VERSION}}${{matrix.tox-env}} <path/to/tests> doesn't actually limit runs to tests in that directory. So I don't think this approach is generally valid.
I still don't think the current approach actually works in limiting the tests being run - I've been doing something similar for the Jenkins tests here and had to add some custom logic (which still isn't quite working) - #24204
1de3954 to
0315dec
Compare
0315dec to
48c4386
Compare
Codecov Report
@@ Coverage Diff @@
## master #23451 +/- ##
==========================================
+ Coverage 73.46% 74.63% +1.16%
==========================================
Files 709 714 +5
Lines 95983 103589 +7606
==========================================
+ Hits 70514 77311 +6797
- Misses 24152 24961 +809
Partials 1317 1317
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
e0c33c9 to
430bcd7
Compare
852f193 to
79195a5
Compare
aa5515c to
722b2dd
Compare
722b2dd to
5a3c7c5
Compare
| branches: [ 'master', 'release-*'] | ||
| tags: 'v*' | ||
| paths: | ||
| - 'sdks/python/apache_beam/coders/**' |
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.
By default, lets run all of these workflows on sdks/python/apache_beam. Some folders have dependencies on other folders and we want to make sure we catch issues with tests
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions. |
As part of the migration of Precommit and Postcommit Jobs from Jenkins to GA in self-hosted runners, this PR contains:
The migrated workflows were added to CI.md
DO NOT MERGE until the effort to use self-hosted runners is completed #22703
These workflows use base actions listed in #23109 (Dependency)
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.