-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Python bindings: add a pyproject.toml with numpy as a build requirement #8926
Conversation
…ult, unless the GDAL_PYTHON_BINDINGS_WITHOUT_NUMPY env var is set (refs OSGeo#8069)
@rouault Have been trying to figure out how to respond on Adding a Functionally I'm not so sure that adding Numpy as a build requirement changes anything for users right now. Since the Researching this a bit more, it seems Unclear to me if the |
subscribe at https://lists.osgeo.org/mailman/listinfo/gdal-dev and check your spam folder for confirmation email
This does fix "pip install gdal[numpy]" To be honest, I'm quite beyond my mastering of Python packaging, which itself seems to be in a quite sad state, so I don't really feel like being able to do any further improvements/complications. Door open to anyone else feeling like they want to try :-) I'm a bit ambivalent about this PR as they are situations like the GDAL conda packaging which use pip options to not install dependencies and ignore installed depencies, so the "require numpy at build time" check fails, and I had to get around that with 442b785. Hopefully that's an unsual way of installing the bindings. |
Should |
how could it not be there ? It does a lot of things that a .toml file can't do... The .toml file indicates that the underlying build system is setuptools. I've taken inspiration from https://github.com/rasterio/rasterio/blob/main/pyproject.toml and https://github.com/toblerity/fiona/blob/master/pyproject.toml which do similar things |
After research and ask ppl in matrix, seems So, there is some alternatives, one keep using setuptools but with Maybe, the easiest way now if just keep the mix, while GDAL uses the new |
OK I tried the hook, and found some additional documentation showing that the settings these hooks receive are not useful. I also did some digging into the
Yes, but in exchange it forces users who just want the OGR side to install numpy or set the environment variable. I wasn't thinking about that when I suggested just making Numpy a requirement. The mailing list thread suggests the primary maintainers at least are OK with this change, and I do think most users probably already have Numpy installed in their environment.
Yep totally agree. Not at all asking you to do any additional work. I have been dealing with Python packaging a lot recently, and it is incredibly frustrating. I think the additional research I mentioned is enough to understand that this PR is probably as good as it gets.
Yeah, same. What about just providing more detailed installation instructions? I wrote the snippet below for isofit/isofit#420, but it seems like something that should be included in the GDAL documentation. I think this probably provides enough information for most users to reliably get
|
@latot IMO this is the right approach right now:
Fully moving off of |
I agree, actually would be better just get the |
@pl-kevinwurster Thanks for the proposal ! yes, a bit scary... Actually the python bindings should now always include the needed part (gdal_array module) for numpy support (unless people have built with the env variable set). The only missing thing is just to install numpy if it isn't already there (but yes I'd be unclear of what will happen when a ABI breaking numpy 2.0 will come...)
The last part of those instructions are no longer true with this PR (at least when installing through pip), are they? |
To install the version of the Python bindings matching your native GDAL library: | ||
|
||
:: | ||
|
||
pip install GDAL=="$(gdal-config --version).*" | ||
pip install gdal=="$(gdal-config --version).*" |
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.
Oh I think this does something I have not accounted for: Does the trailing .*
account for versions like 3.8.1.1
where a new version of the Python package has been released to fix some bug with the package itself? I have always done "gdal==$(gdal-config --version)".
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.
Does the trailing
.*
account for versions like3.8.1.1
yes
swig/python/pyproject.toml
Outdated
@@ -0,0 +1,32 @@ | |||
[build-system] | |||
requires = ["setuptools", "oldest-supported-numpy"] |
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.
Any reason not to just use a broad numpy
dependency? Forcing users to install the oldest supported version of Numpy supported by their version of Python doesn't seem great. They may have installed a specific version of Numpy already, and I think this could cause a downgrade?
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.
Any reason not to just use a broad
numpy
dependency?
just copying what rasterio does...
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.
ok, I've changed that to numpy
in an extra commit. I suspect rasterio does what it does to be able to do binary wheels using ancient toolchain so that they can work on the largest number of systems. Plain numpy should be fine in a GDAL context where we don't provide binary wheels
authors = [ | ||
{name = "Frank Warmerdam"}, | ||
{name = "Howard Butler"}, | ||
{name = "Even Rouault"}, |
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 see a few fields that are duplicated between setup.py
and pyproject.toml
. You should be able to introduce a setup.cfg
file and define common bits of metadata there to avoid having to update both setup.py
and pyproject.toml
. I'm pretty sure the setuptools.setup()
call in setup.py
will use the contents of setup.cfg
as defaults, and override with whatever parameters are still provided via the setup()
function. I can't think of any adverse effects in terms of maintainability, but I also have never dealt with a project containing all 3 files.
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 should be able to introduce a
setup.cfg
file
sigh, yet another config file... I'd prefer sticking with the current 2 file setup, unless we see that it is broken...
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 think that you no longer need setup.cfg
if you use
[build-system]
requires = ["setuptools>=61.0.0", "wheel"]
Almost all the metadata can be simply moved from setup.py
to pyproject.toml
leaving in setup.py
only the part defining the Extensions
and the logic that cannot be expressed in a declarative manner.
@rouault Oh yeah. Sorry, I should have clarified that the instructions are written for users attempting to install the If these |
you mean, if we merge this PR ?
sounds good |
AFAIK normally the python packages are built using "build isolation".
|
…oldest-supported-numpy", "wheel"]
ok, I've reverted to that, as well as adding ["setuptools>=61.0.0", "wheel"] to requires |
In https://github.com/avalentino/gdal/tree/fix_8069 I have committed a change to move all the static(-ish) metadata form I have also noted that both Finally there is another |
thanks. I've just done it, with an extra commit to change from 'python setup.py sdist' to 'python -m build . --sdist', but I see that in swig/python/CMakeLists.txt we do also other invokations of setup.py directly. I believe further changes would be needed
I'm not sure... scripts was historical, and entry-points is a recent addition. I'd prefer not modifying that at that time if possible.
when the dust has settled on this PR... :-) |
@avalentino Your changes borke "python setup.py install".
I reproduce locally too doing "make install DESTDIR=/tmp" |
unfortunately that's not enough to fix it. I'm going to revert most of your changes, as I feel them too risky. The subset I've kept is in 80a147b |
I will have another look later today.
is |
ah, no, I was at 60.0. But even when upgrading, I run into issues when running setup.py |
I'm not able to reproduce locally on ubuntu 23.10 (setuptools v68.1.2). |
@rouault Looking good. I'll get you some documentation early next week. As for potentially breaking |
for those following this PR, we have a new related PR in #9405 . I don't see anything obviously wrong to me in it, but thoughts appreciated |
Fixes #8069.
Hopefully a compromise that will satisfy most needs...
Demo: