Does python setup.py nosetests invoke build_ext (or, more generally, build)? 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?
