Move dependency tests from PreCommit Coverage to PostCommit #30704
Conversation
Abacn
force-pushed
the
splitcoverage
branch
2 times, most recently
from
c3851ab to
9a7a1d8
Compare
|
btw code coverage report seems broken for a while: |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @liferoad for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
checked that these marked tests also run on Python PreCommit Coverage and the specific Precommits (e.g. Dataframe) |
|
|
||
| toxTask "testPy38pyarrow-8", "py38-pyarrow-8", "${posargs}" | ||
| test.dependsOn "testPy38pyarrow-8" | ||
| preCommitPyCoverage.dependsOn "testPy38pyarrow-8" | ||
| postCommitPyDep.dependsOn "testPy38pyarrow-8" |
There was a problem hiding this comment.
Each of these categories are not strictly dependency tests, they also test some other functionality which is not necessarily tested elsewhere. I agree we don't need the full matrix of all versions in our precommits, but I think it would be good to keep at least 1 version per type that also runs in the precommit. So for example, we could change this line to 2 lines:
preCommitPyCoverage.dependsOn "testPy38pyarrow-8"
postCommitPyDep.dependsOn "testPy38pyarrow-8"
but leave the rest of the pyarrow versions for the dependency task
There was a problem hiding this comment.
I thought these tests are also running on usual precommit, with dependency version unrestricted (see comment above). Let me open a draft PR to double check
There was a problem hiding this comment.
From test result of #30226 Only pyarrow and pandas tests were executed in other PreCommit (PreCommit dataframe), so these two tests can be fully moved to postcommit; other tests leave at least one version in PreCommit coverage
|
Still takes 2h 0min. The very slow part is to install certain dependency: took 30 min to install sentence-transformers; 20 min to install torch and mpmath; another 10 min to install tensorflow and torch |
|
sentence-transformers has several very large dependencies:
|
…0704) * Move dependency tests from PreCommit Coverage to PostCommit * Move at least one version to precommit
| fail-fast: false | ||
| matrix: | ||
| job_name: [beam_PostCommit_Python_Dependency] | ||
| job_phrase: [Run Python PostCommit Dependency] |
There was a problem hiding this comment.
Thanks for making this change. Do we still need to explicitly mention new suites anywhere under release/src/main/scripts or this is no longer necessary?
There was a problem hiding this comment.
It's not necessary. job_phrase for postcommit currently is not effective (#28909). I just copy-paste from existing yml which had this structure.
Please add a meaningful description for your change here
PreCommit Python Coverage costs ~2h30min to run outstandingly. Splitting the dependency compatibility unit tests from this test suite and put into a new PostCommit
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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 or the workflows README to see a list of phrases to trigger workflows.