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?

Reply via email to