Sorry I didn't realize you already had a solution for the shadowing issue and BEAM-8572.
On Tue, Dec 10, 2019 at 6:21 PM Chad Dombrova <chad...@gmail.com> wrote: > Hi Udi, I know you're aware of my PR > <https://github.com/apache/beam/pull/10038>, but I really encourage you > to look into pep517 and pep518. They are the new solution for all of this > -- declaring build dependencies and creating isolated out-of-source builds > e.g. using tox. Another thing I added to my PR is something which asserts > that cython is *or is not* enabled based on an env var set in the tox > config. In other words, we're currently skipping tests when cython is not > installed or when code is not cythonized, but we're not asserting whether > we *expect* that code to be cythonized or not. Also there are a few bugs > where cython tests don't run at all because we're identifying the wrong > module name to check for cythonization ("apache_beam.coders" which is a > package). > > -chad > > > On Tue, Dec 10, 2019 at 6:03 PM Udi Meiri <eh...@google.com> wrote: > >> To follow up, since I'm trying to run cython-based tests using pytest: >> - tox does in fact correctly install apache-beam with cythonized modules >> in its virtualenv. >> - Since our tests are under apache_beam/, local sources shadow those in >> the installed apache_beam package. >> - The original issue I raised (BEAM-8572 >> <https://issues.apache.org/jira/browse/BEAM-8572>) was due to bad usage >> of utils.check_compiled(). >> >> So I'm going to add an additional "python setup.py build_ext --inplace" >> to pyXX-cython-pytest envs. >> If we want to remove this additional step we'll probably have to move >> tests under a separate directory that is not part of the apache_beam >> package. >> >> See also: >> https://github.com/tox-dev/tox/issues/514 >> https://blog.ionelmc.ro/2014/05/25/python-packaging/#the-structure >> >> On Mon, Nov 11, 2019 at 3:21 PM Ahmet Altay <al...@google.com> wrote: >> >>> Thank you for spending time on this to clarify it for all of us! Much >>> appreciated. >>> >>> On Sun, Nov 10, 2019 at 3:45 PM Chad Dombrova <chad...@gmail.com> wrote: >>> >>>> Hi all, >>>> >>>> >>>>> The sdist step creates a package that should be installed into each >>>>> tox environment. If the tox environment has cython when this apache >>>>> beam package is installed, it should be used. Nose (or whatever) >>>>> should then run the tests. >>>>> >>>> I spent some time this weekend trying to understand the Beam python >>>> build process, and here’s an overview of what I’ve learned: >>>> >>>> - the :sdks:python:sdist gradle task creates the source tarball (no >>>> surprises there) >>>> - the protobuf stubs are generated during this process >>>> - the sdist is provided to tox, which installs it into the the >>>> virtualenv for that task >>>> - for *-cython tasks, tox installs the cython dep and, as Ahmet >>>> asserted, python setup.py nosetests performs the cythonization. >>>> - this cythonized build overrides the one installed by tox >>>> >>>> Here’s what I learned about the current status of tests wrt cython: >>>> >>>> - cython tox tasks *are* using cython (good!) >>>> - non-cython tox tasks *are not* using cython (good!) >>>> - none of the GCP or integration tests are using cython (bad?) >>>> >>>> This is intentional with the recent change to drop base only tests. >>> Otherwise we would not have coverage for non-cythonized tests. >>> >>>> >>>> - This is because the build is only cythonized when python setup.py >>>> nosetests is used in conjunction with tox (tox installs cython, >>>> python >>>> setup.py nosetests compiles it). >>>> - GCP tests don't install cython. ITs don't use tox. >>>> >>>> To confirm my understanding of this, I created a PR [1] to assert that >>>> a cythonized or pure-python build is being used. A cythonized build is >>>> expected by default on linux systems unless a special flag is provided to >>>> inform the test otherwise. It appears as though the portable tests passed >>>> (i.e. used cython), but I forgot to add the assertion for those; like the >>>> other ITs they are not using cython. >>>> >>>> *Questions:* >>>> >>>> - Is the lack of cython for ITs expected and/or desired? >>>> - Why aren't ITs using tox? It's quite possible to pass arguments >>>> into tox to control it's behavior. For example, it seems reasonable >>>> that >>>> run_integration_test.sh could be inside tox >>>> >>>> >>> For ITs the primary execution will happen on a remote system. As long as >>> those remote workers have cython installed the tests will run in a >>> cythonized environment. This is true for any IT tests, that runs in a >>> container based on the Dockerfile we have in beam (which installs cython), >>> and also true for legacy Dataflow environments that are not yet using this >>> Beam provided Dockerfile. >>> >>> >>>> >>>> *Next Steps:*There has been some movement in the python community to >>>> solve problems around build dependencies [2] and toolchains [3]. I hope to >>>> have a proposal for how to simplify this process soon. >>>> >>> Thank you! >>> >>> [1] https://github.com/apache/beam/pull/10058 >>>> [2] https://www.python.org/dev/peps/pep-0517/ >>>> [3] https://www.python.org/dev/peps/pep-0518/ >>>> >>>> -chad >>>> >>>
smime.p7s
Description: S/MIME Cryptographic Signature
