On Thu, Nov 7, 2019 at 11:31 AM Robert Bradshaw <rober...@google.com> wrote:
> Does python setup.py nosetests invoke build_ext (or, more generally, > build)? It's unclear from the nose source[1] whether it's calling build_py and build_ext, or just build_ext. It's also unclear whether the result of that build is actually used. When python setup.py nosetests runs, it runs inside of a virtualenv created by tox, and tox has already installed beam into that venv. It seems unlikely to me that build_ext or build_py is going to install over top of the beam package installed by tox, but who knows, it may end up first on the path. Also recall that there is an sdist gradle task running before tox that creates a tarball that is passed to run_tox.sh which passes it along to tox --installpkg flag[2] so that tox won't build beam itself. We should designate a single place that always does the build. I thought that was supposed to be the gradle sdist task, but invoking nose via `python setup.py` means that we're introducing the possibility that a build is occurring which may or may not be used, depending on the entirely unclear dependencies of the setup commands, and the entirely unclear relationship of that build output to the tox venv. As a first step of clarity, we could stop invoking nose using `python setup.py nosetests`, and instead use `nosetests` (and in the future `pytest`). I think the reason for `python setup.py nosetest` is to ensure the test requirements are installed, but we could shift those to a separate requirements file or use extra_requires and tox can ensure that they're installed. I find these two options to be pretty common patterns [3]. [1] https://github.com/nose-devs/nose/blob/master/nose/commands.py#L113 [2] https://tox.readthedocs.io/en/latest/config.html#cmdoption-tox-installpkg [3] https://stackoverflow.com/questions/29870629/pip-install-test-dependencies-for-tox-from-setup-py > It's possible cython is present, but the build step is not > invoked which would explain the skip for slow_coders_test. The correct > test is being used in > > https://github.com/apache/beam/blob/master/sdks/python/apache_beam/coders/fast_coders_test.py#L34 > > On Thu, Nov 7, 2019 at 11:20 AM Ahmet Altay <al...@google.com> wrote: > > > > I believe tox is correctly installing cython and executes "python > setup.py nosetests" which triggers cythonzation path inside setup.py. Some > indications that cython is installed and used is the following log entries > (from a recent precommit cron job [1]) > > - [ 1/12] Cythonizing apache_beam/coders/coder_impl.py > > - Errors with cython.cast in the stack traces. > > - Tests skipped with: test_using_slow_impl > (apache_beam.coders.slow_coders_test.SlowCoders) ... SKIP: Found cython, > cannot test non-compiled implementation. > > > > At the same time there are log entries as following: > > - test_using_fast_impl (apache_beam.coders.fast_coders_test.FastCoders) > ... SKIP: Cython is not installed > > > > It might be an issue with what these tests are suing to check whether > they are cythonized or not. We seem to have at least 2 different versions > of this check [2][3]. Maybe we need to converge on one (former?). > > > > Ahmet > > > > [1] > https://builds.apache.org/view/A-D/view/Beam/view/All/job/beam_PreCommit_Python_Cron/2008/consoleFull > > [2] > https://github.com/apache/beam/blob/master/sdks/python/apache_beam/coders/slow_coders_test.py#L32 > > [3] > https://github.com/apache/beam/blob/master/sdks/python/apache_beam/tools/utils.py#L33 > > > > > > > > On Wed, Nov 6, 2019 at 6:19 PM Chad Dombrova <chad...@gmail.com> wrote: > >> > >> Another potential solution would be to _not_ use the sdist task to > build the tarball and let tox do it. Tox should install cython on > supported platforms before running sdist itself (which it does by default > unless you explicitly provide it with a tarball, which we are doing). This > has the added benefit of one less virtualenv. Right now we create a > virtualenv to build the sdist tarball, then we create another virtualenv to > run tox, then tox creates a virtualenv to run the task. It's unclear (to > me) whether the tarball is rebuilt for each tox task or if it's reused. > > > > > > I do not know if not passing the tarball will solve the issue. I tried > this and ran into the same problem. > > > > I agree that we can get rid of setup virtualenv task if it is not adding > value. > > > >> > >> > >> -chad > >> > >> > >> On Wed, Nov 6, 2019 at 6:13 PM Udi Meiri <eh...@google.com> wrote: > >>> > >>> I opened this bug today after commenting on Chad's type hints PR. > >>> https://issues.apache.org/jira/browse/BEAM-8572?filter=-1 > > > > > > Thank you for filing an issue. > > > >>> > >>> > >>> > >>> I am 95% sure that our Precommit tests are using tarballs that are > built without Cython (including the Cython tasks). > >>> > >>> I'm NOT currently working on fixing this. One solution might be to add > an additional task (sdistCython) and tell gradle that sdist and the new > task should not run concurrently. > >>> Does anyone want to take this up? >