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 build issues in combination with generated sources #31

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

Cython build issues in combination with generated sources #31

rgommers opened this issue Jul 4, 2021 · 22 comments
Assignees

Comments

@rgommers
Copy link
Owner

rgommers commented Jul 4, 2021

There's multiple places where we can't express dependencies correctly, or we can but they go missing. I filed one issue upstream: mesonbuild/meson#8961.

Extensions that have problems:

scipy.special._ufuncs

[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"
^

scipy.linalg._decomp_update

If we build without expressing the dependency on cython_blas.pxd:

TypeError: C function scipy.linalg.cython_blas.daxpy has wrong signature (expected void (int *, __pyx_t_5scipy_6linalg_11cython_blas_d *, __pyx_t_5scipy_6linalg_11cython_blas_d *, int *, __pyx_t_5scipy_6linalg_11cython_blas_d *, int *), got void (int *, __pyx_t_11cython_blas_d *, __pyx_t_11cython_blas_d *, int *, __pyx_t_11cython_blas_d *, int *))

And if we add it, that doesn't seem supported:

Found ninja-1.10.2 at /home/rgommers/anaconda3/envs/scipy-meson/bin/ninja
Traceback (most recent call last):                                                                  
  File "/home/rgommers/anaconda3/envs/scipy-meson/lib/python3.9/site-packages/mesonbuild/mesonmain.py", line 223, in run
    return options.run_func(options)
  File "/home/rgommers/anaconda3/envs/scipy-meson/lib/python3.9/site-packages/mesonbuild/msetup.py", line 281, in run
    app.generate()
  File "/home/rgommers/anaconda3/envs/scipy-meson/lib/python3.9/site-packages/mesonbuild/msetup.py", line 184, in generate
    self._generate(env)
  File "/home/rgommers/anaconda3/envs/scipy-meson/lib/python3.9/site-packages/mesonbuild/msetup.py", line 246, in _generate
    intr.backend.generate()
  File "/home/rgommers/anaconda3/envs/scipy-meson/lib/python3.9/site-packages/mesonbuild/backend/ninjabackend.py", line 544, in generate
    self.generate_target(t)
  File "/home/rgommers/anaconda3/envs/scipy-meson/lib/python3.9/site-packages/mesonbuild/backend/ninjabackend.py", line 753, in generate_target
    transpiled_sources = self.generate_cython_transpile(target)
  File "/home/rgommers/anaconda3/envs/scipy-meson/lib/python3.9/site-packages/mesonbuild/backend/ninjabackend.py", line 1601, in generate_cython_transpile
    generated_sources[ssrc] = mesonlib.File.from_built_file(gen.subdir, ssrc)
AttributeError: 'CustomTargetIndex' object has no attribute 'subdir'
FAILED: build.ninja 

If we add all of _generate_cy as a dependency, then we rebuild cython_blas.pyx and cython_lapack.pyx which isn't right.

optimize._lsq.givens_elimination

Not built yet, but also depends on cython_lapack.pxd

@rgommers
Copy link
Owner Author

rgommers commented Jul 4, 2021

The problem in both cases is missing files in the build/ directory. A workaround is to copy them over manually, then re-run the build. Once the build works once, it will keep working until you do git clean -xdf.

So:

cp scipy/*.pxd build/scipy/

cp scipy/special/*.pxi build/scipy/special/
cp scipy/special/*.pxd build/scipy/special/
cp scipy/special/__init__.py build/scipy/special/

python scipy/linalg/_generate_pyx.py --outdir build
cp scipy/linalg/__init__.py build/scipy/linalg/

@rgommers
Copy link
Owner Author

rgommers commented Jul 4, 2021

@Smit-create maybe one of these was your problem? If so, the workaround should help.

@Smit-create
Copy link

I had the following error:

$ meson setup build --prefix=$PWD/installdir
The Meson build system
Version: 0.58.1
Source dir: /home/smit/Smitlunagariya/scipy
Build dir: /home/smit/Smitlunagariya/scipy/build
Build type: native build
Project name: SciPy
Project version: 1.7.0.dev0+ababababab
Fortran compiler for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-

gfortran (gcc 9.3.0 "GNU Fortran (crosstool-NG 1.24.0.133_b0863d8_dirty) 9.3.0")
Fortran linker for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-gf
ortran ld.bfd 2.36.1
C compiler for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-cc (gc
c 9.3.0 "x86_64-conda-linux-gnu-cc (crosstool-NG 1.24.0.133_b0863d8_dirty) 9.3.0")
C linker for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-cc ld.bf
d 2.36.1
C++ compiler for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-c++
(gcc 9.3.0 "x86_64-conda-linux-gnu-c++ (crosstool-NG 1.24.0.133_b0863d8_dirty) 9.3.0")
C++ linker for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-c++ ld
.bfd 2.36.1

meson.build:1:0: ERROR: Tried to use unknown language "cython".

When, I removed cython as the 1st language just to check whether it works without it, I get:

$ meson setup build --prefix=$PWD/installdir
The Meson build system
Version: 0.58.1
Source dir: /home/smit/Smitlunagariya/scipy
Build dir: /home/smit/Smitlunagariya/scipy/build
Build type: native build
Project name: SciPy
Project version: 1.7.0.dev0+ababababab
Fortran compiler for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-
gfortran (gcc 9.3.0 "GNU Fortran (crosstool-NG 1.24.0.133_b0863d8_dirty) 9.3.0")
Fortran linker for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-gf
ortran ld.bfd 2.36.1

C compiler for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-cc (gc
c 9.3.0 "x86_64-conda-linux-gnu-cc (crosstool-NG 1.24.0.133_b0863d8_dirty) 9.3.0")
C linker for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-cc ld.bf
d 2.36.1
C++ compiler for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-c++
(gcc 9.3.0 "x86_64-conda-linux-gnu-c++ (crosstool-NG 1.24.0.133_b0863d8_dirty) 9.3.0")
C++ linker for the host machine: /home/smit/anaconda3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-c++ ld
.bfd 2.36.1
Host machine cpu family: x86_64
Host machine cpu: x86_64
Program cython found: YES (/home/smit/anaconda3/envs/scipy-dev/bin/cython)
Program pythran found: YES (/home/smit/anaconda3/envs/scipy-dev/bin/pythran)
Program python3 found: YES (/home/smit/anaconda3/envs/scipy-dev/bin/python)
Found pkg-config: /home/smit/anaconda3/envs/scipy-dev/bin/pkg-config (0.29.2)
Dependency python found: YES (pkgconfig)
Library npymath found: YES
Library npyrandom found: YES
Run-time dependency openblas found: YES 0.3.15
Dependency openblas found: YES 0.3.15 (cached)
Program cp found: YES (/bin/cp)
Program cp found: YES (/bin/cp)
Program cp found: YES (/bin/cp)
Run-time dependency threads found: YES
Program cp found: YES (/bin/cp)
Checking for size of "void*" : 8
Dependency threads found: YES unknown (cached)

Checking for function "open_memstream" : NO
Configuring messagestream_config.h using configuration
Compiler for C++ supports arguments -fvisibility=hidden: YES
Program cp found: YES (/bin/cp)
Program cp found: YES (/bin/cp)
Program cp found: YES (/bin/cp)
Compiler for C++ supports arguments -fvisibility=hidden: YES (cached)
Program cp found: YES (/bin/cp)
Dependency openblas found: YES 0.3.15 (cached)
Program cp found: YES (/bin/cp)
Build targets in project: 145

Found ninja-1.8.2 at /usr/bin/ninja

ERROR: No specified compiler can handle file scipy/sparse/csgraph/_flow.pyx

@rgommers
Copy link
Owner Author

rgommers commented Jul 4, 2021

Ah, that does mean you have an old version, it says you're using meson 0.58.1. If you had installed from the master branch, you'd be at 0.58.999 (it is what I have locally, and it is the version number at https://github.com/mesonbuild/meson/blob/master/mesonbuild/coredata.py#L46). You can either use:

python -m pip install git+https://github.com/mesonbuild/meson.git@master

or you can just clone the repo and locally run pip install . in the root of the repo.

@rgommers
Copy link
Owner Author

rgommers commented Oct 6, 2021

Some more failures in gh-76. Cause should be mesonbuild/meson#8961.

@rgommers
Copy link
Owner Author

This is starting to be urgent to fix. It continues to fail on the macOS CI job, and on Windows it needs -j 128 to work around it.

Here is a simple explanation of the issue: scipy#14847 (comment)

And a potential solution in Cython that I hadn't considered before: scipy#14847 (comment).

@rgommers
Copy link
Owner Author

Cython finds the module name from the path in a package here: https://github.com/cython/cython/blob/f372c5ab67c73a4c28e68e2aa8bee16a2ec0e4e1/Cython/Compiler/Main.py#L381

full_module_name is passed through a whole chain of functions, because it may be auto-determined by extract_module_name, or for files outside a package it's the filename, or it can be specified by the user in the Extension module (setuptools-specific). Related: exporting module names can be skipped with CYTHON_NO_PYINIT_EXPORT. Where the chain starts:

  • calling cython .... somefile.pyx on the command line calls main()
  • main() basically does options, sources = parse_command_line(args) followed by compile(sources, options)
  • compile() has a full_module_name=None parameter, but this cannot be used from the command line, only from Extension or another Python interface.

Generated C code will end up in a build dir, like at: build/scipy/special/_ufuncs.cpython-39-x86_64-linux-gnu.so.p/scipy/special/_ufuncs.pyx.c

The generated C code will contain the module name, like so:

#define __Pyx_MODULE_NAME "scipy.special._ufuncs"

If something goes wrong related to this issue, that name changes:

#define __Pyx_MODULE_NAME "_ufuncs"

A decent design would be to allow using cython ... --name scipy.special._ufuncs _ufuncs.pyx. This doesn't look too difficult to implement, and would get rid of all the .py file copying. And it's just bringing feature parity between the command line interface and Extension.

@matthew-brett
Copy link

@rgommers - I'm happy to have a go at that - let me know?

@rgommers
Copy link
Owner Author

That would be great, thanks @matthew-brett. I had just started looking into it, but I'm really short on time so would be great if you wanted to drive this one.

Here's the full extent of my WIP patch, to save you about 5 minutes:

$ git diff
diff --git a/Cython/Compiler/CmdLine.py b/Cython/Compiler/CmdLine.py
index ffff6a61c..6126ea3e2 100644
--- a/Cython/Compiler/CmdLine.py
+++ b/Cython/Compiler/CmdLine.py
@@ -145,6 +145,10 @@ def create_cython_argparser():
                       dest='compile_time_env', type=str,
                       action=ParseCompileTimeEnvAction,
                       help='Provides compile time env like DEF would do.')
+    parser.add_argument("--name", dest='name', type=str, nargs=1, action='store',
+                      help='Fully qualified module name. If not given, is '
+                           'deduced from the import path if source file is in '
+                           'a package, or equals the filename otherwise.')
     parser.add_argument('sources', nargs='*', default=[])
 
     # TODO: add help

@matthew-brett
Copy link

matthew-brett commented Dec 29, 2021 via email

@eli-schwartz
Copy link

That looks neat. Since meson knows what the fully qualified module name is supposed to be -- using py_installation.extension_module('_ufuncs', sources, subdir: 'scipy/special') it knows it will install scipy/special/_ufuncs.cpython-310-x86_64-linux-gnu.so which is also the above mentioned module name -- it should be easy to automatically handle in meson without requiring any options (just like setuptools does by taking advantage of the same logic and using cython as a python module instead of a command line program).

I will look forward to the opportunity to plug that prospective support into meson.

matthew-brett added a commit to matthew-brett/cython that referenced this issue Dec 30, 2021
It can be useful to specify the module name for the output file
directly, rather than working it out from the enclosing file tree -
particularly for out of tree build systems, like Meson.

See: rgommers/scipy#31 (comment)
for background.
@matthew-brett
Copy link

Sketch at cython/cython#4548

matthew-brett added a commit to matthew-brett/cython that referenced this issue Jan 8, 2022
It can be useful to specify the module name for the output file
directly, rather than working it out from the enclosing file tree -
particularly for out of tree build systems, like Meson.

See: rgommers/scipy#31 (comment)
for background.
matthew-brett added a commit to matthew-brett/cython that referenced this issue Jan 8, 2022
It can be useful to specify the module name for the output file
directly, rather than working it out from the enclosing file tree -
particularly for out of tree build systems, like Meson.

See: rgommers/scipy#31 (comment)
for background.
@matthew-brett
Copy link

I think this allows us to do something neat like https://github.com/matthew-brett/cyext/blob/main/meson.build .

@rgommers
Copy link
Owner Author

I think this allows us to do something neat like https://github.com/matthew-brett/cyext/blob/main/meson.build .

That'll be a very temporary thing right? We want support in Meson, and then cython will be invoked by ninja. Or maybe I'm missing what is neat here?

@matthew-brett
Copy link

matthew-brett commented Jan 10, 2022

Hmm - I hadn't registered how complex the Cython build was. Just for my reference, there are two issues for the source / build tree and Cython:

  • Finding the correct module name (can be solved by the --module-name argument);
  • Making sure needed generated .pyx .pxd files are cimportable at build time (not solved by new argument).

Here's a first sketch at respecifying a dependency. I believe it also correctly adds a cimport dependency (on cython_blas.pyx cython_blas.pyx):

matthew-brett@7b33fce

@rgommers
Copy link
Owner Author

Making sure needed generated .pyx files are cimportable at build time (not solved by new argument).

I don't think I understand this. You cannot cimport a .pyx file normally, this is what .pxd files are for. It looks like Cython has an option to allow cimporting .pyx file, but it's disabled by default and I have never seen it used: https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html#Cython.Compiler.Options.cimport_from_pyx

I suspect what you're seeing is that a .pxd file that is still needed in the build directory got copied there too late.

@matthew-brett
Copy link

Sorry - typing too quickly, and too late - I meant .pxd files - I'll edit in the original comment. In particular, I was working (above) on build for a file that cimports the generated cython_blas (cython_blas.pxd). The point I was reminding myself of was that our current need for copies of the import tree during Cython compilation comes both from the need for correct module name discovery (removed by the cython --module-name arg), and the need for cimport to work when referencing generated .pxd files.

It is somewhat unrelated to that point, but yes, in testing, it turned out that the _bglu_dense extension did not declare its dependence on cython_blas.pxd, so it was possible for the build to fail, if it got out of order.

@rgommers
Copy link
Owner Author

and the need for cimport to work when referencing generated .pxd files.

Ugh. Yes, you're right. We still need a fix for mesonbuild/meson#8961.

@matthew-brett
Copy link

What about splitting out the .pyx to .c compilation steps, until we do have a good solution to the problem as defined in mesonbuild/meson#8961 ? It's a bit of work, but probably only a few hours, it wouldn't take long to reverse when we do have a solution.

@rgommers
Copy link
Owner Author

That should be fine. It won't solve everything, but it will solve this particular issue I suspect.

@matthew-brett
Copy link

I've done the split in scipy#15407 . I don't get any out-of-order errors now. I think I caught all the dependencies, but please do review.

rgommers pushed a commit to scipy/scipy that referenced this issue Jan 18, 2022
As discussed in rgommers#31 and
Meson issue gh-8961, the current implementation for Cython
(and maybe other) transpile steps means that we cannot
specify dependencies that apply to the transpile (pyx -> c)
step; dependencies only apply to the c building step.  This in turn
means it is possible to start a pyx -> c transpile without e.g. pxd or
`__init__py` files in place, that Cython needs for module name discovery
or `cimports`.

This PR drops 'cython' as a project compiler, and splits all the Cython
transpile steps out into separate steps prior to extension building.  We
can therefore add the transpile dependencies explicitly.

I have tested a `ninja` `-j 1` compile, and several default compiles,
without getting any of the Cython transpile errors we were getting
before, without very high `-j` numbers.
@rgommers
Copy link
Owner Author

Since this should now be fixed, I'll close it. I will add the upstream Meson issue to our tracking issue (gh-22), because we do want to get that fixed at some point, so we can simplify our meson.build files.

scoder pushed a commit to cython/cython that referenced this issue Jul 19, 2022
It can be useful to specify the module name for the output file
directly, rather than working it out from the enclosing file tree -
particularly for out of tree build systems, like Meson.

See background in
rgommers/scipy#31 (comment)
h-vetinari pushed a commit to h-vetinari/cython that referenced this issue Jul 19, 2022
It can be useful to specify the module name for the output file
directly, rather than working it out from the enclosing file tree -
particularly for out of tree build systems, like Meson.

See background in
rgommers/scipy#31 (comment)

Backported-By: H. Vetinari <[email protected]>
scoder pushed a commit to cython/cython that referenced this issue Jul 27, 2022
Backport of #4548

It can be useful to specify the module name for the output file
directly, rather than working it out from the enclosing file tree -
particularly for out of tree build systems, like Meson.

See background in
rgommers/scipy#31 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants