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
Enable building DIPY with Meson #2715
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2715 +/- ##
==========================================
- Coverage 81.81% 81.72% -0.10%
==========================================
Files 147 147
Lines 20554 20587 +33
Branches 3289 3288 -1
==========================================
+ Hits 16816 16824 +8
- Misses 2905 2933 +28
+ Partials 833 830 -3
|
meson.build
Outdated
# Hide symbols when building on Linux with GCC. For Python extension modules, | ||
# we only need `PyInit_*` to be public, anything else may cause problems. So we | ||
# use a linker script to avoid exporting those symbols (this is in addition to | ||
# Meson using `-fvisibility=hidden` for C and `-fvisibility-inlines-hidden` for | ||
# C++ code. See gh-15996 for details. | ||
_linker_script = meson.project_source_root() / 'skimage/_build_utils/link-version-pyinit.map' | ||
version_link_args = ['-Wl,--version-script=' + _linker_script] | ||
# Note that FreeBSD only accepts version scripts when -shared is passed, | ||
# hence we need to pass that to `cc.links` explicitly (flag is already | ||
# present for `extension_module` invocations). | ||
if not cc.links('', name: '-Wl,--version-script', args: ['-shared', version_link_args]) | ||
version_link_args = [] | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you strictly need this? In SciPy's case it's complicated because they have a variety of code sources, and for example, Fortran code doesn't support visibility, so hiding Fortran symbols requires the linker script. And for vendored code embedded as static libraries, it can be too complicated to modify that external code to make it hide itself.
You may be able to get away with not bothering with this. :)
Thank you for your feedback @eli-schwartz! I will deeper into all this later this week. As you notice, there is still a lot of cleaning to do. I will ping you back when I feel this PR ready. |
We renamed |
ok, some progress. Quick note to myself (or anyone here)
-some cleaning still needed. |
Out of curiosity, what is the status of this PR, and are there any blockers? |
Hi @stefanv, Thank you for asking. Currently the main blocker is that the compilation fails on Windows (mingw-64) with this error To be correct, they failed with the github action. I did not try yet on a personal window machine (working with macOs) If you have any idea how to fix this, it will be more than welcomed! Otherwise, some optimization flags to check (make sure they are correct in all OS) and make sure that we are using correctly the |
Would compiling with clang be an option? That's the only compiler we were able to get working for skimage: https://github.com/scikit-image/scikit-image/blob/main/.github/workflows/wheels_recipe.yml#L168 |
Interesting, I will get back to this next week and try with clang. Thanks @stefanv ! |
ok, some progress, it seems to pass on Windows now with Few tests are failing and it might be due to some compilation flag. Also, not sure why pytest do not ignore benchmark. the config file is ignored for some reason tons of commit need to be squash. |
You may want to try spin 0.7: https://pypi.org/project/spin/0.7/ Here are the release notes: https://github.com/scientific-python/spin/blob/main/CHANGELOG.md |
.spin/cmds.py
Outdated
|
||
@click.command() | ||
@click.argument("asv_args", nargs=-1) | ||
def asv(asv_args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may also want to look at what we've written for NumPy:
https://github.com/numpy/numpy/blob/main/.spin/cmds.py#L331
Yours is much more straightforward, which I like, but perhaps there are useful features there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's cool! thanks @stefanv.
for sure, I will have a closer look later on a new PR. For now, I keep it simple and focus on meson + failing CI's.
this PR is close to be done. I will tackle / update the documentation on a new PR.
@jarrodmillman and @stefanv, thank you for spin
tools, really useful, looking forward to add new commands on a new PR.
ps: Do you know why on Windows set_num_threads
does not work. it seems that OpenMP use all the threads each times. Did you encounter this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which set_num_threads
are you referring to?
Are you looking for something like https://pypi.org/project/threadpoolctl/ perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was referring to our own codebase:
Line 82 in 79c3939
cdef void set_num_threads(num_threads): |
but more generally, do you have issue with openmp with scikit-image or other package ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't had any reports, but I'm not sure anyone is watching that closely on Windows. I think the most likely place where these issues would have been hashed out is in threadpoolctl
.
3c73ed9
to
7fd33fd
Compare
Hello @skoudoro, Thank you for updating !
Comment last updated at 2023-11-17 02:34:56 UTC |
For now, this PR is done. Can you someone test it and have a look @arokem? @Garyfallidis? this PR superesed #2787. Next week, I will create new PR:
|
provides benchmarking mvp
@jhlegarreta, Can you look why it failed on python3.8? You can see the error here: https://github.com/dipy/dipy/actions/runs/6740916808/job/18324698874?pr=2715#step:7:150 maybe there is another way to get includes files from numpy. thank you. |
https://github.com/dipy/dipy/actions/runs/6740916808/job/18324698874?pr=2715#step:9:6644 type float has no attribute sqrt |
Python 3.8 failure:
At fist sight having to do with mason and maybe the NumPy version in Python 3.8. |
@skoudoro concerning #2715 (comment) I am not sure if NumPy is available at the time this call is performed (?) No trace of messages saying that |
Numpy is installed before as you can see here : https://github.com/dipy/dipy/actions/runs/6741509003/job/18326222539?pr=2715#step:6:66 |
OK, missed that line then. Thanks. |
Getting the NumPy include directory is a bit cumbersome right now I'm afraid. I suggest to copy exactly what SciPy does: https://github.com/scipy/scipy/blob/6ce4aaf4bc5222e3d1355cd8a392e88f45b4f959/scipy/meson.build#L30-L82. The different cases are needed to deal with things like in-package-tree virtualenvs. I aim to make this as simple as np_dep = dependency('numpy') but that won't land before NumPy 2.0 early next year. |
Thank you @rgommers, looking forward for the new way to handle this! |
Do you have an idea @rgommers why numpy include dir could fail on python3.8. see here: https://github.com/dipy/dipy/actions/runs/6786880602/job/18448417652?pr=2715#step:7:159 all our python 3.8 build failed with this same error:
|
You have no entry for numpy with python 3.8 in your build dependencies ( Also, your |
Arf, how I missed that... Good catch! thank you @rgommers ok, I will just drop the python 3.8 CI's. Sorry @jhlegarreta (but no worries, wheels will be created for python 3.8 for this release a last time). |
add python 3.12 CIs
If you drop CI's do you have a means to know and let people openly know that wheels for Python 3.8 will be working correctly? If the changes Ralf has mentioned are not hard, I would keep 3.8 until the release has been done. |
wheels will not be created if CI's fails for python 3.8. I will let you look at https://github.com/MacPython/dipy-wheels to go deeper in the wheels creation mechanism for DIPY. Another thing that we need to document 😅 and clarify. |
Then it seems counter-intuitive to remove the CIs for Python 3.8: if CI's pass, you have a better guarantee that you will not run into trouble creating the wheels. I still do not see why we should drop the CIs it at this stage; it would be 2 lines to modify in |
ok, I see no additional comment, all is fixed. I am going ahead and merge this PR to move forward for the release. Feel free to create an issue if there are any concerns! Thanks everyone for the helps / advise / reviews |
Monumental work on this serge! Kudos and big thanks to everyone who helped! |
@skoudoro I have noticed that adapting some of the CI scripts to this change was inadvertently left behind, e.g. We no longer have a |
This is the main motivation of this PR. Python 3.12 should be out in September/October this year.
Our current build system is using
distutils
😨 . Since all major packages/depencencies are moving to meson (scipy, numpy, scikit-image, scikit-learn, etc..), we follow the trend 😅 . Futhermore, Meson seems easy to use from my small and short experience. The maintenance should be easier.The build should be complete. The full test suite passes. It is slower for now. I suppose, it is just missing some important compilation flags. Tested only on my macOS M1 (arm64 and rosetta - x86_64).
For now, This contains:
To test it
pip install -r requirements/build.txt python -m dev.py build python -m dev.py test -- -svv
Need to be done:
*.pyd
management. This will help us a lot and it seems to be done. We might need updatecython
andmeson-python
minimum version soon.openmp
managementSome References
Quansight Labs blog: Moving SciPy to the Meson build system
SciPy docs: How to build SciPy with Meson
SciPy docs: advanced Meson build topics
Scipy migration discussion
close #2514