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

Fix Python PreCommit dependency compatibility test not run or redundant setup #25115

Merged
merged 2 commits into from
Jan 23, 2023

Conversation

Abacn
Copy link
Contributor

@Abacn Abacn commented Jan 23, 2023

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.

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)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI.

…nt setup

* Move pyarrow and pandas matrix unit test to preCommitPyCoverage

* Fix Dataframe compatibility test not run

* Update readme
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #25115 (9fc7ef0) into master (cd20288) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #25115      +/-   ##
==========================================
- Coverage   73.14%   73.13%   -0.01%     
==========================================
  Files         735      735              
  Lines       98161    98161              
==========================================
- Hits        71796    71789       -7     
- Misses      25002    25009       +7     
  Partials     1363     1363              
Flag Coverage Δ
python 82.68% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../python/apache_beam/transforms/periodicsequence.py 97.01% <0.00%> (-1.50%) ⬇️
...eam/runners/portability/fn_api_runner/execution.py 92.49% <0.00%> (-0.64%) ⬇️
sdks/python/apache_beam/runners/direct/executor.py 96.46% <0.00%> (-0.55%) ⬇️
...ks/python/apache_beam/runners/worker/sdk_worker.py 89.08% <0.00%> (-0.33%) ⬇️
...hon/apache_beam/runners/worker/bundle_processor.py 93.54% <0.00%> (-0.13%) ⬇️
...on/apache_beam/runners/dataflow/dataflow_runner.py 81.88% <0.00%> (+0.14%) ⬆️
...hon/apache_beam/runners/direct/test_stream_impl.py 94.02% <0.00%> (+0.74%) ⬆️
sdks/python/apache_beam/internal/metrics/metric.py 94.00% <0.00%> (+1.00%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Abacn Abacn marked this pull request as ready for review January 23, 2023 02:35
@Abacn
Copy link
Contributor Author

Abacn commented Jan 23, 2023

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

@github-actions
Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control


// 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, moving dataframe compatibility tests into preCommitPyCoverage

@Abacn
Copy link
Contributor Author

Abacn commented Jan 23, 2023

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

@Abacn
Copy link
Contributor Author

Abacn commented Jan 23, 2023

https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Commit/14639/ actually succeeded. merge for now.

@Abacn Abacn merged commit b3aa2e8 into apache:master Jan 23, 2023
@Abacn Abacn deleted the adjustpyprecommitmatrix branch January 23, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants