On Thu, Nov 7, 2019 at 1:37 PM Chad Dombrova <chad...@gmail.com> wrote:

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

I believe the build step is executed as expected and during installation
results in cythonized package to be installed. This could be verified by,
in a new virtual environment creating a source distribution, installing
cython, then installing the source distribution. Resulting installation
does have the .so files. This is done before running nosetests.

> 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,

I believe the reason we are invokign nosetest this way is related to the
beam testing plugin. It is configure in setup.py. The behavior is
documented here: https://nose.readthedocs.io/en/latest/api/commands.html

> 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].

We do use extras is tox already. GCP tests work this way by installing
additional GCP package. In my opinion, letting tox to setup the virtual
environment either from the package or from setup.py is a better option
than using requirements file. Otherwise we would need a way to keep
setup.py and requirements file in sync. This is also doable.

> [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?

Reply via email to