Improve wheel support in pex. #388

MarkChuCarroll · 2017-05-15T17:04:56Z

Wheel handling in pex files is broken. The wheel standard says
that wheel files are not designed to be importable archives,
but pex treats them as if they are. This causes many standard
compliant wheels, including things like tensorflow and opencv,
to fail to import in pexes (and thus in pants).

This change modifies wheel handling, so that when a wheel
is added to a pex, it's installed in an importable form.

MarkChuCarroll · 2017-05-22T19:49:29Z

Ping? It's been a week - any chance anyone can look at this soon? @jsirois?

kwlzn

this seems like a reasonable first step towards wheel installation improvements, mod one comment.

if possible, it would also be nice to ensure that pex(1)-built pex files also net this improvement (they currently do not appear to afaict) so that there's consistency between pex and pants for e.g. testing and repros.

kwlzn · 2017-05-22T21:01:09Z

pex/pex_builder.py

+    # wheel dir.
+    if dist_name.endswith("whl"):
+      wf = WheelFile(path)
+      wf.install(overrides=self._get_installer_paths(dist_name), force=True)


seems to me like this conditional block should be mutually exclusive with the zip explosion below.. or else land in an isolated install path?

otherwise you could end up with dirs containing both installed and unzipped wheels, which could be problematic if e.g. inner paths collide?

or possibly just short-circuit with return CacheHelper.zip_hash(wf.zipfile)

I tested this a bit and noticed that the wheel-installed files will be ignored if they are not also written directly to the chroot via self._chroot.write. The Chroot class maintains an internal set of written files (by label) in Chroot.filesets and files that are included in this data structure get written out to the pex zip in Chroot.zip . So any files installed directly by wheel that are not also installed in the normal path below will be ignored.

There doesn't appear to be a clean way to update chroot.filesets without writing directly, so perhaps the best approach here is to install the wheel into a temporary directory, then read the installed RECORD file and write each entry to the chroot?

dpkp · 2017-05-26T02:35:14Z

pex/pex_builder.py

+    """
+    base = os.path.join(self.path(), self._pex_info.internal_cache, dist_name)
+    return {
+      'purelib': os.path.join(base),


nit: could drop os.path.join here and just use base

dpkp · 2017-05-26T19:00:58Z

I am unable to get console scripts to work with this wheel install (amended to skip the additional direct zip install). using ansible for example, the current pex builder generates

.deps/ansible-2.2.2.0-py2-none-any.whl/ansible-2.2.2.0.data/scripts/ansible

while this PR generates

.deps/ansible-2.2.2.0-py2-none-any.whl/bin/ansible

and if I try to build a pex using this console script, I get an error:

pex ansible==2.2.2.0 -o ansible -c ansible

# dpowers at dpowers-m01.local in ~/Code/pex on git:PR388 ✖︎ [11:58:04]
→ ./ansible
Traceback (most recent call last):
  File ".bootstrap/_pex/pex.py", line 360, in execute
  File ".bootstrap/_pex/pex.py", line 288, in _wrap_coverage
  File ".bootstrap/_pex/pex.py", line 320, in _wrap_profiling
  File ".bootstrap/_pex/pex.py", line 401, in _execute
  File ".bootstrap/_pex/pex.py", line 430, in execute_script
_pex.pex.NotFound: Could not find script ansible in pex!

again, this is only if I modify the PR to not also unpack the wheel as a zipfile directly).

MarkChuCarroll · 2017-05-29T21:10:28Z

Some context on this update: this properly uses WheelFile.install to extract the contents of a wheel, and then copies that into the pex chroot. I think this is a much better solution than the first try.

The alternative approach, which I think would work, but be more brittle, would be to add a wheel-based finder (including an override of the ImpImporter from pkgutils). That would allow us to include the wheel, in its original pypi form, in the pex. The catch, and the reason why I prefer this approach, is that we'd basically be reproducing some logic from the pkgutils WheelFile.install in the finder, and that would require us to make sure that we update pex logic after any changes to pkgutils. The approach in this change just uses wheelfile, and then pulls in the lesult.

Wheel handling in pex files is broken. The wheel standard says that wheel files are not designed to be importable archives, but pex treats them as if they are. This causes many standard compliant wheels, including things like tensorflow and opencv, to fail to import in pexes (and thus in pants). This change modifies wheel handling, so that when a wheel is added to a pex, it's installed in an importable form.

(Code disagreed about whether scripts were stored in /bin or /scripts.)

…ty with python 3.

MarkChuCarroll · 2017-05-30T20:22:44Z

This passes on my laptop with Python 2.6. Not sure what Jenkins' problem is.

dpkp · 2017-05-31T00:38:20Z

Nice updates! My local testing is raising exception during build w/ python2 pex:

PR388> pex ansible==2.2.2.0 -o ansible -c ansible
Traceback (most recent call last):
  File "/Users/dpowers/Library/Python/2.7/bin/pex", line 11, in <module>
    sys.exit(main())
  File "/Users/dpowers/Library/Python/2.7/lib/python/site-packages/pex/bin/pex.py", line 578, in main
    pex_builder = build_pex(reqs, options, resolver_options_builder)
  File "/Users/dpowers/Library/Python/2.7/lib/python/site-packages/pex/bin/pex.py", line 542, in build_pex
    pex_builder.set_script(options.script)
  File "/Users/dpowers/Library/Python/2.7/lib/python/site-packages/pex/pex_builder.py", line 220, in set_script
    script, ', '.join(str(d) for d in self._distributions)))
pex.pex_builder.InvalidExecutableSpecification: Could not find script 'ansible' in any distribution pyparsing 2.2.0, pycrypto 2.6.1, pyasn1 0.2.3, packaging 16.8, ansible 2.2.2.0, cryptography 1.9, pycparser 2.17, enum34 1.1.6, ipaddress 1.0.18, asn1crypto 0.22.0, appdirs 1.4.3, cffi 1.10.0, six 1.10.0, idna 2.5, setuptools 35.0.2, PyYAML 3.12, paramiko 2.1.2, Jinja2 2.9.6, MarkupSafe 1.0 within PEX!

…ation and execution

dpkp · 2017-05-31T21:01:33Z

bb4088f fixes my console script error! But I can reproduce the build failures on py26 locally, so I think those are real.

MarkChuCarroll · 2017-06-01T00:54:23Z

Can you tell me about your py26 environment? I haven't been able to reproduce it, so I'd like to try to figure out what's different.

What OS?
What version of 2.6? (I've got 2.6.9)
What version of pip are you using? (9.0.1)

dpkp · 2017-06-01T00:54:41Z

The test failure appears to be an import error -- wheel.install is imported during pex bootstrap and is not available (it isn't part of stdlib).

> /var/folders/pb/gwlknvpx1l70js5vw_1xnyp00005fb/T/tmpQUHimR/app.pex
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/runpy.py", line 122, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/runpy.py", line 34, in _run_code
    exec code in run_globals
  File "__main__.py", line 25, in <module>
  File ".bootstrap/_pex/pex_bootstrapper.py", line 82, in bootstrap_pex
  File ".bootstrap/_pex/pex.py", line 17, in <module>
  File ".bootstrap/_pex/environment.py", line 23, in <module>
  File ".bootstrap/_pex/pex_builder.py", line 12, in <module>
ImportError: No module named wheel.install

I'm not sure how pex_builder is used within the packaged pex bootstrap environment, but it seems like you want to only import during build and avoid import at runtime.

dpkp · 2017-06-01T01:00:48Z

fwiw, I reproduced using tox tox -v -e py26 -- tests/test_pex.py . Running system mac 2.6.9 and my python2.6 does not have wheel installed globally. pip is 9.0.1

dpkp · 2017-06-01T06:54:23Z

So the import error is a bug that I think needs fixing, but it is not the cause for test failures. The pex artifacts are run within the tox virtualenv which gets 'wheel' installed. So the tests should get past that issue.

I think what is happening in the test failures is WheelFile.install is not copying the full script to bin/ . When I inspect the pex artifact, the test scripts (hello_world and shell_script) are empty except for the hashbang. I confirmed that this doesn't happen on master (which just looks at the raw script in the wheel archive).

MarkChuCarroll · 2017-06-01T14:08:41Z

I think that the wheel import is, ultimately, the root cause.

The tests that are failing on 2.6 are the tests that rely on the pex generation script successfully running Wheelfile.install; I think that what's going on under the covers is that we're winding up with an invalid pex because the generation process is failing due to wheel not being available on the 2.6 host.

dpkp · 2017-06-01T14:37:42Z

setup.py

@@ -61,6 +61,7 @@
    'twitter.common.lang>=0.3.1,<0.4.0',
    'twitter.common.testing>=0.3.1,<0.4.0',
    'twitter.common.dirutil>=0.3.1,<0.4.0',
+    'wheel==0.29.0',


I think that WHEEL_REQUIREMENT should now be in install_requires

dpkp · 2017-06-02T00:18:53Z

I dug through a bit more and I'm confident this is actually hitting a bug deep within the interaction between wheel and zipfile on python2.6 . Specifically, when wheel installs a script it attempts to massage the #!python hashbang before processing the rest of the script. The code is here:
https://bitbucket.org/pypa/wheel/src/0.29.0/wheel/install.py?at=default&fileviewer=file-view-default#install.py-346

It does this by attempting to read a single line from the zipfile using zipfile.readline() . It then reads the remainder of the script using zipfile.read(). The problem is that the underlying zipfile library on python2.6 does not handle this sequence properly. You can test very easily by creating a zipfile with any script, open via z = zipfile.open(filename), then attempt to read a single line followed by the rest of the script w/ readline() and read(). read() returns nothing! Very strange. This does not happen on python2.7, likely because the zipfile module was refactored to fix the underlying issue.

I'm not sure what the best solution is here. First, I wonder if you can reproduce this zipfile bug? If so, I think the options are to (1) break pex installs of wheel files using python2.6, (2) write custom code to install the wheel package that does not have this bug, or (3) see if we can get a patch applied to upstream wheel.

MarkChuCarroll · 2017-06-02T13:04:02Z

Thanks for taking the time to do that! I can't reproduce the error on my macbook, but I can using a linux VM, in which I remove the system python, and install Python2.6 from sources. I'm hesitant to write custom code for wheel installs to bypass this issue. It's not difficult, but it's a very brittle solution: we'd wind up with a parallel version of wheel install, which would need to track changes to the standard wheel package. That's just begging from trouble down the line, in order to continue to support a version of Python that's already unsupported. (One of the problems I've had building an environment to reproduce this bug locally is that it's hard to install a working 2.6 environment; even homebrew withdrew the formula for building one!) -Mark

…

On Thu, Jun 1, 2017 at 8:18 PM Dana Powers ***@***.***> wrote: I dug through a bit more and I'm confident this is actually hitting a bug deep within the interaction between wheel and zipfile on python2.6 . Specifically, when wheel installs a script it attempts to massage the #!python hashbang before processing the rest of the script. The code is here: https://bitbucket.org/pypa/wheel/src/0.29.0/wheel/install.py?at=default&fileviewer=file-view-default#install.py-346 It does this by attempting to read a single line from the zipfile using zipfile.readline() . It then reads the remainder of the script using zipfile.read(). The problem is that the underlying zipfile library on python2.6 does not handle this sequence properly. You can test very easily by creating a zipfile with any script, open via z = zipfile.open(filename), then attempt to read a single line followed by the rest of the script w/ readline() and read(). read() returns nothing! Very strange. This does not happen on python2.7, likely because the zipfile module was refactored to fix the underlying issue. I'm not sure what the best solution is here. First, I wonder if you can reproduce this zipfile bug? If so, I think the options are to (1) break pex installs of wheel files using python2.6, (2) write custom code to install the wheel package that does not have this bug, or (3) see if we can get a patch applied to upstream wheel. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#388 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AArDqCPMK6TdyGZ20pj6UraaZftz3YC6ks5r_1TugaJpZM4Nbam4> .

MarkChuCarroll · 2017-06-21T13:34:36Z

So how can we move this forward? We'd really like to have a version of pex that works with the tensorflow distribution wheel.

My inclination is not to put resources into supporting a version of Python that no one should be using. Would it be an acceptable compromise to use a check in the wheel code, which signals an error for using wheels with .data directories on Python 2.6?

dpkp · 2017-06-21T18:08:04Z

I'm just an interested third-party, not a maintainer. But it seems reasonable to me to error, or perhaps just warn, if it seems like someone is trying to use python2.6 to build a pex from a wheel that has console scripts.

@kwlzn -- what are your thoughts?

dpkp · 2017-06-21T18:13:58Z

Also would suggest skipping the test on py2.6:

@pytest.mark.skipif(sys.version_info <= (2,6),
                    reason="wheel script installation is broken on python 2.6")

MarkChuCarroll · 2017-06-22T15:04:51Z

Can anyone help out with the isort-test failures? Isort running locally on my machine shows no errors; and the file that it's complaining about is unchanged in this diff!

rouge8 · 2017-06-22T15:55:39Z

Can anyone help out with the isort-test failures?

isort 4.2.9+ seems to catch things earlier versions of isort missed. Looks like 4.2.15 is getting installed on Travis, not sure what you have locally.

MarkChuCarroll · 2017-06-27T18:55:27Z

Ping?

This now includes checks for python versions, to generate a warning on older versions of Python.
On those earlier versions of Python, it defaults to treating a wheel file as an object that can be added to the pythonpath (which is the current behavior for all wheels on all versions of Python.)

The build issues are fixed, and all tests pass.

kwlzn · 2017-07-07T06:26:32Z

@MarkChuCarroll apologies for the delay, just catching back up on the pex PR backlog now - will try to take a look at this tomorrow.

kwlzn

thanks for the PR @MarkChuCarroll! looks pretty good to me. handful of smaller things and maybe one issue - then we can expedite to release.

kwlzn · 2017-07-07T06:40:22Z

pex/pex_builder.py

+    # into an importable shape. We can do that by installing it into its own
+    # wheel dir.
+    if not self.interpreter.supports_wheel_install():
+      print("*** Wheel dependencies may not work correctly with Python 2.6.", file=sys.stderr)


it'd be great to avoid printing directly to std descriptors due to the unexpected output effect on downstream consumers like e.g. pants..

how about using the logging module - or at a minimum the warnings module instead? ideally with a one liner message.

kwlzn · 2017-07-07T07:11:43Z

pex/pex_builder.py

@@ -253,7 +256,48 @@ def _add_dist_dir(self, path, dist_name):
        self._copy_or_link(filename, target)
    return CacheHelper.dir_hash(path)

+  def _get_installer_paths(self, dist_name, base):


dist_name appears unused here.

kwlzn · 2017-07-07T07:14:33Z

pex/finders.py

-        dist.get_resource_string('', script_path).replace(b'\r\n', b'\n').replace(b'\r', b'\n'))
+  # This can get called in different contexts; in some, it looks for files in the 
+  # wheel archives being used to produce a pex; in others, it looks for files in the
+  # install wheel directory included in the pex. So we need to look at both locations.


looks like this was the only caller of the safer_name function above, so would kill that along with the removal of it's usage.

kwlzn · 2017-07-07T07:21:37Z

pex/pex_builder.py

+    if self.interpreter.supports_wheel_install() and dist_name.endswith("whl"):
+      from wheel.install import WheelFile
+      try:
+        tmp = tempfile.mkdtemp()


use of pex.common.safe_mkdtemp is preferred.

kwlzn · 2017-07-07T07:35:24Z

pex/pex_builder.py

+            self._chroot.copy(fullpath, target)
+      finally:
+        shutil.rmtree(tmp)
+      return CacheHelper.dir_hash(whltmp)


won't the parent dir (tmp) of whltmp be removed before dir_hash(whltmp) is called?

assuming this was a bug and since this wasn't caught by existing tests, adding some extra test coverage here might be appropriate here if you're up for it.

kwlzn

lgtm!

kwlzn · 2017-07-08T01:08:38Z

thanks a ton for the PR @MarkChuCarroll - this will go out in the 1.2.8 release tracked in #389

kwlzn · 2017-07-10T21:14:31Z

the pex version this went out in is now being consumed in pants master @ pantsbuild/pants@cdb4e5e

dpkp · 2017-07-11T13:46:07Z

Great work! On Jul 10, 2017 2:14 PM, "Kris Wilson" <[email protected]> wrote: this version is now being consumed in pants master @ pantsbuild/pants@ cdb4e5e <pantsbuild/pants@cdb4e5e> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#388 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAzetGWZaM8G0BNZNpAXkx78j9BThAuSks5sMpQ4gaJpZM4Nbam4> .

kwlzn mentioned this pull request May 22, 2017

Release 1.2.8 #389

Closed

kwlzn reviewed May 22, 2017

View reviewed changes

dpkp reviewed May 26, 2017

View reviewed changes

MarkChuCarroll added 6 commits May 30, 2017 15:29

Rewrite the pex wheel fix, using the wheel installer.

533e641

Clean up.

bac461a

Fix some path confusion.

cb5da41

(Code disagreed about whether scripts were stored in /bin or /scripts.)

Switch to use os.walk instead of os.path.walk to increase compatibili…

5a9575c

…ty with python 3.

Tests pass locally; trying to get them to pass in CI.

027635c

Update the wheel script finder so that it works during both pex gener…

bb4088f

…ation and execution

MarkChuCarroll added 2 commits June 1, 2017 09:34

Move "wheelfile" import so it only gets loaded during pex generation.

75014cf

Fix isort issue.

95c01e1

Add an explicit dependency on "wheel" to setup.py's test deps.

86b420c

dpkp reviewed Jun 1, 2017

View reviewed changes

MarkChuCarroll added 2 commits June 1, 2017 11:10

Update requirements to pull wheel in for py26.

0e45922

Run isort to fix import sort issues.

e63644a

MarkChuCarroll added 4 commits June 22, 2017 10:00

Add a bypass of the wheel install code for Python 2.6.

1cd5e1b

Add a bypass of the wheel install code for Python 2.6.

1637e5a

Another try at fixing the build.

e66ade4

Another try at fixing the build.

2ed5338

MarkChuCarroll added 3 commits June 22, 2017 11:09

Try fixing the "skipif" flag.

02a9e61

Add "skipif" to the texs_pex_builder_wheeldep for Python 2.6

5625240

Duh.

570c754

MarkChuCarroll added 2 commits June 22, 2017 12:04

Update isort.

bb3e066

Another try at the import issue.

fcdf3fd

kwlzn reviewed Jul 7, 2017

View reviewed changes

MarkChuCarroll added 4 commits July 7, 2017 09:20

Address some review comments.

031463c

isort.

2701d98

Fix the 26 test.

d84f211

Keep trying to fix the stupid isort.

1100e3f

kwlzn approved these changes Jul 8, 2017

View reviewed changes

kwlzn merged commit 2bdd9c3 into pex-tool:master Jul 8, 2017

kwlzn mentioned this pull request Sep 22, 2017

./pants binary can fail with unhandled wheel.install.BadWheelFile: No expected hash for file ... pantsbuild/pants#4886

Closed

Improve wheel support in pex. #388

Improve wheel support in pex. #388

Conversation

MarkChuCarroll commented May 15, 2017

MarkChuCarroll commented May 22, 2017

kwlzn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpkp May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpkp commented May 26, 2017

MarkChuCarroll commented May 29, 2017

MarkChuCarroll commented May 30, 2017

dpkp commented May 31, 2017

dpkp commented May 31, 2017

MarkChuCarroll commented Jun 1, 2017

dpkp commented Jun 1, 2017

dpkp commented Jun 1, 2017 • edited Loading

dpkp commented Jun 1, 2017

MarkChuCarroll commented Jun 1, 2017

Choose a reason for hiding this comment

dpkp commented Jun 2, 2017

MarkChuCarroll commented Jun 2, 2017 via email

MarkChuCarroll commented Jun 21, 2017

dpkp commented Jun 21, 2017 • edited Loading

dpkp commented Jun 21, 2017

MarkChuCarroll commented Jun 22, 2017

rouge8 commented Jun 22, 2017

MarkChuCarroll commented Jun 27, 2017

kwlzn commented Jul 7, 2017

kwlzn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwlzn left a comment

Choose a reason for hiding this comment

kwlzn commented Jul 8, 2017

kwlzn commented Jul 10, 2017 • edited Loading

dpkp commented Jul 11, 2017 via email

dpkp May 26, 2017 •

edited

Loading

dpkp commented Jun 1, 2017 •

edited

Loading

dpkp commented Jun 21, 2017 •

edited

Loading

kwlzn commented Jul 10, 2017 •

edited

Loading