Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cython does not respect dependency for generating .pyx.c file #8961

Closed
rgommers opened this issue Jul 4, 2021 · 17 comments
Closed

Cython does not respect dependency for generating .pyx.c file #8961

rgommers opened this issue Jul 4, 2021 · 17 comments

Comments

@rgommers
Copy link
Contributor

rgommers commented Jul 4, 2021

Describe the bug

Building a Python extension from a generated Cython file (_ufuncs.pyx) fails with:

[1/539] Compiling Cython source scipy/special/_ufuncs.pyx
FAILED: scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/scipy/special/_ufuncs.pyx.c 
cython --fast-fail -3 -o scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/scipy/special/_ufuncs.pyx.c scipy/special/_ufuncs.pyx

Error compiling Cython file:
------------------------------------------------------------
...
# This file is automatically generated by _generate_pyx.py.
# Do not edit manually!

include "_ufuncs_extra_code_common.pxi"
^
------------------------------------------------------------

The relevant meson.build content is:

# Generates the `_ufuncs.pyx` file that fails to build later
_generate_cy = custom_target('_generate_cy',
  output : ['_ufuncs.pyx',
            '_ufuncs_defs.h',
            '_ufuncs_cxx.pyx',
            '_ufuncs_cxx.pxd',
            '_ufuncs_cxx_defs.h',
            '_ufuncs.pyi',
            'cython_special.pyx',
            'cython_special.pxd'
           ],
  input : ['_generate_pyx.py', 'functions.json', 'add_newdocs.py'],
  command : [py3, '@INPUT0@', '-o', '@OUTDIR@'],
)

# Simply copy over the files we need to the build dir
_ufuncs_pxi_pxd_sources = custom_target('_ufuncs_pxi_pxd',
  output : [
    '__init__.py',
    ...
    '_ufuncs_extra_code.pxi',
    '_ufuncs_extra_code_common.pxi',
  ],
  input : [
    '__init__.py',
    ...
    '_ufuncs_extra_code.pxi',
    '_ufuncs_extra_code_common.pxi',
  ],
  command : ['cp', '@INPUT@', '-t', '@OUTDIR@'],
)

# FIXME: the generated ninja.build file is missing the dependency between
#       copying these files into the build dir and the `.pyx -> .c` stage.
#       For the `.c -> .so` stage the dependency (see `||` in the `build.ninja`
#       file) is picked up correctly. This only shows up when running the build
#       with `-j 1` (when the steps are run in parallel, the copying of sources
#       seems to always happen quickly enough. This will likely only be fixed
#       by the "structured sources" PR on the Meson repo.
_ufuncs_pxi_pxd_dep = declare_dependency(sources: _ufuncs_pxi_pxd_sources)

py3.extension_module('_ufuncs',
  [ufuncs_sources, _generate_cy[0], _dummy_init],
  c_args : [numpy_nodepr_api, use_math_defines],
  include_directories: [inc_np, '../_lib', '../_build_utils/src'],
  dependencies : [py3_dep, lapack, npymath_lib, _ufuncs_pxi_pxd_dep],  # We have the right dependency here
  link_with: [
    amos_lib,
    cdflib_lib,
    cephes_lib,
    mach_lib,
    specfun_lib,
  ],
  install : true,
  subdir : 'scipy/special'
)

What seems to be happening is that a ninja order dependency goes missing. These are the build rules generated:

build scipy/special/_ufuncs.pyx scipy/special/_ufuncs_defs.h scipy/special/_ufuncs_cxx.pyx scipy/special/_ufuncs_cxx.pxd scipy/special/_ufuncs_cxx_defs.h scipy/special/_ufuncs.pyi scipy/special/cython_special.pyx scipy/special/cython_special.pxd: CUSTOM_COMMAND ../scipy/special/_generate_pyx.py ../scipy/special/functions.json ../scipy/special/add_newdocs.py | /home/rgommers/anaconda3/envs/scipy-meson/bin/python3.9
 COMMAND = /home/rgommers/anaconda3/envs/scipy-meson/bin/python3.9 ../scipy/special/_generate_pyx.py -o scipy/special
 description = Generating$ _generate_cy$ with$ a$ custom$ command

build scipy/special/__init__.py scipy/special/_agm.pxd scipy/special/_boxcox.pxd scipy/special/_cephes.pxd scipy/special/_complexstuff.pxd scipy/special/_convex_analysis.pxd scipy/special/_cunity.pxd scipy/special/_digamma.pxd scipy/special/_ellip_harm.pxd scipy/special/_ellip_harm_2.pxd scipy/special/_ellipk.pxd scipy/special/_evalpoly.pxd scipy/special/_exprel.pxd scipy/special/_factorial.pxd scipy/special/_hyp0f1.pxd scipy/special/_hypergeometric.pxd scipy/special/_lambertw.pxd scipy/special/_legacy.pxd scipy/special/_loggamma.pxd scipy/special/_ndtri_exp.pxd scipy/special/_sici.pxd scipy/special/_spence.pxd scipy/special/_spherical_bessel.pxd scipy/special/_trig.pxd scipy/special/_wright_bessel.pxd scipy/special/_xlogy.pxd scipy/special/orthogonal_eval.pxd scipy/special/sf_error.pxd scipy/special/sph_harm.pxd scipy/special/_cython_special.pxi scipy/special/_cython_special_custom.pxi scipy/special/_ufuncs_extra_code.pxi scipy/special/_ufuncs_extra_code_common.pxi: CUSTOM_COMMAND ../scipy/special/__init__.py ../scipy/special/_agm.pxd ../scipy/special/_boxcox.pxd ../scipy/special/_cephes.pxd ../scipy/special/_complexstuff.pxd ../scipy/special/_convex_analysis.pxd ../scipy/special/_cunity.pxd ../scipy/special/_digamma.pxd ../scipy/special/_ellip_harm.pxd ../scipy/special/_ellip_harm_2.pxd ../scipy/special/_ellipk.pxd ../scipy/special/_evalpoly.pxd ../scipy/special/_exprel.pxd ../scipy/special/_factorial.pxd ../scipy/special/_hyp0f1.pxd ../scipy/special/_hypergeometric.pxd ../scipy/special/_lambertw.pxd ../scipy/special/_legacy.pxd ../scipy/special/_loggamma.pxd ../scipy/special/_ndtri_exp.pxd ../scipy/special/_sici.pxd ../scipy/special/_spence.pxd ../scipy/special/_spherical_bessel.pxd ../scipy/special/_trig.pxd ../scipy/special/_wright_bessel.pxd ../scipy/special/_xlogy.pxd ../scipy/special/orthogonal_eval.pxd ../scipy/special/sf_error.pxd ../scipy/special/sph_harm.pxd ../scipy/special/_cython_special.pxi ../scipy/special/_cython_special_custom.pxi ../scipy/special/_ufuncs_extra_code.pxi ../scipy/special/_ufuncs_extra_code_common.pxi | /usr/bin/cp
 COMMAND = /usr/bin/cp ../scipy/special/__init__.py ../scipy/special/_agm.pxd ../scipy/special/_boxcox.pxd ../scipy/special/_cephes.pxd ../scipy/special/_complexstuff.pxd ../scipy/special/_convex_analysis.pxd ../scipy/special/_cunity.pxd ../scipy/special/_digamma.pxd ../scipy/special/_ellip_harm.pxd ../scipy/special/_ellip_harm_2.pxd ../scipy/special/_ellipk.pxd ../scipy/special/_evalpoly.pxd ../scipy/special/_exprel.pxd ../scipy/special/_factorial.pxd ../scipy/special/_hyp0f1.pxd ../scipy/special/_hypergeometric.pxd ../scipy/special/_lambertw.pxd ../scipy/special/_legacy.pxd ../scipy/special/_loggamma.pxd ../scipy/special/_ndtri_exp.pxd ../scipy/special/_sici.pxd ../scipy/special/_spence.pxd ../scipy/special/_spherical_bessel.pxd ../scipy/special/_trig.pxd ../scipy/special/_wright_bessel.pxd ../scipy/special/_xlogy.pxd ../scipy/special/orthogonal_eval.pxd ../scipy/special/sf_error.pxd ../scipy/special/sph_harm.pxd ../scipy/special/_cython_special.pxi ../scipy/special/_cython_special_custom.pxi ../scipy/special/_ufuncs_extra_code.pxi ../scipy/special/_ufuncs_extra_code_common.pxi -t scipy/special
 description = Generating$ _ufuncs_pxi_pxd$ with$ a$ custom$ command


build scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/scipy/special/_ufuncs.pyx.c: cython_COMPILER scipy/special/_ufuncs.pyx
 ARGS = --fast-fail -3 -o scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/scipy/special/_ufuncs.pyx.c

build scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/meson-generated_scipy_special__ufuncs.pyx.c.o: c_COMPILER scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/scipy/special/_ufuncs.pyx.c || scipy/__init__.py scipy/special/__init__.py scipy/special/_agm.pxd scipy/special/_boxcox.pxd scipy/special/_cephes.pxd scipy/special/_complexstuff.pxd scipy/special/_convex_analysis.pxd scipy/special/_cunity.pxd scipy/special/_cython_special.pxi scipy/special/_cython_special_custom.pxi scipy/special/_digamma.pxd scipy/special/_ellip_harm.pxd scipy/special/_ellip_harm_2.pxd scipy/special/_ellipk.pxd scipy/special/_evalpoly.pxd scipy/special/_exprel.pxd scipy/special/_factorial.pxd scipy/special/_hyp0f1.pxd scipy/special/_hypergeometric.pxd scipy/special/_lambertw.pxd scipy/special/_legacy.pxd scipy/special/_loggamma.pxd scipy/special/_ndtri_exp.pxd scipy/special/_sici.pxd scipy/special/_spence.pxd scipy/special/_spherical_bessel.pxd scipy/special/_trig.pxd scipy/special/_ufuncs_extra_code.pxi scipy/special/_ufuncs_extra_code_common.pxi scipy/special/_wright_bessel.pxd scipy/special/_xlogy.pxd scipy/special/orthogonal_eval.pxd scipy/special/sf_error.pxd scipy/special/sph_harm.pxd
 DEPFILE = scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/meson-generated_scipy_special__ufuncs.pyx.c.o.d
 DEPFILE_UNQUOTED = scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/meson-generated_scipy_special__ufuncs.pyx.c.o.d
 ARGS = -Iscipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p -Iscipy/special -I../scipy/special -I/home/rgommers/anaconda3/envs/scipy-meson/lib/python3.9/site-packages/numpy/core/include -Iscipy/_lib -I../scipy/_lib -I../scipy/_build_utils/src -Iscipy -I/home/rgommers/anaconda3/envs/scipy-meson/include/python3.9 -I/home/rgommers/anaconda3/envs/scipy-meson/include -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -w -std=c99 -O2 -g -Wno-unused-function -Wno-conversion -Wno-unused-but-set-variable -Wno-misleading-indentation -Wno-incompatible-pointer-types -fPIC -DNPY_NO_DEPRECATED_API=NPY_1_9_API_VERSION

If I inspect the files in the build dir, the .pxi files are indeed missing:

$ ls build/scipy/special
_comb.cpython-39-x86_64-linux-gnu.so.p           _precompute
cython_special.cpython-39-x86_64-linux-gnu.so.p  specfun.cpython-39-x86_64-linux-gnu.so
cython_special.pxd                               specfun.cpython-39-x86_64-linux-gnu.so.p
cython_special.pyx                               specfunmodule.c
_ellip_harm_2.cpython-39-x86_64-linux-gnu.so.p   _test_round.cpython-39-x86_64-linux-gnu.so.p
libamos.a                                        tests
libamos.a.p                                      _ufuncs.cpython-39-x86_64-linux-gnu.so.p
libcdflib.a                                      _ufuncs_cxx.cpython-39-x86_64-linux-gnu.so.p
libcdflib.a.p                                    _ufuncs_cxx_defs.h
libcephes.a                                      _ufuncs_cxx.pxd
libcephes.a.p                                    _ufuncs_cxx.pyx
libmach.a                                        _ufuncs_defs.h
libmach.a.p                                      _ufuncs.pyi
libspecfun.a                                     _ufuncs.pyx
libspecfun.a.p

If I understand what Ninja is doing correctly, this line:

build scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/scipy/special/_ufuncs.pyx.c: cython_COMPILER scipy/special/_ufuncs.pyx

is missing the || scipy/__init__.py .... scipy/special/_ufuncs_extra_code_common.pxi ... order dependency that the next line does have.

To Reproduce

I can write a standalone test case, but given that it's a bit of work and I'm not 100% sure what the best way is to write a test for such a timing sensitive order dependency issue, I wanted to describe my actual failure first. Should the test case use -j 1, somehow inspect the generated ninja build rules, or something else?

In the mean time, this is reproducible from https://github.com/rgommers/scipy/tree/meson-issue-ufuncs-pyx with:

conda env create -f environment.yml
conda activate scipy-dev
python -m pip install git+https://github.com/mesonbuild/meson.git@master

meson setup build --prefix=$PWD/installdir
ninja -C build -j 1  # it's timing-dependent, so build with 1 job to avoid that

Expected behavior
Build should respect order dependency, and hence Cython should not fail on a missing .pxi file that is a dependency.

system parameters

  • native build
  • operating system: Arch Linux
  • Python version: 3.9.2
  • meson --version: 0.58.999
  • ninja --version: 1.10.2

@dcbaker I know gh-8775 is supposed to address this in the long term, but I believe there will still be cases where we will need custom_target in combination with compiling Cython code - we have a lot of different cases where we codegen .pyx or .pxd files. EDIT: we also need it for adding --cplus for example.

@eli-schwartz
Copy link
Member

So the problem is, a py3.extension_module() with cython inputs requires the dependencies: [..., _ufuncs_pxi_pxd_dep] to be fulfilled in time for the .pyx to .pyx.c transpile, but it is only guaranteed to be available for the .c to .c.o compile?

@rgommers
Copy link
Contributor Author

rgommers commented Jul 4, 2021

Yes exactly.

@rgommers
Copy link
Contributor Author

A few people have run into this now with the SciPy build, and it also looks like it's worse on Windows, maybe because we have to copy files there with a Python script which is slower than invoking cp (Cc @matthew-brett). To work on fixing it, it'd be great to have some input on these two questions (@dcbaker or @eli-schwartz?):

  1. Any pointers on the high-level approach for a fix?
  2. How to write a reliable test case - copy of my question from issue description:

I can write a standalone test case, but given that it's a bit of work and I'm not 100% sure what the best way is to write a test for such a timing sensitive order dependency issue, I wanted to describe my actual failure first. Should the test case use -j 1, somehow inspect the generated ninja build rules, or something else?

@dcbaker
Copy link
Member

dcbaker commented Oct 17, 2021

I think this is actually a problem for vala as well. My understanding is that for languages like vala and cython that transpile to another language which is then compiled, is that some dependencies need to be provided for the transpile step, and some for the compile step. Currently they're provided to the compile step. This likely needs to change to the Meson DSL, in that BuildTargets need to have something like transpile_depends, transpile_dependencies, and transpile_depend_files. At the very least we need the dependencies one, so that things are passed to the right rule. We could make depends and depend_files act at the transpile step instead of the compile step, but that might artificially slow things down

@eli-schwartz
Copy link
Member

I agree with that analysis... a test case would involve making a minimal example that exercises the need for transpile dependencies, and then having the test case attempt to build ninja scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/scipy/special/_ufuncs.pyx.c rather than attempting to hit the problem via timing.

The alternatives to transpile_depend_files would be:

@Jehan
Copy link
Contributor

Jehan commented Oct 17, 2021

a generic buildorder function for binding new order-only dependencies, as @Jehan asked for in Adding a generic build order rule #8123

The generic buildorder function would not really be just an alternative. It could be complementary. More efficient specific options for transpiling are nice. But the generic feature is just the way to not be blocked when you discover new use cases (blocked e.g. if we have to wait for meson to add specific options). Just saying. 😉

@matthew-brett
Copy link
Contributor

Just to check I understand - the underlying problem here is that the depends of the py.extension with Cython sources, applies only to the final .c -> .so step, but not ? Is there / should there be any way of specifying that the depends applies instead to the initial Cython source -> .c step? Or otherwise - what is the next step for defining and implementing the best solution?

@rgommers
Copy link
Contributor Author

only to the final .c -> .so step, but not ?

yes indeed, not .pyx to .c

Is there / should there be any way of specifying that the depends applies instead to the initial Cython source -> .c step?

Yes, the potential solutions are the last comments above of @dcbaker and @eli-schwartz.

@matthew-brett
Copy link
Contributor

matthew-brett commented Jan 11, 2022

Sorry - yes - I saw the earlier suggestions. What I was floating was a slight variant of @dcbaker's suggestions, which was to add a depends_apply='compile' (default) or depends_apply='transpile'. But can see the argument for separate transpile_dependencies etc.

In any case - what's the best way forward from here?

@tristan957
Copy link
Contributor

Possible solution: depends: {'compile': xxx, 'transpile': xxx}

@xclaesse
Copy link
Member

While digging how cython works in Meson I also noticed this issue. I think the fix is actually pretty easy, for any other source file (e.g. C++ files), all unknown generated sources is considered a "header" and added into order_deps:

                # Assume anything not specifically a source file is a header. This is because
                # people generate files with weird suffixes (.inc, .fh) that they then include
                # in their source files.
                header_deps.append(raw_src)

The issue is simply that for cython sources, that code path is not reached. I'm currently working on exactly that.

But while working on it, I also noticed that cython does not generate a depfile when compiling, there is no CLI option to generate one?

@eli-schwartz
Copy link
Member

But while working on it, I also noticed that cython does not generate a depfile when compiling, there is no CLI option to generate one?

The lack of a CLI option is in fact exactly why we had to implement cython support without using such an option. It's a known issue which I am tracking.

@rgommers
Copy link
Contributor Author

Re depfile support in Cython: this was added to cythonize (not cython) in Cython 0.29.27, released about a month ago. See #9049 (comment)

@lithomas1
Copy link
Member

We are hitting this in pandas as well, and building the .o file individually works correctly, while building all targets doesn't work, since the .pyx.c target might get built before the .o target.

Maybe a solution could be to mark the .pyx.c file generation as not built by default? It will get built anyways since the .o target will depend on it, but only attempt to build it once the order-only deps(.pxi) are satisfied.

@lithomas1
Copy link
Member

@xclaesse @eli-schwartz Any thoughts? I'm going to look into this soon.

@eli-schwartz
Copy link
Member

eli-schwartz commented Oct 25, 2022

I don't think that will work... the .pxi, creation rule, the .c creation rule, and the .o compiler rule all end up in the build graph either way, queued for building, and we need a relationship rule for .pxi -> .c, the same way we have a .c -> .o relationship rule.

We can do that with the generated headers approach (add .pxi output files as additional sources), we just need a way to "push" that from the "before .o" to "before .c" stage.

@eli-schwartz
Copy link
Member

We now handle this with pxi header-style order dependencies. Thanks @lithomas1!

I think this is solved now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants