Install with --no-binary ignores PEP 517 build system

[takluyver]

Environment

• pip version: 19.0.1
• Python version: 3.7
• OS: Linux

Description

In a clean environment, run:

pip install --no-binary :all: flit

The output includes this:

Skipping bdist_wheel for flit, due to binaries being disabled for it.
...
  Running setup.py install for flit ... error
    Complete output from command /home/takluyver/miniconda3/envs/pip19/bin/python -u -c "import setuptools, tokenize;__file__='/tmp/pip-install-ud6iwhp3/flit/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" install --record /tmp/pip-record-wsv_33qq/install-record.txt --single-version-externally-managed --compile:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    ModuleNotFoundError: No module named 'setuptools'

Pip 19 is aware of PEP 517, and uses it if you do pip wheel --no-binary :all: flit (the output includes Building wheel for flit (PEP 517) ... done). But when installing from an sdist, it seems to default to running setup.py install instead of building a wheel using the PEP 517 backend and installing that.

This is a problem in practice because pip does respect the PEP 518 build dependencies and create an isolated build environment, without setuptools. Setuptools is not required to build with the PEP 517 backend, but it is if you use the fallback setup.py script.

It appears that the no-binary option disables the 'build a wheel and install that' pathway. But I think that option is generally taken to mean 'download the sdist rather than the published wheel'. The plan is for all PEP 517 installations to work by building a wheel and then installing that.

[takluyver]

From pypa/flit#245

[cjerdonek]

See this comment #6163 (comment) and this thread for a related discussion: https://discuss.python.org/t/pep-517-backend-bootstrapping/789

[takluyver]

I think this is separate from the bootstrapping issue (which I don't think currently affects Flit), but I suspect @pfmoore's comment that "--no-binary :all: has significant unintended consequences" is probably significant.

The problem appears to be that pip isn't even trying to use the PEP 517 backend, because it interprets the --no-binary option to mean 'install directly without building wheels', which isn't an operation PEP 517 allows.

[cjerdonek]

I'll change this to "needs triage" for now because PEP 517 isn't my bailiwick. (I don't know if this is a case of "hasn't been implemented yet" or if something hasn't been implemented correctly. Either way it should probably be failing more gracefully.)

[pfmoore]

The problem appears to be that pip isn't even trying to use the PEP 517 backend, because it interprets the --no-binary option to mean 'install directly without building wheels', which isn't an operation PEP 517 allows.

Yes, if that is what's happening, it's unintended. --no-binary should (IMO) only mean "don't consider prebuilt binaries", not "don't build binaries as part of the install process". But I don't fully understand the use cases behind --no-binary, so if someone else wants to chip in to say different, then that's fine (but like you say, I don't see any other way PEP 517 could operate, given there's no "install from source" hook by design).

[pfmoore]

Quick check of the code, it looks like this is the problem.

InstallCommand.run creates a WheelCache using the format_control option. The wheel cache (in pip._internal.cache) seems to use get_allowed_formats to decide what to serve back from the cache, and I presume that as --no-binary is set, it's refusing to serve wheels.

I don't know how to fix this, and I don't want to go for an obvious "quick fix" approach. My instinct is that it's insane to ever have an instance of a wheel cache class that can't return wheels - so I can only presume that my understanding of this code is incomplete. I don't know who wrote that code but if anyone who understands it (@pradyunsg?) can explain the intention and/or suggest a fix, that would be great. I'll try to find some time to investigate in more detail, but I really don't have much free time at the moment.

[cjerdonek]

It looks like #2675 is the source of the --no-binary option, with associated PR #2699.

[takluyver]

The use cases I'm aware of for --no-binary are all avoiding the pre-built wheel. For instance, people building packages for another packaging system want to make sure that any build steps take place in their build environment. I don't know of any use case where you need to avoid building a wheel, except to work around odd packaging bugs.

From your description, I suspect it's a historical accident. The cache machinery is designed for a persistent cache, which may already hold wheels you don't want to use at the moment. I derived the 'ephemeral cache' fairly recently, to hold wheels built, used and discarded in one run of pip.

[takluyver]

Looks like I was entirely wrong, because the original motivation was:

some projects simply don't work when installed via Wheel... we'll need some method for end users to opt out of, on a per project basis, the auto wheel building.

Given that PEP 517 projects can only be installed by building a wheel, maybe we have to say that the flag means different things for a project built by a PEP 517 backend and a setup.py project. I can't imagine that will be fun to implement. 😞

[cjerdonek]

I don't know of any use case where you need to avoid building a wheel, except to work around odd packaging bugs.

Actually, that looks to be precisely the use case they were targeting when --no-binary was first introduced. From #2699 (comment):

With wheel autobuilding in place a release blocker is having some granular way to opt-out of wheels for known-bad packages. This patch introduces two new options: --no-binary and --only-binary to control what archives we are willing to use ...

[pfmoore]

Yep, that looks pretty much like a direct conflict between PEP 517 and --no-binary. And more worryingly, PEP 517 directly envisions a world where everything is installed via wheels, so those "known-bad" packages that don't work from wheels are never going to work with PEP 517.

The big question then is whether we desupport projects that cannot be installed via wheel. IMO, we probably should (or we'll never be able to make significant progress towards reliable static metadata among other things) but we'll need to handle that desupport carefully (recent deprecations like dependency-links haven't been handled ideally and we need to be seen to be improving in that regard if we want to keep credibility).

I've created this thread for further discussion.

[pradyunsg]

--no-binary and --only-binary are related to how the package is built / installed -- --no-binary projects are installed skipping the build-a-wheel process entirely.

it's insane to ever have an instance of a wheel cache class that can't return wheels

It's more like, "hey, this package should not be going through a wheel so the cache won't expose those". It's just another case of intertwined responsibilities in the codebase.

Ideally, Cache won't contain FormatControl instances but I didn't have the time to clean that up because something else with a higher priority had come up back then IIRC.

[pfmoore]

@pradyunsg whoops, you took what I said a little out of context. I said "My instinct is that it's insane..." What I was getting at was precisely that it seems unlikely that it was done without a reason, but there's a reasonable probability that it's just mixed responsibilities and a bit of "good enough for now, tidy it up later", as you confirmed. Sorry if it sounded like I was criticising. The mistake was mine, in blindly using the cache without checking (my excuse for doing so was that the name looked like it did what I thought, so I didn't check further).

But as Chris identified, the original intention actually was to prohibit "build from wheel", so we got to the motivating requirement, which was the main question.

[pradyunsg]

Sorry if it sounded like I was criticising.

No no. I didn't take it as a criticism. 🙈 (yay miscommunication) You have nothing to be sorry for.

---

The point I wanted to make was, I hadn't changed that behavior due to the initial functionality expectations (as @cjerdonek found from the tracker) -- the current behavior, post the refactor, is weird but in line with the initial motivation for introducing the functionality. I didn't bother elaborating upon/reiterating the original motivation for adding that, since it had already been figured out. :)

None the less, I think I should have spent some more time making sure that my own tone wasn't so rough -- since I was basically just adding a neutral informational point, not reacting semi-defensively -- in that previous comment.

[pfmoore]

Ha - one day we'll meet in person and realise that we never actually understood anything the other person was talking about :-)

[pradyunsg]

Ha - one day we'll meet in person and realise that we never actually understood anything the other person was talking about :-)

😆 Perhaps.

[ncoghlan]

Even when building with --no-binary :all: without PEP 517, it still seems reasonable to interpret/implement that as --prefer-binary for a dedicated temporary cache set up specifically for that run. That way if something is both a build dependency and a runtime dependency, it only gets built once.

[dstufft]

Yea, --no-binary should only be able what format we source a package from, not whether or not we route things through wheels to begin with, or whether or not we cache them.

[seppeljordan]

I think I found the issue.

This issue seems to occur only if you try to install a package that has a dependency to a "PEP 517" package but not if you install a package that itself is "PEP 517". This led me to inspect the InstallRequirement class.
If you go and check the install method of that class it becomes clear that there is a code path missing for requirements that use PEP 517.

pip/src/pip/_internal/req/req_install.py

Line 895 in a240a98

def install(

I guess there needs to be a statement that builds the wheel form source if pep517 is detected and then copies the wheel files over to the desired location similarly as we do when we install a requirement from a wheel.

[seppeljordan]

So I submitted a PR #6606 for pip that solves the issue (at least for me). Now there should be some tests to make sure that my fix works properly and continues to do so in the future. Would anybody be willing to write those tests or at least help me with writing them? I am not familiar with testing in pip and quite frankly, I don't know much about the assumptions about the inner workings of pip.

[pradyunsg]

You can look at tests/integration and find a file with pep517 in it's name.

You can use those tests as reference for writing new tests here. :)

[pradyunsg]

Shout-out to @seppeljordan for actually fixing this! ^.^