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

Use wheels for Dataflow postcommit tests #25970

Merged
merged 8 commits into from
Apr 6, 2023
Merged

Conversation

Abacn
Copy link
Contributor

@Abacn Abacn commented Mar 24, 2023

Fixes #25966
Fixes #26049

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.

@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #25970 (d41b7c6) into master (87db1ae) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #25970      +/-   ##
==========================================
- Coverage   71.17%   71.17%   -0.01%     
==========================================
  Files         787      787              
  Lines      103294   103311      +17     
==========================================
+ Hits        73523    73532       +9     
- Misses      28283    28291       +8     
  Partials     1488     1488              
Flag Coverage Δ
python 79.85% <ø> (-0.01%) ⬇️

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

see 8 files with indirect coverage changes

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

@Abacn Abacn force-pushed the usewheel branch 2 times, most recently from 53f64a9 to e50b97e Compare March 24, 2023 22:10
@github-actions github-actions bot removed the docker label Mar 24, 2023
@Abacn Abacn force-pushed the usewheel branch 7 times, most recently from 008170e to 2a4e5a3 Compare March 25, 2023 02:16
@Abacn Abacn force-pushed the usewheel branch 2 times, most recently from fff0bf3 to e783586 Compare March 25, 2023 04:25
@Abacn
Copy link
Contributor Author

Abacn commented Mar 25, 2023

Tests on dataflow actually passed and indeed takes much faster: https://ci-beam.apache.org/job/beam_PostCommit_Python310_PR/38/

However, flink runner test not running due to [Errno 2] No such file or directory: 'apache-beam-2.47.0.dev0/apache_beam/... this is a racing condition that sdist generates a temporary directory during packaging and delete it when finishing.

-- this is due to pypa/setuptools#1222

@Abacn
Copy link
Contributor Author

Abacn commented Mar 25, 2023

Run Python 3.10 PostCommit

Update: running time from ~3h10min (cron job) reduced to 2.5 h (this PR)

@Abacn
Copy link
Contributor Author

Abacn commented Mar 27, 2023

Run Python 3.7 PostCommit

@Abacn
Copy link
Contributor Author

Abacn commented Mar 27, 2023

Run Python 3.8 PostCommit

@Abacn
Copy link
Contributor Author

Abacn commented Mar 28, 2023

Run Python 3.9 PostCommit

@Abacn
Copy link
Contributor Author

Abacn commented Mar 28, 2023

@Abacn
Copy link
Contributor Author

Abacn commented Mar 31, 2023

Run Python 3.8 PostCommit

@Abacn
Copy link
Contributor Author

Abacn commented Mar 31, 2023

Run Python 3.10 PostCommit

@Abacn
Copy link
Contributor Author

Abacn commented Mar 31, 2023

Thanks @tvalentyn for the suggestion. I have rewritten the part of build wheel. Summary of change since last review:

  • added bdistPy{version}{platfoem} tasks in :sdks:python gradle project to build wheels.
  • created a 'initializeForDataflowJob' task in test-suites/dataflow/common.gradle handling sdk distribution setup.
  • reverted changes to BeamModulePlugin.

PostCommit succeeded. Ready for review again.

flink_versions=1.12,1.13,1.14,1.15,1.16
# supported python versions
python_versions=3.7,3.8,3.9,3.10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a gradle property here because now :sdks:python project need it to define bdist tasks. This also simplifies gradle.properties in python/test-suites a little bit because now it has a single source of truth for supported python version

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use this one instead?

final static List<String> ALL_SUPPORTED_VERSIONS = [

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 for pointing to this. Not aware of it. This code path is for Jenkins only (.test-infra/jenkins/); gradle files works independently with Jenkins (gradle tasks can trigger locally). To reduce to duplicate declaration, refer to gradle.properties in Jenkins script should work

Copy link
Contributor

@tvalentyn tvalentyn Apr 4, 2023

Choose a reason for hiding this comment

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

there is also https://github.com/apache/beam/blob/master/sdks/python/test-suites/gradle.properties Would be good to see if we can these version configurations together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is sdks/python/test-suites/gradle.properties is not accessible to sdks/python gradle project. I understand test-suites/gradle.properties as a configuration to manage which python version used in different test. While for bdist it belongs to the python root project (just as sdist). I did not find a source of truth for supported Python versions accessible in the sdks/python context.

Copy link
Contributor

Choose a reason for hiding this comment

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

ack, thanks for checking. @AnandInguva FYI one more place to add python versions in the future.

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, will experiment to read gradle.properties in jenkins/github actions so reduce the duplicate declarations of py version

environment CIBW_BUILD: "cp${pyversion}-${idsuffix}"
environment CIBW_ENVIRONMENT: "SETUPTOOLS_USE_DISTUTILS=stdlib"
environment CIBW_BEFORE_BUILD: "pip install cython numpy && pip install --upgrade setuptools"
// note: sync cibuildwheel version with GitHub Action build_wheel
Copy link
Contributor

Choose a reason for hiding this comment

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

lets add a pointer to exact place where this needs to be modified and maybe make it a inline comment, and do the same in the counterpart where wheels are built.

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, added to both build.gradle and .yml

@Abacn
Copy link
Contributor Author

Abacn commented Apr 5, 2023

Doing a rebase, will squash the reviewed commit (that is before 8329ee5)

@tvalentyn
Copy link
Contributor

Reminder about code freeze: https://lists.apache.org/thread/rpmq9r9tbgrry8yco70mzyt4psyk5m20

@tvalentyn
Copy link
Contributor

Thank you very much, @Abacn !

@Abacn
Copy link
Contributor Author

Abacn commented Apr 6, 2023

Run Python 3.7 PostCommit

@Abacn
Copy link
Contributor Author

Abacn commented Apr 6, 2023

Run Python 3.11 PostCommit

@Abacn
Copy link
Contributor Author

Abacn commented Apr 6, 2023

Jenkins not triggering. The latest commit just switched to opt-in buildWheels from opt-out (convenient for testing without the need of triggering seed job). Merging based on the signal in 697635a.

at there Python postcommit passed. "pending" tests were actually passed; failed Java tests irrelevant.

@Abacn
Copy link
Contributor Author

Abacn commented Apr 6, 2023

There are 3 stuck Java test and 1 failed java tests unrelated. Other precommits passed. Merging for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants