You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "Abacn (via GitHub)" <gi...@apache.org> on 2023/01/23 02:09:52 UTC
[GitHub] [beam] Abacn opened a new pull request, #25115: Fix Python PreCommit dependency compatibility test not run or redundant setup
Abacn opened a new pull request, #25115:
URL: https://github.com/apache/beam/pull/25115
Followup of #24866
It is found that dataframe compatibility matrix tests are actually not exercised after splitting due to a typo.
Also, sharded precommits set up full version of pyarrow and pytorch compatibility test tasks, but most of them do not have test run. Observed that most of these tests costs >20 min run, where about 10 minutes are contributed by the tox setup for these compability test tasks. This is inefficient. Moved these tasks to be exercised by beam_PreCommit_Python_Coverage which deals with precommit tests exercised by single py version (currently is Py38).
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `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, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on a diff in pull request #25115: Fix Python PreCommit dependency compatibility test not run or redundant setup
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #25115:
URL: https://github.com/apache/beam/pull/25115#discussion_r1084158880
##########
sdks/python/test-suites/tox/py38/build.gradle:
##########
@@ -36,52 +36,58 @@ testPy38Cython.mustRunAfter testPython38, testPy38CloudCoverage
// TODO(BEAM-12000): Move tasks that aren't specific to 3.8 to Py 3.9.
def posargs = project.findProperty("posargs") ?: ""
-def runDataframesTests = (posargs == "" || posargs.contains("dataframes"))
-logger.info('Running dataframes tests: ' + runDataframesTests)
+def runDataframeTests = (posargs == "" || posargs.contains("dataframe"))
+logger.info('Running dataframes tests: ' + runDataframeTests)
+
+// PyCoverage Precommit runs test suites that evaluate test coverage and compatibility of
+// particular dependencies. It is exercised on a single Python version.
+project.tasks.register("preCommitPyCoverage") {
+ dependsOn = ["testPy38CloudCoverage"]
+}
// Create a test task for each major version of pyarrow
toxTask "testPy38pyarrow-0", "py38-pyarrow-0", "${posargs}"
test.dependsOn "testPy38pyarrow-0"
-preCommitPy38.dependsOn "testPy38pyarrow-0"
+preCommitPyCoverage.dependsOn "testPy38pyarrow-0"
Review Comment:
thanks, moving dataframe compatibility tests into preCommitPyCoverage
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #25115: Fix Python PreCommit dependency compatibility test not run or redundant setup
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #25115:
URL: https://github.com/apache/beam/pull/25115#issuecomment-1399710162
Python_Dataframes precommit now has Task like `:sdks:python:test-suites:tox:py38:testPy38pandas-15` running: https://ci-beam.apache.org/job/beam_PreCommit_Python_Dataframes_Commit/225/console
Python_Examples precommit now costs 14 min compared to ~25 min before: https://ci-beam.apache.org/job/beam_PreCommit_Python_Examples_Commit/275/ other jobs also costs ~10 min shorter
Python_Coverage now runs pyarrow and pytorch compatibility matrix tests.
Ready for review now. R: @damccorm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #25115: Fix Python PreCommit dependency compatibility test not run or redundant setup
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #25115:
URL: https://github.com/apache/beam/pull/25115#issuecomment-1400613434
Python_Coverage now becomes slowest task and can costs > 1h when jenkins nodes are full. Not ideal. The dependency tasks can actually run in parallel
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #25115: Fix Python PreCommit dependency compatibility test not run or redundant setup
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #25115:
URL: https://github.com/apache/beam/pull/25115#discussion_r1084095718
##########
sdks/python/test-suites/tox/py38/build.gradle:
##########
@@ -36,52 +36,58 @@ testPy38Cython.mustRunAfter testPython38, testPy38CloudCoverage
// TODO(BEAM-12000): Move tasks that aren't specific to 3.8 to Py 3.9.
def posargs = project.findProperty("posargs") ?: ""
-def runDataframesTests = (posargs == "" || posargs.contains("dataframes"))
-logger.info('Running dataframes tests: ' + runDataframesTests)
+def runDataframeTests = (posargs == "" || posargs.contains("dataframe"))
+logger.info('Running dataframes tests: ' + runDataframeTests)
+
+// PyCoverage Precommit runs test suites that evaluate test coverage and compatibility of
+// particular dependencies. It is exercised on a single Python version.
+project.tasks.register("preCommitPyCoverage") {
+ dependsOn = ["testPy38CloudCoverage"]
+}
// Create a test task for each major version of pyarrow
toxTask "testPy38pyarrow-0", "py38-pyarrow-0", "${posargs}"
test.dependsOn "testPy38pyarrow-0"
-preCommitPy38.dependsOn "testPy38pyarrow-0"
+preCommitPyCoverage.dependsOn "testPy38pyarrow-0"
Review Comment:
This is the right move for these tests, we should probably actually do the same thing for the dataframes tests so that they get counted towards codecoverage.
I'm approving this PR, feel free to merge as is or make that change. If you merge as is, I'll put up a follow up PR to move the dataframes tests into the coverage job.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #25115: Fix Python PreCommit dependency compatibility test not run or redundant setup
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #25115:
URL: https://github.com/apache/beam/pull/25115#issuecomment-1400693446
https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Commit/14639/ actually succeeded. merge for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn merged pull request #25115: Fix Python PreCommit dependency compatibility test not run or redundant setup
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn merged PR #25115:
URL: https://github.com/apache/beam/pull/25115
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org