-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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: add option to specify numpy header location #61095
Conversation
pandas/meson.build
Outdated
@@ -4,17 +4,24 @@ incdir_numpy = run_command( | |||
'-c', | |||
''' | |||
import os | |||
import numpy as np | |||
|
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.
This custom code to get the include path from NumPy was implemented as a workaround prior to Meson adding first class support, which I believe landed in 1.4
Rather than continue to patch this, I'd prefer if we bump our minimum Meson to 1.4 and use Meson's built-in NumPy resolution mechanisms
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'm also wary of pandas itself adding an option for this; you may want to check upstream if Meson supports NumPy for cross compilation, and if not ask for it to be implemented there rather than here
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 started to rework it to use meson's numpy support - it does look nicer.
I need a day or two to stare at this a bit more though before committing. Meson supports numpy via pkgconfig and numpy-config. Numpy's pkgconfig file is not usable for cross-compiling. numpy-config looks more promising, but I couldn't make it work yet for cross-compiling - at this time I think it's numpy's issue, but want to touch some grass to think it through, maybe I find something if I stop looking at it for a short time. If it's numpy issue, than I would contact numpy about this before changing the meson scripts in pandas, otherwise it could make things more difficult than they are. Will be back.
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.
Nope, unfortunately it requires a change in numpy to make it better - a way to get the includes folder without actually importing the numpy module (it's closely related to numpy/numpy#18209 ).
I think that currently, using meson's numpy resolution support would cause a regression for cross-compiling, it would make things harder. If this PR isn't merged, that's fine - the ultimate goal was to inform you about the root cause of the linked bug, and that this is one (but certainly not the only) possible solution.
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.
@rgommers do you have any thoughts on 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.
But it's the same absolute path as you need to pass in as a build option, like in this PR, right? I was suggesting generating this numpy.pc
on the fly. It's maybe a couple lines more code in your build script, but it avoids adding this build option to every package that depends on NumPy - it seems like a win to me.
If that really doesn't work then this PR is okay too (it's what we've done in SciPy too after all).
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.
Hmmm... technically it is possible. Let me do a poc, and see what the Yocto folks say about it. Will be back.
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.
Going around the same idea, eventually I found an even simpler solution, which I think won't find a lot of resistance in Yocto - we actually don't necessarily need the full absolute path for cross-compiling, the relative path to the sysroot is perfectly enough, and that's a static path. That 1-liner sed needs to be done only once - I don't expect this to be seen controversial at all, will just submit it during the week as a run of the mill patch. Thanks for the hint.
I have pushed the version that uses Meson's own dependency resolution instead of the custom code.
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.
Sounds like good news, thanks!
we actually don't necessarily need the full absolute path for cross-compiling, the relative path to the sysroot is perfectly enough
Out of interest: how does the translation from sysroot path to numpy header location happen?
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.
Yocto has a standard variable that points to the current site-packages dir (without the sysroot path). The change I have just tested is essentially
sed -i "s:\${pcfiledir}:${PYTHON_SITEPACKAGES_DIR}/numpy/_core/lib/pkgconfig:g" [...]/numpy.pc
ff1b9fa
to
563ea31
Compare
563ea31
to
ee76552
Compare
d62c160
to
9867106
Compare
I don't think the build failures showing now are related to this change, but rather a longstanding issue where build warnings were being suppressed with our Meson 1.2.1 pin. #60681 should have the more comprehensive fix for that For now, you can just remove any setup calls with |
9867106
to
e247c1a
Compare
Sure. Though there are still at least 1 or 2 that were broken by this - though if the list would get a bit cleaner, that wouldn't be a problem of course. |
c514975
to
49dac18
Compare
For the pre-commit error I think you just need to run I thought the pre-commit hook would do this for you, so maybe it was skipped. If that doesn't work can help take a closer look |
49dac18
to
d613efa
Compare
You are right, that did the trick. But I'm not really sure why the wheel* workflows weren't kicked off |
83cf21d
to
74e5be0
Compare
I see the problem - it's about the |
Ah yes, |
1338a90
to
d9fb595
Compare
Instead of querying the include folder's location from NumPy during building, use Meson's built-in dependency resolution for NumPy. With this change during build-time Meson first tries to query the dependency details using numpy-config (which, in turn essentially uses the same method as the original code this commit replaces), and in case that fails for some reason, it tries to discover NumPy resources using pkg-config - which, beside being a good fail-over mechanism, has the added benefit of somewhat simpler cross-compiling, as querying the include folder location from NumPy module is only usable for cross-compiling only in some corner cases, while pkg-config is a bit more universal. Signed-off-by: Gyorgy Sarvari <[email protected]>
d9fb595
to
557a93d
Compare
I dunno. I tried it, but i doesn't seem to have any effect. It looks the environment variables are passed correctly, but they disappear in thin air by the time meson is executed. At this time I suspect that might be running into the same issue as mesonbuild/meson-python#604 - and it would need a clean build folder (but today I'm not brave enough to do If you have any ideas, suggestions, please go for it, I'm all ears. Personally I have to put this to rest on my end for at least a day. |
I might have missed a prior conversation but does NumPy definitely distribute the pkg-config file when installed via conda? Checking my local machine in an environment with 1.26.4 there is no directory for |
@@ -40,11 +40,18 @@ jobs: | |||
with: | |||
fetch-depth: 0 | |||
|
|||
- name: Determine NumPy pkg-config location | |||
run: | | |||
echo "PKG_CONFIG_PATH=$(python -c 'import site; print(site.getsitepackages()[0])')/numpy/_core/lib/pkgconfig" >> $GITHUB_ENV |
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 feel like sprinkling PKG_CONFIG_PATH in every workflow seems a bit hacky.
Is it possible for us to upstream something to meson/numpy to autodetect this better so that we can avoid this?
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.
Can you also add a comment (if this is necessary and not upstreamable) explaining what this environment variable does?
(At a quick glance, it is not entirely obvious to me what the ordering should be - i.e. should this go before or after the conda env is activated, and the significance of the hard-coded path inside numpy, so if you could add an explanation that would help me and the other core team members that aren't as familiar with pkgconfig a lot)
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.
Is it possible for us to upstream something to meson/numpy to autodetect this better so that we can avoid this?
As far as Meson is concerned providing the pkg_config_path
is a standard argument to setup/configure, so I don't think there is anything left that could be done in that library. For NumPy, Ralf would know best, but I am not sure that it would be able to install the pkg-config file to the normal location on a user's disk given that would be outside the typical installation architecture of a Python 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.
I feel like sprinkling PKG_CONFIG_PATH in every workflow seems a bit hacky
It's only needed for cross-compiling, so it should not be present/needed in other workflows.
With numpy 2.0, which provides numpy-config
, this is all you need in meson.build
(and nothing in CI jobs):
dependency('numpy')
When using meson's numpy resolution, there are two ways one can go in my understanding:
I do realize that currently it looks like a hack, and in the past days I only wanted to see it work at least once, to have at least a baseline - I suspect that half of the env-declarations are useless, and could be removed. But unfortunately I still have no idea why it doesn't work - and unless some miracle happens and I have some revelation, I think I might have to give this up - unfortunately I am not able to learn the ins and outs of a non-trivial CI setup due to time constraints :( |
It's new in 2.0.0 |
default_options: ['buildtype=release', 'c_std=c11', 'warning_level=2'], | ||
) | ||
|
||
fs = import('fs') | ||
py = import('python').find_installation(pure: false) | ||
tempita = files('generate_pxi.py') | ||
versioneer = files('generate_version.py') | ||
|
||
numpy_dep = dependency('numpy', method: 'auto') |
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.
This should use required: false
as long as numpy 1.26.x is still supported
@OldManYellsAtCloud thank you for the effort, this was helpful to understand the limitations of the |
I don't have the time personally to work on this at the moment, but am happy to review any changes that come through. I do think this is close |
In some cases the numpy module might not be usable during build-time, especially when cross-compiling. (E.g. when compiling for arm32 on a x86-64 machine, the arm32 module is not usable at build time).
This makes meson fail, as it isn't able to figure out the location of numpy headers.
To allow an alternative way to find these headers, introduce a meson build option, where the location of the numpy headers can be specified.
In case numpy module cannot be loaded for some reason to query the include folder location, fall back to the value of this meson option.
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.