Simplify Python test process #10038

chadrik · 2019-11-08T05:01:40Z

Use pyproject.toml to install requirements for building beam
- Consolidate a bunch of requirements that were spread across gen_protos, gradle, and tox
- Remove the convoluted process in gen_protos where it tries to install deps, they're covered by pyproject.toml now
Use tox to build sdist of beam instead of gradle: one less virtualenv and fewer deps installed for no reason
Remove test_requires because they're already handled by extras
Don't call python setup.py nosetests because it rebuilds beam: we want this to be entirely controlled by our tox (our pep517-build system)
Stop running tests against code in the source directory! This undermines the sandboxing that tox provides and requires more work from gradle and tox cleanup scripts to try to resolve.

One thing to note here is that despite the progress made by peps 517 and 518 toward improving the python build toolchains, it's still not a turnkey solution. You can't just pip build sdist yet. I tried a dozen different things, but it looks like the best solution is this pep517 module. References:

Another problem is that we're always building cython on linux, rather than doing it conditionally based on the test we're running. Need to figure out if we can control that with environment markers

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
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.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza	Spark
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

chadrik · 2019-11-10T04:12:29Z

Run Python PreCommit

chadrik · 2019-11-11T20:04:43Z

Run Python2_PVR_Flink PreCommit

udim · 2019-11-25T17:37:50Z

sdks/python/scripts/run_pytest.sh

@@ -28,10 +28,10 @@ envname=${1?First argument required: suite base name}
 posargs=$2

 # Run with pytest-xdist and without.
-python setup.py pytest --addopts="-o junit_suite_name=${envname} \
+pytest --addopts="-o junit_suite_name=${envname} \


You should be able to remove the --addopts flag and leave the value part.

Ah yes, I was wrangling with that the other day. It would simplify things if we could just get rid of run_pytest.sh. Do we still need to run the tests with and without xdist?

Yes, there are a few tests that are still incompatible with xdist, such as those using save_main_session.

If the goal is to run both tests regardless of whether there is a failure (i.e. avoid early abort) then you can set the tox setting ignore_errors=true: https://tox.readthedocs.io/en/latest/config.html#conf-ignore_errors

If false, a non-zero exit code from one command will abort execution of commands for that environment. If true, a non-zero exit code from one command will be ignored and further commands will be executed. The overall status will be “commands failed”, i.e. tox will exit non-zero in case any command failed.

I was trying to get run_pytest.sh to work with tox -e py37-pytest -- path/to/test and I was struggling to get the arg passing correct, and ultimately I just moved it to tox.ini and it worked. With ignore_errors=true in place I don't think the shell script will be necessary.

I could try helping with arg passing.

If you specify a certain test to run, one of the pytest runs will certainly fail since it either has or doesn't have the no_xdist marker. The script special-cases the 5 exit status from pytest to handle this scenario.

I'm probably missing something, because the way you set things up tox -e py37-pytest -- path/to/test should have failed (either the -m not no_xdist or the -m no_xdist command should have failed).

In any case, I'm strongly in favor of simplifying the build process by having as few shell scripts as possible.

There's also this plugin to suppress certain exit statuses:
https://github.com/yashtodi94/pytest-custom_exit_code

chadrik · 2019-11-27T16:05:49Z

Run Python PreCommit

kennknowles · 2019-11-27T17:12:31Z

I know it shouldn't be relevant, but I noticed the Java flakes and will file something about them.

kennknowles · 2019-11-27T17:12:36Z

run java precommit

kennknowles · 2019-11-27T17:17:15Z

Both already had Jiras, so Java precommit is g2g.

udim · 2019-11-27T17:38:30Z

sdks/python/setup.py

- setup_requires=['pytest_runner'],
- tests_require= [
- REQUIRED_TEST_PACKAGES,
- INTERACTIVE_BEAM,


I would expect that removing INTERACTIVE_BEAM would cause some tests that depend on these package to fail. I'm guessing there aren't any tests for this yet.

Any tests that require the interactive deps should add the 'interactive' extra. So far I haven't run into any failures related to this.

chadrik · 2019-11-27T17:38:39Z

Thanks. Slowly chipping away at this. I'll bring this to the list when I have the remaining kinks worked out.

udim · 2019-11-27T17:39:49Z

sdks/python/tox.ini

@@ -307,3 +331,10 @@ commands =
 coverage report --skip-covered
 # Generate report in xml format
 coverage xml
+
+[testenv:py37-interactive]


Does this add any additional test coverage? I'd rather fold any 'interactive' tests into the py37 target if possible. Or at least not repeat all the py37 unit tests.

I think this addition is actually a rebase error on my part. These tests were not intentionally added by me. IIRC, they were added to tox on master then removed in a later commit, and I must've messed up on one of my rebases and left it in.

chadrik · 2019-11-27T18:49:23Z

My last two jenkins jobs indicated it took 0s to run, but the logs show that it was aborted after 2 hours. Any idea what could be going on?

udim · 2019-11-27T19:04:00Z

My last two jenkins jobs indicated it took 0s to run, but the logs show that it was aborted after 2 hours. Any idea what could be going on?

It looks like it hit the 2h Jenkins timeout. Should be extended to 3h once #10234 is merged and the next seed job runs. (I'll start one in #10234)

markflyhigh · 2019-11-27T21:01:49Z

sdks/python/setup.py

@@ -226,12 +307,6 @@ def run(self):
 ]),
 install_requires=REQUIRED_PACKAGES,
 python_requires=python_requires,
- test_suite='nose.collector',


What's alternative collector it will use? Can we ensure the collection result unchanged?

I believe it will still use nose, just not via setup.py. We can compare nosetests.xml files to verify.

Correct, test_requires and test_suite are for the python setup.py test and derivative workflows. That whole stack is now deprecated in favor of extras and pep517.

chadrik · 2019-11-28T01:15:16Z

Run Python PreCommit

chadrik · 2019-12-05T23:45:14Z

Run Python PreCommit

chadrik · 2019-12-10T17:25:09Z

Run Python PreCommit

- Use pyproject.toml and pep517 to install requirements for building beam - Use tox to build sdist of beam instead of gradle sdist - Don't call python setup.py nosetests because it rebuilds beam - Do not run tests on code in the source directory - Run lint jobs against the source

Tox knows how to do this too, and now that we're using pyproject.toml (pep517 & pep518) we can describe how to do this in a portable way.

WIP: trying to debug why these are failing

Also, fix a bug where cython coder tests were not actually being run fix fix Fixes

we have to set the pep517 isolated build directory as the site dir for the google .pth file to work and find the nested protobuf package

stale · 2020-03-17T05:52:35Z

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.

stale · 2020-03-24T06:25:20Z

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

chadrik force-pushed the simplify-tests branch 3 times, most recently from fb14488 to ac0b3d2 Compare November 9, 2019 21:38

chadrik force-pushed the simplify-tests branch 3 times, most recently from 6f5558b to 2bc357c Compare November 13, 2019 18:29

chadrik force-pushed the simplify-tests branch 2 times, most recently from 2128a40 to 491a190 Compare November 25, 2019 07:41

chadrik mentioned this pull request Nov 25, 2019

[BEAM-7746] Add python type hints (part 1) #9915

Merged

3 tasks

chadrik force-pushed the simplify-tests branch 2 times, most recently from 17ec0b1 to a9d4a72 Compare November 25, 2019 07:51

udim reviewed Nov 25, 2019

View reviewed changes

kennknowles changed the title ~~Simplify test process~~ Simplify Python test process Nov 27, 2019

udim reviewed Nov 27, 2019

View reviewed changes

markflyhigh reviewed Nov 27, 2019

View reviewed changes

chadrik force-pushed the simplify-tests branch from b1d0d2c to 862d733 Compare November 27, 2019 23:39

chadrik force-pushed the simplify-tests branch from 862d733 to 5347a29 Compare December 5, 2019 06:29

chadrik force-pushed the simplify-tests branch from 5347a29 to 863d2af Compare January 17, 2020 05:41

chadrik added 6 commits January 16, 2020 21:45

[BEAM-2366] Move generate_proto_files into setup.py

b5090ff

Stop using gradle to build sdist

eee9e4d

Tox knows how to do this too, and now that we're using pyproject.toml (pep517 & pep518) we can describe how to do this in a portable way.

Run flink PVR tests inside tox

80f7631

WIP: trying to debug why these are failing

Ensure that the code is cythonized when we expect it to be

80aaa37

Also, fix a bug where cython coder tests were not actually being run fix fix Fixes

Fix for the google namespace package on python2

ed06bf1

we have to set the pep517 isolated build directory as the site dir for the google .pth file to work and find the nested protobuf package

chadrik force-pushed the simplify-tests branch from 863d2af to ed06bf1 Compare January 17, 2020 05:45

stale bot added the stale label Mar 17, 2020

stale bot closed this Mar 24, 2020

damccorm mentioned this pull request Jun 4, 2022

tox: use isolated builds (PEP 517 and 518) #20051

Closed

AnandInguva mentioned this pull request Apr 13, 2023

[Feature Request]: Upgrade setup.py to pyproject.toml #26266

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify Python test process #10038

Simplify Python test process #10038

chadrik commented Nov 8, 2019 •

edited

Loading

chadrik commented Nov 10, 2019

chadrik commented Nov 11, 2019

udim Nov 25, 2019

chadrik Nov 25, 2019

udim Nov 25, 2019

chadrik Nov 26, 2019

udim Nov 26, 2019

chadrik commented Nov 27, 2019

kennknowles commented Nov 27, 2019

kennknowles commented Nov 27, 2019

kennknowles commented Nov 27, 2019

udim Nov 27, 2019

chadrik Nov 27, 2019

chadrik commented Nov 27, 2019

udim Nov 27, 2019

chadrik Nov 27, 2019

chadrik commented Nov 27, 2019

udim commented Nov 27, 2019

markflyhigh Nov 27, 2019

udim Nov 27, 2019

chadrik Nov 27, 2019

chadrik commented Nov 28, 2019

chadrik commented Dec 5, 2019

chadrik commented Dec 10, 2019

stale bot commented Mar 17, 2020

stale bot commented Mar 24, 2020

Simplify Python test process #10038

Simplify Python test process #10038

Conversation

chadrik commented Nov 8, 2019 • edited Loading

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

chadrik commented Nov 10, 2019

chadrik commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadrik commented Nov 27, 2019

kennknowles commented Nov 27, 2019

kennknowles commented Nov 27, 2019

kennknowles commented Nov 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadrik commented Nov 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadrik commented Nov 27, 2019

udim commented Nov 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadrik commented Nov 28, 2019

chadrik commented Dec 5, 2019

chadrik commented Dec 10, 2019

stale bot commented Mar 17, 2020

stale bot commented Mar 24, 2020

chadrik commented Nov 8, 2019 •

edited

Loading