-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
pip sometimes builds install_requires dep AFTER project, which forces cython .pxd imports to be either setup_requires (breaks cython) or pep517 (breaks wheels and therefore tox) #6406
Comments
There's not guarantee with respect to installation order when it comes to install requirements, you should not rely on it. |
Even if a dependency is installed before, there's no guarantee it will be available during the build phase (as the build may be isolated). |
Hm the isolation remark is interesting, but I'm fairly sure build isolation wasn't disabled and it never was an issue. (not sure why) Well my suggestion is to make a guarantee, so I can rely on it. That is why I made this ticket. Or is there any advantage in the current behavior of not keeping the order? As I wrote above especially the time consumption when using setup_requires or build-system.requires can be massive, 5 minutes or longer with a larger Cython dep, and this can lead to 10 minutes or more increases in build time in cross-compilation tools like https://github.com/kivy/python-for-android if they need to analyze the deps. (And there is AFAIK inherently no way to speed this up, since |
Sorry, to be more clear: the exact different use cases are 1. deps are needed for build_ext/fully building the wheel/running it (which is the case for |
So there's a lack of "this is needed for build_ext but not for any of the other basic stuff you might want to do with the package" option, and |
What basic stuff? If it's a build time dependency, it needs to be build before being installed. If you want to speed up things, build wheels, so those build time dependency don't have to be build from source. |
It really would be easier to understand what's the exact issue if you push an example or the offending project to Github and provide the exact command being used. |
How simply you just put it simply doesn't reflect the actual pep517-landscape. There's not just "build time dependency" and "needed at runtime dependency". There is also "needed just to run basic setuptools and examine package" and currently that's lumped in together with "build time dependency" |
@ example: that really depends on what you want to see. If you want to see the slow-down, you need to write code using pep517 examining a package using So the problem here really is in the nuances of package analysis of packages that are not installed without actually installing them, which historically I think was impossible but with pep517 it now is, but there slamming unnecessary stuff into setup_requires/build-system.requires really tanks the analysis time in an extremely undesirable manner |
First, there's PEP 518 and PEP 517. PEP 518 is the replacement for |
Just for clarity, I did actually mean pep517 interacting with build-system.requires/pep 518. We use pep517 for packaging analysis in python-for-android to map the deps to cross-compile patches when packaging for android Edit: but this affects really any tool that wants to be able to visualize dependencies without actually installing the package. I realize that's not super common but I'd say it shouldn't be treated as super arcane either |
Build wheels, than analyze those? |
pep517 combined with new setuptools has |
Sorry if I express all of this poorly, I got into this mostly by necessity. (I work on Cython libraries and python-for-android components that deal with this) I'm really not a packaging expert |
Because things are not declarative, there's no way to inspect source packages without involving the build systems and build dependencies because you never know when they are going to be needed (like an import in In theory, a valid source distribution |
That is a simplified view that really prevents a useful discussion. With pep517 there are different type of how far you go into the build, and you do not need to run the full build system anymore. That is new and very beneficial, but the current way |
In theory, because setuptools currently does not fill in all the metadata ( |
If "in theory" refers to getting deps without fully buliding the wheel, no that actually works, you can analyze with pep517 without building the wheel. We're doing that, I'm not making that up 😄
As for this, yes, but to put that together you also need to install build-system.requires since it makes no differentiation between basic packaging tools needed (like setuptools, poetry, ...) and stuff only needed for build_ext like |
How far you go with the build does not mean anything when 99.9% of the projects run python code in |
No, a source distribution must contain a pre-generated PKG-INFO entry. The problem is pip support non source distributions too, like VCS checkouts or snapshots, for those there's no way around the need to run |
Again, this is not theoretical. You can have |
OK, sure, you have a use case, but that's not in the PEP. |
Or are you having an issue with the particular PEP 517 implementation you're using? |
I never claimed it was @ covered by any PEP standard. The only thing I'm saying is hey, if pip installed install_require order then the world would be a lot nicer for this use case. Is this so difficult to understand? Sorry that I'm getting frustrated over here, I don't get what is so complicated about this |
No, because there's no guarantee those run time requirements must be available during any of the build phase (and they won't be if the builds are isolated). |
This is all so problematic because it seems to be a case nobody thought of (hence not specified as an option ever) but |
Build isolation is interesting, I never had that issue. Are you sure the deps are not around? Build isolation is enabled by default, isn't it? It seems pip |
Ok, here is an actual example I just tried: setup.py: from Cython.Build import cythonize
import sys
from setuptools import setup
from setuptools.command.build_ext import build_ext
class my_build_ext_hook(build_ext):
def run(self):
import PIL
print("IMPORTED PILLOW!", file=sys.stderr)
super().run()
setup(
name="test2",
version="0.1",
cmdclass={
"build_ext": my_build_ext_hook,
},
install_requires=["Pillow"],
packages=["test2"],
ext_modules = cythonize("test2/__init__.pyx"),
)
Build in isolated mode where the
I can now also use the module:
So the only issue here seems to be that only sometimes the install order isn't in dependency order. If that weren't the case it would work perfectly. I don't know why that is, but that is how things seem to be, and since order seemed to me like something that may be fixable I made this ticket! I hope this clears up things a little |
You're confusing running pip isolated ( |
And your actual example would not work when using a clean virtualenv, no? Since the Cython build time requirement is not declared anywhere. |
No Cython not being specified is not correct, I agree. But I wasn't trying to make a perfect package, just show that apparently the isolation allows access to install_requires. I just added a PEP518 /
Then I added this MANIFEST.in:
And the install still works, including uninstalling & freshly reinstalling. Or do I still need to do something else to enable build isolation? |
Scratch that, it might depending on the install order... and only if Pillow does the same. Extremely ugly, and broken. You should not need to install Cython when installing from a wheel for example, since it's a build dependency. |
Can you upload your example zip? |
Sure! Here you go |
And maybe you're right, and install_requires for this with pip changed to obey install order always (which it doesn't right now) is a horrible idea. All I'm trying to say is that from my point of view it looks like not the worst thing to try, but again I'm not a packaging expert so if you're all very certain this is nonsense then I'll take that response 🤷♀️ |
Are you sure you're using an up-to-date pip? |
Using
|
Oh, fascinating! I wasn't, I was using 18.1. Now I updated to 19.x and you're right, it doesn't work anymore. (Even when adding Cython to build-system.requires) So I suppose using install_requires for this is just the wrong idea, and I'll close the ticket. Sorry for all the confusion 😂 it looked like a reasonable approach to me from my pip version and from what I've tried, but I suppose given the new build isolation it really doesn't make any sense |
Thanks so much for all your responses! It's sad this doesn't make too much sense then, but I really learned a lot |
I do think guaranteeing the order when handling And how do you handle the case where 2 packages need different versions of the same build time dependency? |
Yeah you are right, I thought it helped with this |
Are you sure? This documentation seems to suggest otherwise: https://pip.pypa.io/en/stable/reference/pip_install/#installation-order |
@jdemeyer: yeah, there's some old code in pip that's supposed to do this, but:
So there's still no guarantee it will work, and it definitely won't help when build isolation is used. Really some of the arguments about the benefits don't make sense:
Likely, right...
Pip has rollbacks. Old code, old documentation, IMHO, it would be better if at least that part of the documentation was removed. |
Case in point, if wheel is installed, pip will first build wheels for all packages, then install them:
$ ./venv/bin/python src/pip freeze --all
pip==19.0.dev0
setuptools==41.0.0
$ ./venv/bin/python src/pip install -t tmp TopoRequires4 --no-index -f tests/data/packages
Looking in links: tests/data/packages
Collecting TopoRequires4
Collecting TopoRequires2 (from TopoRequires4)
Collecting TopoRequires (from TopoRequires4)
Collecting TopoRequires3 (from TopoRequires4)
Installing collected packages: TopoRequires, TopoRequires2, TopoRequires3, TopoRequires4
Running setup.py install for TopoRequires: started
Running setup.py install for TopoRequires: finished with status 'done'
Running setup.py install for TopoRequires2: started
Running setup.py install for TopoRequires2: finished with status 'done'
Running setup.py install for TopoRequires3: started
Running setup.py install for TopoRequires3: finished with status 'done'
Running setup.py install for TopoRequires4: started
Running setup.py install for TopoRequires4: finished with status 'done'
Successfully installed TopoRequires-0.0.1 TopoRequires2-0.0.1 TopoRequires3-0.0.1 TopoRequires4-0.0.1
$ ./venv/bin/python src/pip freeze --all
pip==19.0.dev0
setuptools==41.0.0
wheel==0.33.1
$ ./venv/bin/python src/pip install -t tmp TopoRequires4 --no-index -f tests/data/packages
Looking in links: tests/data/packages
Collecting TopoRequires4
Collecting TopoRequires2 (from TopoRequires4)
Collecting TopoRequires (from TopoRequires4)
Collecting TopoRequires3 (from TopoRequires4)
Building wheels for collected packages: TopoRequires4, TopoRequires2, TopoRequires, TopoRequires3
Building wheel for TopoRequires4 (setup.py): started
Building wheel for TopoRequires4 (setup.py): finished with status 'done'
Stored in directory: /home/bpierre/.cache/pip/wheels/94/75/42/aaf87ce968d605bb557fc354b6c60bf98590d40f6e83675489
Building wheel for TopoRequires2 (setup.py): started
Building wheel for TopoRequires2 (setup.py): finished with status 'done'
Stored in directory: /home/bpierre/.cache/pip/wheels/a1/6a/95/944f370b4d5f04b46d2ce7352dca59dca33a7aa0bcc59a3295
Building wheel for TopoRequires (setup.py): started
Building wheel for TopoRequires (setup.py): finished with status 'done'
Stored in directory: /home/bpierre/.cache/pip/wheels/cd/e5/f7/72357310b735e0151afaf2b24736c79c276ee08c32e3e98f91
Building wheel for TopoRequires3 (setup.py): started
Building wheel for TopoRequires3 (setup.py): finished with status 'done'
Stored in directory: /home/bpierre/.cache/pip/wheels/c8/43/5c/58057081e51597ec84fa61de1764cb3816854a58b003574768
Successfully built TopoRequires4 TopoRequires2 TopoRequires TopoRequires3
Installing collected packages: TopoRequires, TopoRequires2, TopoRequires3, TopoRequires4
Successfully installed TopoRequires-0.0.1 TopoRequires2-0.0.1 TopoRequires3-0.0.1 TopoRequires4-0.0.1
Target directory /home/bpierre/progs/src/pip/tmp/toporequires4 already exists. Specify --upgrade to force replacement.
Target directory /home/bpierre/progs/src/pip/tmp/toporequires3 already exists. Specify --upgrade to force replacement.
Target directory /home/bpierre/progs/src/pip/tmp/toporequires2 already exists. Specify --upgrade to force replacement.
Target directory /home/bpierre/progs/src/pip/tmp/toporequires already exists. Specify --upgrade to force replacement. |
Interesting analysis. So this isn't caused by |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Ok, so I tried to use tox, and it runs the following command:
myproject
has an install_requires dependency onwobblui
, from which it will attempt to.pxd
import things. (Cython'scimport
)Now what pip does is build the wheels in this order:
Please note this builds
myproject
BEFORE it'sinstall_requires
dependency. As a result, unsurprisingly, the build fails.Now this is somewhat problematic because:
why would pip even need to build in this order? Does this have any upside?
setup_requires not only slows down pep517 when analyzing for deps (because
get_requires_for_build_wheel
can analyze deps without any of theinstall_requires
/actually building the wheel, but will need to install all ofsetup_requires
first) but also sometimes breaks Cython(!) so is simply not an option for Cython projects: Question: odd error, disappears on second run: AssertionError: PyTypeTest on non extension type cython/cython#2730pep518/pyproject.toml's
build-system.requires
doesn't allow building wheels sometimes, and if that is used in a dependency that itself isbuild-system.requires
in tox then it just breaks downtox
making it also not a good option: some more details here Please consider undeprecating setup_requires setuptools#1742Therefore, I think pip should just not do that. Please make it build things always such that
install_requires
is build first, it brings a host of other troubles for.pxd
cross-packagecimport
s/Cython if it just jumbles the order aroundThe text was updated successfully, but these errors were encountered: