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

BLD: Adding spatial to Meson build #28

Merged
merged 11 commits into from
Jun 30, 2021
Merged

Conversation

czgdp1807
Copy link

Reference issue

What does this implement/fix?

Additional information

scipy/spatial/meson.build Outdated Show resolved Hide resolved
mesondev.sh Outdated Show resolved Hide resolved
Copy link
Owner

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the messagestream incorporation and the missing /EHsc and fvisibility-hidden flags.

@czgdp1807
Copy link
Author

spatial tests fail with the following Traceback,

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/czgdp1807ssd/scipy_project/meson_build/lib/python3.9/site-packages/scipy/spatial/__init__.py", line 101, in <module>
    from ._procrustes import procrustes
  File "/home/czgdp1807ssd/scipy_project/meson_build/lib/python3.9/site-packages/scipy/spatial/_procrustes.py", line 9, in <module>
    from scipy.linalg import orthogonal_procrustes
  File "/home/czgdp1807ssd/scipy_project/meson_build/lib/python3.9/site-packages/scipy/linalg/__init__.py", line 212, in <module>
    from ._decomp_update import *
  File "scipy/linalg/_decomp_update.pyx", line 1, in init scipy.linalg._decomp_update
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 *))

Seems like a problem with _decomp_update.pyx.in -> _decomp_update.pyx conversion.

@rgommers
Copy link
Owner

Seems like a problem with _decomp_update.pyx.in -> _decomp_update.pyx conversion.

Oh that's possible, linalg was one of the first modules I did, and maybe I did something odd in the "deal with generated Cython code" department. Let me have a look.

@rgommers
Copy link
Owner

Odd, this passes for me:

$ pytest installdir/lib/python3.9/site-packages/scipy/spatial/tests/test__procrustes.py
...
collected 6 items                                                                                           

installdir/lib/python3.9/site-packages/scipy/spatial/tests/test__procrustes.py ......                 [100%]

@rgommers
Copy link
Owner

That said, the way _decomp_update is built is not quite right, I'll change it to see if that fixes the problem.

@rgommers
Copy link
Owner

There's one new build warning, maybe you can fix that?

../scipy/spatial/qhull_misc.c:3: warning: "trace1" redefined
    3 | #define trace1(args) {}
      | 
In file included from ../scipy/spatial/qhull_misc.c:1:
../scipy/spatial/qhull_src/src/qhull_ra.h:81: note: this is the location of the previous definition
   81 | #define trace1(args) {if (qh->IStracing >= 1) qh_fprintf args;}
      | 

note that the diff of the fix should also be recorded in qhull_src/README.scipy.

@czgdp1807
Copy link
Author

Should I send the fix to main repo?

@rgommers
Copy link
Owner

Yes, that would be great.

@rgommers
Copy link
Owner

rgommers commented Jun 30, 2021

I can reproduce the _decomp_update issue now, it's a dependency issue (and hence sensitive to timing, can look non-reproducible). To make the issue clearer, in:

TypeError: C function scipy.linalg.cython_blas.daxpy has wrong signature
(expected vs. got:)
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 *)
void (int *, __pyx_t_11cython_blas_d *, __pyx_t_11cython_blas_d *, int *, __pyx_t_11cython_blas_d *, int *))

the difference is scipy_6linalg_11cython_blas_d vs cython_blas_d. This happens when the __init__.py and .pxd files are missing when Cython is run. This is the config:

py3.extension_module('_decomp_update',
  [_decomp_update_pyx, _dummy_init, _dummy_init2, _linalg_pxd],
  include_directories : inc_np,
  dependencies : py3_dep,
  install : true,
  subdir : 'scipy/linalg')

and these are the build rules generated (in build/build.ninja):

build scipy/linalg/_decomp_update.pyx: CUSTOM_COMMAND ../scipy/linalg/_decomp_update.pyx.in | ../scipy/_build_utils/tempita.py /home/rgommers/anaconda3/envs/scipy-meson/bin/python3.9
 COMMAND = /home/rgommers/anaconda3/envs/scipy-meson/bin/python3.9 ../scipy/_build_utils/tempita.py ../scipy/linalg/_decomp_update.pyx.in -o scipy/linalg
 description = Generating$ _decomp_update$ with$ a$ custom$ command

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

build scipy/linalg/_decomp_update.cpython-39-x86_64-linux-gnu.so.p/meson-generated_scipy_linalg__decomp_update.pyx.c.o: c_COMPILER scipy/linalg/_decomp_update.cpython-39-x86_64-linux-gnu.so.p/scipy/linalg/_decomp_update.pyx.c || scipy/__init__.py scipy/linalg.pxd scipy/linalg/__init__.py
 DEPFILE = scipy/linalg/_decomp_update.cpython-39-x86_64-linux-gnu.so.p/meson-generated_scipy_linalg__decomp_update.pyx.c.o.d
 DEPFILE_UNQUOTED = scipy/linalg/_decomp_update.cpython-39-x86_64-linux-gnu.so.p/meson-generated_scipy_linalg__decomp_update.pyx.c.o.d
 ARGS = -Iscipy/linalg/_decomp_update.cpython-39-x86_64-linux-gnu.so.p -Iscipy/linalg -I../scipy/linalg -I/home/rgommers/anaconda3/envs/scipy-meson/lib/python3.9/site-packages/numpy/core/include -Iscipy -I/home/rgommers/anaconda3/envs/scipy-meson/include/python3.9 -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

The second build command misses || scipy/__init__.py scipy/linalg.pxd scipy/linalg/__init__.py. The || operator is a ninja "order dependency". If it's missing, cython can be run before those files are all present.

I've seen this issue once before; I need to write a standalone reproducer to submit the issue to Meson.

@rgommers
Copy link
Owner

The easy workaround is to rerun the build, touching the _decomp_update.pyx.in file - second time around the dependencies will be present in the build dir, so the problem will go away.

The spatial tests still depend on sparse.linalg, so that's the next thing you'll see:

    from scipy.sparse.linalg import LinearOperator
E   ModuleNotFoundError: No module named 'scipy.sparse.linalg'

@czgdp1807
Copy link
Author

May be we can update mesondev.sh to run the build twice before installing.

The spatial tests still depend on sparse.linalg, so that's the next thing you'll see:

Okay. Sure.

@rgommers
Copy link
Owner

May be we can update mesondev.sh to run the build twice before installing.

good idea, done in the meson branch now

Copy link
Owner

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, merging. Thanks Gagan!

@rgommers rgommers merged commit 1459dfd into rgommers:meson Jun 30, 2021
@czgdp1807
Copy link
Author

Great. Moving on to sparse.linalg. Seems like we are doing DFS now.

@czgdp1807 czgdp1807 deleted the spatial branch June 30, 2021 09:24
@rgommers
Copy link
Owner

DFS?

@czgdp1807
Copy link
Author

Depth First Search - We are moving to the next module if current one relies on the next. Kind of going in depth in the module dependency graph.

rgommers pushed a commit that referenced this pull request Jul 3, 2021
rgommers pushed a commit that referenced this pull request Jul 10, 2021
rgommers pushed a commit that referenced this pull request Jul 15, 2021
rgommers pushed a commit that referenced this pull request Jul 27, 2021
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

Successfully merging this pull request may close these issues.

2 participants