-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
Specify build requirements in pyproject.toml (PEP 517) #25227
Conversation
665b53c
to
6bc1d5e
Compare
@jorisvandenbossche I hope this works around the issues that were reported before. If you know of others that came with |
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.
Thanks! Added some comments.
pyproject.toml
Outdated
@@ -0,0 +1,3 @@ | |||
# When changing the version numbers here, also adjust them in `setup.py` | |||
[build-system] | |||
requires = ["setuptools", "wheel", "cython >= 0.28.2", "numpy >= 1.12.0"] |
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 we need a fixed old numpy version, depending on the python version.
At least it's what we did last time (77bfe21, and it is also what scipy did before they removed it again).
This is because the build happens in an isolated build environment (and with above syntax will always use the latest version), so if your actually installed numpy is then older, it gives problems.
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.
Ah, I'll add that to the toml
.
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.
Actually this fails in my tests as numpy==1.12.1
doesn't compile on Alpine.
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.
Actually this fails in my tests as
numpy==1.12.1
doesn't compile on Alpine.
Is that a known issue with numpy?
But not really sure how to deal with that. I think we need the pins here for the general case. It might be, that if our setup does not work for a specific use case / platform, you will need to use a --no-build-isolation
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.
Yes, this is a known issue with NumPy and was fixed in the 1.13 series. I have now pinned the Python 3.6 build also to 1.13.1 and added an explanatory comment. As Alpine+Python3.6 was the initial origin for tackling the pyproject.toml
again, I made this change.
setup.py
Outdated
# workaround bug in pip 19.0 | ||
here = os.path.dirname(__file__) | ||
if here not in sys.path: | ||
sys.path.insert(0, 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.
Why is this needed?
For versioneer?
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.
Yes, added a comment
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.
BTW, this workaround might not be needed any more, as it should be fixed in setuptools/pip (pypa/pip#6212), which will be released in the coming hours it seems
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 would keep this in here for a bit as the pip release for that was not done. (From another bug report it seems like it will be in the release coming out this weekend but I won't count on that).
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.
New Pip/Setuptools to fix this have been released in the meantime.
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.
Removed this again
Codecov Report
@@ Coverage Diff @@
## master #25227 +/- ##
=======================================
Coverage 92.15% 92.15%
=======================================
Files 166 166
Lines 52294 52294
=======================================
Hits 48194 48194
Misses 4100 4100
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25227 +/- ##
=========================================
Coverage ? 91.73%
=========================================
Files ? 173
Lines ? 52839
Branches ? 0
=========================================
Hits ? 48472
Misses ? 4367
Partials ? 0
Continue to review full report at Codecov.
|
e8791b1
to
88e0544
Compare
This might ensure it works on alpine+python3.6, but then can give errors on other platforms in case you would eg do I am not sure what the best way to go is here (although I am inclined to keep it at the lowest we support (1.12), so it works when numpy 1.12 works on the specific platform). cc @rgommers (since you were involved in the pyproject.toml file in scipy) |
Pip support should finally be good enough now with pip 19, we're adding it back in SciPy soon: scipy/scipy#8734
Yes, this is the only correct way to do it, you have to build against the lowest supported numpy version for each Python version. If that lowest version varies (e.g. Python 3.7 + numpy 1.12.x won't work), then specify multiple versions like so: PyWavelets/pywt@5e6d53a#diff-522adf759addbd3b193c74ca85243f7d |
Yes, that's what we are doing (and did before as well before we removed the pyproject.toml again .. just like scipy did as well :)). |
maybe soln is to bump numpy requirement to 1.13? or higher |
Is is really that common that people are using Pandas with a numpy version older than a year? Updating numpy has been quite smooth in the last years, so I would have expected that this is one of the simpler migrations. |
agree if bumping helps here let’s do it |
I would keep this patch for 0.25 anyway. We don't want to experiment with pyproject.toml in a bug fix release (it should not give that much troubles as last time we added it, but still) |
Indeed there isn't. There are no features of
Yes it is. I'm quite sure that that's more common than people using non-manylinux-compatible distros.
In this case, SciPy is anyway bumping to 1.13.3 as minimum numpy version for the next release. So bumping won't impact many users. So +1 for that solution. |
The |
This reverts commit faf7df7.
Does this fully close |
If we are fine with cython being a build dependency (also for sdists, as pyproject.toml cannot really distinguish from source vs from sdist builds), then the unconditional call to cythonize should not really be a problem. But as I said already before, I don't think this PR closes #25193 for 0.24.2 (we don't want to start using pyproject.toml in a bug fix release, and also the numpy version increase should go in 0.25). |
It's not that urgent to merge this I think, so we might still be able to remove it before that. |
Bumping numpy comes with a bit more complexity (esp. re: compat code) - I've started a PR dedicated to just that in #25554. |
@h-vetinari Thanks for taking care of the NumPy bump. I hope I can spend some time on this again once the bump is merged. |
Yes, it is a good idea to do the numpy version bump in a separate PR. |
The numpy bump has been merged a few days ago. :) |
@xhochy can you merge master. we are now in 1.13 min, so you can remove a lot of these changes. |
can you merge master |
https://travis-ci.org/pandas-dev/pandas/jobs/529095120
seems like pypa/pip#6434 |
Yes, there was indeed a regression with the latest pip 19.1.0 for projects using editable installs and already having pyproject.toml file (although I think pip 19.1.1 which was just released should mostly fix that) We could choose to postpone adopting a pyproject.toml somewhat more until the dust settles around it (I think people are working on a PEP update to include editable installs in PEP517/518, but that will take some more time). |
@@ -415,6 +415,7 @@ Reshaping | |||
- Bug in :func:`pandas.cut` where large bins could incorrectly raise an error due to an integer overflow (:issue:`26045`) | |||
- Bug in :func:`DataFrame.sort_index` where an error is thrown when a multi-indexed DataFrame is sorted on all levels with the initial level sorted last (:issue:`26053`) | |||
- Bug in :meth:`Series.nlargest` treats ``True`` as smaller than ``False`` (:issue:`26154`) | |||
- Bug in :func:`factorize` when passing an ``ExtensionArray`` with a custom ``na_sentinel`` (:issue:`25696`). |
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.
some merge left-overs?
setup.py
Outdated
# commit: 0f16dc307b72e613e71067b6498f82728461434a | ||
# | ||
# ensure cwd is on sys.path | ||
# workaround bug in pip 19.0 |
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 all of this can be removed now, see also discussion below
Is this a blocker for 0.25.0, or were all the blocking issue resolved elsewhere? |
I don't think this is urgent (eg #25193 which triggered, you already solved that on master) |
closing; we need to see if we actually need this. |
git diff upstream/master -u -- "*.py" | flake8 --diff
Verified this using the Dockefile: