Open up plat/abi/impl options to `install --target` #5404

lorencarvalho · 2018-05-14T16:12:49Z

Summary:

Move dist restriction options to be re-usable (between install/download)
Make dist restriction options usable in install (exclusively with --target)
Add a check_supported_wheels bool to RequiriementSet for non-resolved (full path) dependencies

Greetings pip maintainers!

This is an implementation of the suggestions in #5355 - @pfmoore suggested that allowing a user to circumvent the local compatibility checks would be allowable only in the case of using --target, and I agree completely.

One part that may be controversial is that this diff also adds the --platform, --abi, --python & --implementation args to the install command. Though it is constrained to only if --target is being used. This is purely for convenience sake, so that users who wish to install with --target don't have to make multiple invocations of pip (e.g. pip download and then pip install --target $dir ./wheel1.whl ./wheel2.whl etc).

One thing I'm a little iffy on is the test suite, I looked at similar tests and tried to adhere to their style and convention, but I needed an additional wheel and I'm not certain how they were created (the wheels with mocked pep425 names). What I ended up doing was copying one of them and simply changing the file's name. I imagine there's a better way?

Here's some examples of the UX:

using --target and dist options:

pip install \
--target /tmp/test \
--python-version 36 \
--platform manylinux1_x86_64 \
--abi cp36m \
--implementation cp \
--only-binary :all: \
Cython numpy

output:

Collecting Cython
  Using cached https://files.pythonhosted.org/packages/19/eb/c4d9f3beafd5ac0615936860bcee41d93ca58f8734a16715da0037d2c468/Cython-0.28.2-cp36-cp36m-manylinux1_x86_64.whl
Collecting numpy
  Using cached https://files.pythonhosted.org/packages/71/90/ca61e203e0080a8cef7ac21eca199829fa8d997f7c4da3e985b49d0a107d/numpy-1.14.3-cp36-cp36m-manylinux1_x86_64.whl
Installing collected packages: Cython, numpy
Successfully installed Cython-0.28.2 numpy-1.14.3

trying to use dist options w/o specifying --target:

pip install \
--python-version 36 \
--platform manylinux1_x86_64 \
--abi cp36m \
--implementation cp \
--only-binary :all: \
Cython numpy

output:

ERROR: Can not use any platform or abi specific options unless installing via '--target'

All the usual dist-related error messages are preserved, for example, not specifying --only-binary:

pip install \
--target /tmp/test \
--python-version 36 \
--platform manylinux1_x86_64 \
--abi cp36m \
--implementation cp \
Cython numpy

ERROR: When restricting platform and interpreter constraints using --python-version, --platform, --abi, or --implementation, either --no-deps must be set, or --only-binary=:all: must be set and --no-binary must not be set (or must be set to :none:).

So as you can see, it's not a terribly drastic change in terms of behavior, but does alter the --help output of install and opens up potential avenues for users to break things in interesting ways, so I understand any reticence to the change and am very receptive to ideas on how to strike a balance between ease-of-support, ease-of-use & this functionality.

Looking forward to discussion
-Loren

kornpow · 2018-05-15T17:57:06Z

I commented on #5355 also, but wanted to repost here too. This feature sounds like exactly what I need!

Hi I have been fighting basically this issue for a while now. I am using the Resin.io service, which uses a cross-compiling build system in order to build a docker container for a Raspberry Pi device. However their build systems run on armv8l, and I'm looking to install for the armv6l for the Pi Zero W. I've knocked into this issue multiple times working with this platform. I think in terms of ARM wheels, Docker containers, and cross compilation, where there is more weirdness and incompatibilities between architectures something like the above solutions would be a great improvement.

lorencarvalho · 2018-05-15T21:14:05Z

I tried running the test suite on a windows machine and couldn't reproduce the errors from AppVeyor, not sure what's going on there if anyone has an insight I'm all ears

pradyunsg · 2018-05-16T08:43:43Z

Might have been a transient issue?

pradyunsg · 2018-05-16T08:44:36Z

(closed-opened to re-trigger the builds)

lorencarvalho · 2018-05-21T20:29:37Z

👋 just offering a friendly ping

pradyunsg · 2018-05-22T05:20:47Z

Hey @sixninetynine! :)

I don't have the bandwidth currently to review this PR (or any non-trivial PR for that matter). I'll get to this soon though. Sorry to keep you waiting.

lorencarvalho · 2018-05-31T21:06:41Z

