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?).



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

> -chad
> On Wed, Nov 6, 2019 at 6:13 PM Udi Meiri <eh...@google.com> wrote:
>> I opened this bug today after commenting
>> <https://github.com/apache/beam/pull/9056#discussion_r343260172> 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?

Reply via email to