@pfmoore would love your opinion on the discussion happening in #5453 and how it relates to this PR. Specifically the notion that merely including dist options in the install command accounts for the explicitness of the request.

I'm perfectly happy removing the --target requirement (and in fact, I think it makes the diff a little cleaner), but of course I defer to the maintainers.

cc @chriskuehl & @asottile

pfmoore · 2018-05-31T21:32:05Z

Personally, I see the two use cases as different. This PR is saying "I want to manage packages for a specific platform/abi/impl, which is not the same as the current environment", whereas #5453 is about allowing the user to describe the current environment's compatibility in terms of non-default unique tags (ones that don't match any of the standard sets). As the use cases differ, I'd rather the UIs differed as well.

To put it another way, I see this PR as being about allowing certain carefully chosen commands to call pep425tags.get_supported() with non-default platform/abi/implementation values. But #5453 is about wanting pep425tags.get_platform() to return a custom value. My feeling is that a plain pip install (that installs code which will be imported into the current environment) should always ensure that what is installed matches pep425tags.get_supported() with default arguments. That's the best way we have of ensuring that what's installed is executable.

I believe there have been previous discussions about having finer-grained ways of describing your environment, fitting between manylinux (highly generic) and "linux" (basically machine-specific). There may even be a mechanism in place. I don't have specifics, unfortunately (see below - I don't really know much about this area of Linux). That sounds pretty close to what #5453 is trying to achieve.

Disclaimer: Personally, I use Windows and #5453 is not really an issue there. So I'm probably not the best person to have an opinion here. My comments above are from a general perspective of making sure we get pip's UI right, but someone with a Linux background may feel differently.

asottile · 2018-05-31T21:38:07Z

I'm not sure they're all that different. I believe (correct me if I'm wrong) the primary usecase of this PR is to be able to build a site-packages from a disparate architecture / platform and deploy it (think: running pip on macos, but zipping up the site-packages and then running it in aws lambda).

On darwin, manylinux1_x86_64 would be just as different to the list of pep425tags.get_supported() as would linux_ubuntu_16_04_x86_64 on the return value of a linux_x64_64 machine.

pfmoore · 2018-05-31T21:46:03Z

@asottile Yes, but the point is that you specify a target directory, precisely because the stuff you want to zip up is incompatible with what's in site-packages. #5453 is, as I understand it, about describing what you consider to be compatible with site-packages.

asottile · 2018-05-31T21:50:37Z

One of the usecases also mentioned in #5453 is identical to this one -- being able to build a virtualenv for an alternate platform direct link to the discussion also linked there

pfmoore · 2018-05-31T22:07:05Z

I see this PR as a better solution to that problem. I'm pretty strongly of the opinion that being able to do

$ pip install --platform linux foo
$ pip install --platform win_amd64 bar

which puts mutually incompatible binaries into site-packages, is wrong. Doing the same with --target is OK, because you have to deliberately choose to use the same target directory, and you can't import from the target location without deliberately choosing to do so.

Basically, I'd rather see #5453 be implemented as a configuration, that says "this environment conforms to platform tag XXX". That's my impression of how previous discussions on this topic have gone.

But as I've said, this is a Linux-related issue, and I'm not a Linux specialist, so I've said what I want to - from this point, I'll leave it to others to decide on how to proceed.

asottile · 2018-05-31T22:58:16Z

I was under the impression that the same requirements that were added for pip download were explicit enough here (as to not require a more complex UX here).

That is, I think it's very explicit that running pip install --platform ... --only-binary :all: ... you're asking pip to do a very specific action -- yes most of the time for the specific usecase (that both issues touch on) you'd want to use --target.

I agree that it's possible for someone to put mutually incompatible binaries into site-packages.
But that's not different from what's being proposed here as someone could just as easily pip install --target ... --platform X && pip install --target ... --platform Y in just the same way. This is also the status quo for the very same pip download command (which itself doesn't require a --target either):

$ pip download --only-binary :all: --platform macosx_10_10_x86_64 numpy
...
Successfully downloaded numpy
$ pip download --only-binary :all: --platform manylinux1_x86_64 numpy
...
Successfully downloaded numpy
$ ls
numpy-1.14.3-cp27-cp27m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl
numpy-1.14.3-cp27-cp27m-manylinux1_x86_64.whl

What I hope to convince is that requiring --target doesn't actually solve the potential mixing problem described. But that's ok! Because a user would have to be type a very-explicit incorrect command to end up there in the first place.

An aside: if this is implemented without requiring --target this also probably partially solves #3689.

kornpow · 2018-06-01T14:10:39Z

Wanted to give a little more information about how I am currently getting around this problem.
Problem: Install python program from Dockerfile, where the builder of the Docker container could be ARMv8 or ARMv6, and ARMv6 is the target.

arch_test.py:
This program checks build architecture, and if it isnt armv6 it renames the wheel to trick pip install
arch_test.py.txt

Install command:

pip3 download -r /requirements.txt --only-binary=:all: --python-version 34 --implementation cp --abi cp34m --platform=linux_armv6l --extra-index-url https://www.piwheels.org/simple -v -d /usr/src/app/wheels1 && python3 arch_test.py /usr/src/app/wheels1/ && pip3 install /usr/src/app/wheels1/*

Not sure if that helps anything. I'd like to specify --platform during pip install even if it meant install to another folder and not site-packages. Then in my code I could use site.addsitedir(target_dir)

Ideal use case:

pip3 download -r /requirements.txt --only-binary=:all: --python-version 34 --implementation cp --abi cp34m --platform=linux_armv6l --extra-index-url https://www.piwheels.org/simple -v -d /usr/src/app/wheels1 && pip3 install /usr/src/app/wheel1/* --python-version 34 --implementation cp --abi cp34m --platform=linux_armv6l --target /usr/src/app/builder-packages

lorencarvalho · 2018-06-04T18:54:32Z

OK, I just want to summarize the current status of this PR and our options...

This PR, as it stands, adds the "dist" options (--platform, --implementation, --abi, --python-version) to the install subcommand only when --target is also in use. This is a simple implementation of the feature I requested in #5355. The restriction to --target was requested there and I have no qualms with it since that fits my use case.

@chriskuehl, @asottile & @sako0938 have since commented regarding a similar feature request #5453 and suggest that this feature would be more useful if not restricted only to the --target use case. The argument being that merely using the dist options alone is explicit enough of a signal to the user, and using --target unnecessarily limits the usability of the feature.

I have no qualms updating this PR to remove the restriction on --target. I also have no qualms adding the dist options to the wheel subcommand (as requested in #5453), and in fact it'd be easy since I moved the dist options to already be resuable in cmdoptions.py -- but, I'd like some more input from pip maintainers, as it seems we are at a bit of an impasse.

Thanks in advance!

lorencarvalho · 2018-06-12T17:50:34Z

I started a thread on distutils-sig to discuss the implications of this PR and what direction to go in.

Quoting @ncoghlan :

The basic idea seems like a good one to me, and starting out with the --target restriction doesn't hurt - it's much easier to relax restrictions like that later than it is to figure out whether or not you've correctly handled all the edge cases that arise without them.

I think this sounds good, especially the point about it being easier to loosen restrictions later.

The main automatic implication I would personally expect to see is that specifying any of those options also imply "--only-binary :all:" (rather than requiring it to be passed in separately). That could wait to a follow-up PR though (since the current PR just throws an error in that case).

I think this is ok as well, given that using these commands without specifying --only-binary raises the usual (expected) error.

lorencarvalho · 2018-06-17T23:19:31Z

@pradyunsg offering a friendly ping, given the above updates

pradyunsg

Appreciate the ping. Just read up on this PR and the related discussions.

LGTM functionally, based on a visual review of the diff on GitHub's UI. I'll take it for a spin sometime today.

pradyunsg · 2018-06-13T05:57:35Z

src/pip/_internal/cmdoptions.py

@@ -60,6 +61,42 @@ def getname(n):
        )


+def check_dist_restriction(options, check_target=False):
+    """Function for determinming if custom platform options are allowed.


pradyunsg · 2018-06-18T01:54:42Z

src/pip/_internal/cmdoptions.py

@@ -406,6 +443,61 @@ def only_binary():
    )


+def platform():
+    return Option(


The pattern in this file is to use functools.partial for this unless there's a mutable value involved. Staying consistent with that would be nice.

pradyunsg · 2018-06-18T01:58:54Z

src/pip/_internal/commands/install.py

@@ -13,6 +13,7 @@
 from pip._internal.exceptions import (
    CommandError, InstallationError, PreviousBuildDirError,
 )
+from pip._internal.index import FormatControl


Unnecessary import.

As a side note, could you rebase on master? There's a bunch of CI related improvements (including reenabling checking of unused imports).

pradyunsg · 2018-06-18T02:01:16Z

tests/functional/test_install.py

+    assert distinfo in result.files_created
+
+    # Test install without --target
+    result = script.pip(


This result isn't being used in any way. I'd suggest adding a check for the exit code immediately after.

Out the back of my head, I think expect_error lets a test pass even if there's no errors. If that's the case, the above would be needed anyway. :)

pradyunsg · 2018-06-18T02:01:36Z

tests/functional/test_install.py

+    assert distinfo in result.files_created
+
+    # Test a full path requirement (without --target)
+    result = script.pip(


pradyunsg · 2018-06-18T02:10:56Z

src/pip/_internal/cmdoptions.py

+        not options.ignore_dependencies
+    )
+
+    if dist_restriction_set and no_sdist_dependencies:


I know it wasn't there originally but adding a comment that explains why these error messages are raised would be appreciated.

pradyunsg · 2018-06-18T02:15:25Z

src/pip/_internal/cmdoptions.py

+    ])
+
+    binary_only = FormatControl(set(), {':all:'})
+    no_sdist_dependencies = (


naming: sdist_dependencies_allowed

lorencarvalho · 2018-06-20T01:56:26Z

@pradyunsg thanks for the review! updated the diff per your comments and rebased as well.

lorencarvalho · 2018-06-26T23:36:17Z

@pradyunsg any chance this could make it into #5516 ? :)

pradyunsg · 2018-06-27T10:12:24Z

I'm not 100% comfortable being the only one to review and merge since I'm not completely familiar with the nuances involved here. Maybe someone else from @pypa/pip-committers who's more familiar could review this.

As for making into 18.0, I don't think we'd block 18.0 for this -- I'm fine if this has to wait until 18.1. It's definitely nicer if it doesn't have to wait. :)

pfmoore · 2018-07-24T14:52:18Z

159 additional commits - did a rebase get done incorrectly?

lorencarvalho · 2018-07-24T14:54:48Z

@pfmoore haha, yep, I royally borked it up. Fixing now

BrownTruck · 2018-07-30T08:00:04Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

lorencarvalho · 2018-08-03T18:03:24Z

@pfmoore @pradyunsg before I resolve these conflicts, what are the next steps for this PR?

pradyunsg · 2018-08-03T22:00:23Z

I can't speak for @pfmoore -- I just haven't find the time to take a deeper dive to actually review this properly; until now.

These changes as they stand, look good to me.

pfmoore · 2018-08-04T07:57:18Z

Changes look OK to me

pfmoore

Looks OK once merge conflicts resolved

pradyunsg · 2018-08-04T09:37:35Z

Cool.

So, the next step is to fix the merge conflicts and ping us. :)

* Move dist restriction options to be re-usable (between install/download) * Make dist restriction options usable in `install` (exclusively with --target) * Add a check_supported_wheels bool to RequiriementSet for non-resolved (full path) dependencies

lorencarvalho · 2018-08-05T22:07:30Z

thanks @pradyunsg & @pfmoore
I appreciate y'all being so patient with me, sorry for any pestering :)

chadrik · 2018-08-13T19:59:53Z

This fixes exactly the issue I was just trying to solve. When can we expect a new release of pip?

Thanks for all the hard work!

benoit-pierre · 2018-08-13T20:10:43Z

@chadrik: October.

lorencarvalho · 2018-08-13T22:28:17Z

@chadrik you can always install from master too:

python3 -m pip install --upgrade git+git://github.com/pypa/pip.git#egg=pip

chadrik · 2018-08-13T22:32:36Z

yup, already on it! Thanks for the quick feedback.

…

On Mon, Aug 13, 2018 at 3:28 PM Loren Carvalho ***@***.***> wrote: @chadrik <https://github.com/chadrik> you can always install from master too: python3 -m pip install --upgrade git+git://github.com/pypa/pip.git#egg=pip — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5404 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD3E0EM4g9eOu4FMjZxK7fnX7F68P9pks5uQf2IgaJpZM4T-F0l> .

pradyunsg · 2018-08-14T03:34:20Z

Thanks for the PR @sixninetynine! Much appreciated. :)

lock · 2019-06-01T18:46:27Z

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.

lorencarvalho force-pushed the feature/platforms_for_target branch from 4ffa9df to 7b56d46 Compare May 14, 2018 16:19

pradyunsg closed this May 16, 2018

pradyunsg reopened this May 16, 2018

pradyunsg added the S: needs triage Issues/PRs that need to be triaged label May 16, 2018

lorencarvalho mentioned this pull request May 23, 2018

Support creating pyz files for different platforms linkedin/shiv#7

Closed

lorencarvalho mentioned this pull request May 31, 2018

Proposal: Add --platform flag to pip install and pip wheel #5453

Open

pradyunsg reviewed Jun 18, 2018

View reviewed changes

pradyunsg added type: feature request Request for a new feature and removed S: needs triage Issues/PRs that need to be triaged labels Jun 19, 2018

lorencarvalho force-pushed the feature/platforms_for_target branch from 9fda856 to 16c8798 Compare June 19, 2018 18:17

BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 24, 2018

lorencarvalho force-pushed the feature/platforms_for_target branch from 372a8ff to 369deee Compare July 24, 2018 14:54

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 24, 2018

lorencarvalho force-pushed the feature/platforms_for_target branch 2 times, most recently from 8bac0fb to f0777e8 Compare July 24, 2018 15:26

BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 30, 2018

pradyunsg added this to the 18.1 milestone Aug 3, 2018

pfmoore approved these changes Aug 4, 2018

View reviewed changes

lorencarvalho force-pushed the feature/platforms_for_target branch from f0777e8 to d86dc8d Compare August 5, 2018 20:46

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 5, 2018

lorencarvalho force-pushed the feature/platforms_for_target branch from d86dc8d to cddcb14 Compare August 5, 2018 21:13

pradyunsg merged commit 20127bf into pypa:master Aug 12, 2018

pradyunsg mentioned this pull request Oct 5, 2018

Feature request: ability to override pep425 checks #5355

Closed

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 1, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 1, 2019

Open up plat/abi/impl options to install --target #5404

Open up plat/abi/impl options to install --target #5404

Conversation

lorencarvalho commented May 14, 2018

kornpow commented May 15, 2018

lorencarvalho commented May 15, 2018

pradyunsg commented May 16, 2018

pradyunsg commented May 16, 2018

lorencarvalho commented May 21, 2018

pradyunsg commented May 22, 2018

lorencarvalho commented May 31, 2018

pfmoore commented May 31, 2018

asottile commented May 31, 2018

pfmoore commented May 31, 2018

asottile commented May 31, 2018

pfmoore commented May 31, 2018

asottile commented May 31, 2018

kornpow commented Jun 1, 2018 • edited Loading

lorencarvalho commented Jun 4, 2018 • edited Loading

lorencarvalho commented Jun 12, 2018

lorencarvalho commented Jun 17, 2018

pradyunsg left a comment

Choose a reason for hiding this comment

pradyunsg Jun 13, 2018

Choose a reason for hiding this comment

pradyunsg Jun 18, 2018

Choose a reason for hiding this comment

pradyunsg Jun 18, 2018

Choose a reason for hiding this comment

pradyunsg Jun 18, 2018

Choose a reason for hiding this comment

pradyunsg Jun 18, 2018

Choose a reason for hiding this comment

pradyunsg Jun 18, 2018

Choose a reason for hiding this comment

pradyunsg Jun 18, 2018

Choose a reason for hiding this comment

lorencarvalho commented Jun 20, 2018

lorencarvalho commented Jun 26, 2018

pradyunsg commented Jun 27, 2018

pfmoore commented Jul 24, 2018

lorencarvalho commented Jul 24, 2018

BrownTruck commented Jul 30, 2018

lorencarvalho commented Aug 3, 2018

pradyunsg commented Aug 3, 2018

pfmoore commented Aug 4, 2018

pfmoore left a comment

Choose a reason for hiding this comment

pradyunsg commented Aug 4, 2018

lorencarvalho commented Aug 5, 2018

chadrik commented Aug 13, 2018

benoit-pierre commented Aug 13, 2018

lorencarvalho commented Aug 13, 2018

chadrik commented Aug 13, 2018 via email

pradyunsg commented Aug 14, 2018

lock bot commented Jun 1, 2019

Open up plat/abi/impl options to `install --target` #5404

Open up plat/abi/impl options to `install --target` #5404

kornpow commented Jun 1, 2018 •

edited

Loading

lorencarvalho commented Jun 4, 2018 •

edited

Loading