Skip to content
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

Use installer to remove custom unzip and spread code #715

Merged
merged 13 commits into from
Jun 30, 2022

Conversation

groodt
Copy link
Collaborator

@groodt groodt commented May 28, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Current behaviour uses bespoke code to unzip Python Wheels. This code does not utilise the PEP standards to unpack files and relies on some fragile globs to include and exclude files in the generated py_library.

What is the new behavior?

As mentioned in #700, installer can be used to unpack Wheels in a PEP standards compliant manner.

In this PR installer is used to replace the custom purelib handling. It also unpacks Wheels into more standard path names that align with a standard Python installation or venv when inspecting sysconfig.

site-packages - The destination for the source code (both purelib and platlib)
include - The location for any header files
bin - The location for entry_point scripts
data - The location for Wheel data

These path names more closely align with standard Python names either on a system or in a venv. See below.

❯ python3
Python 3.9.12 (main, Mar 26 2022, 15:52:10)
[Clang 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from sysconfig import get_paths
>>> from pprint import pprint
>>> info = get_paths()
>>> pprint(info)
{'data': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9',
 'include': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/include/python3.9',
 'platinclude': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/include/python3.9',
 'platlib': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/site-packages',
 'platstdlib': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9',
 'purelib': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/site-packages',
 'scripts': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/bin',
 'stdlib': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9'}
>>> quit()

Similarly for a venv

/tmp
❯ python3 -m venv myvenv

/tmp
❯ source myvenv/bin/activate.fish

/tmp
myvenv ❯ python3
Python 3.9.12 (main, Mar 26 2022, 15:52:10)
[Clang 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from sysconfig import get_paths
>>> from pprint import pprint
>>> info = get_paths()
>>> pprint(info)
{'data': '/private/tmp/myvenv',
 'include': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/include/python3.9',
 'platinclude': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/include/python3.9',
 'platlib': '/private/tmp/myvenv/lib/python3.9/site-packages',
 'platstdlib': '/private/tmp/myvenv/lib/python3.9',
 'purelib': '/private/tmp/myvenv/lib/python3.9/site-packages',
 'scripts': '/private/tmp/myvenv/bin',
 'stdlib': '/usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9'}
>>> quit()

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@groodt groodt marked this pull request as ready for review May 28, 2022 14:10
@groodt groodt force-pushed the groodt-replace-unzip-installer branch from be8be48 to bb5a7c4 Compare May 28, 2022 14:11
@groodt groodt force-pushed the groodt-replace-unzip-installer branch from 70d851e to f37fe38 Compare June 7, 2022 07:16
@groodt
Copy link
Collaborator Author

groodt commented Jun 10, 2022

I can confirm that this passes our CI at $dayjob. That gives me some confidence that this change is robust to real-world use.

The python dependency closure at $dayjob is ~110 direct dependencies and ~320 total in the transitive closure. The closure includes common dependencies such as boto3, click and requests. It also includes many scientific/ML dependencies including scikit-learn, pytorch and tensorflow.

Copy link

@thundergolfer thundergolfer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good. You've marked Refactoring (no functional changes, no api changes) but also the "breaking change" checkbox.

Is this only breaking if rules consumers were depending on the existing layout within the runfiles directory?

@groodt
Copy link
Collaborator Author

groodt commented Jun 21, 2022

Is this only breaking if rules consumers were depending on the existing layout within the runfiles directory?

Yes, exactly. I suspect those use-cases would not be very common.

@thundergolfer thundergolfer requested review from hrfuller and mattem June 22, 2022 00:27
@groodt groodt merged commit 09457c7 into bazelbuild:main Jun 30, 2022
mattem pushed a commit to mattem/rules_python that referenced this pull request Jul 7, 2022
@alexeagle
Copy link
Collaborator

I think this is actually the first breaking change that drops python 3.6 support from rules_python...

  File "/cache/bazel/6fadd30f776596320f945376fa05bc65/external/pypi__installer/installer/scripts.py", line 6, in 
    from importlib.resources import read_binary
ModuleNotFoundError: No module named 'importlib.resources'

did we intend that? we should document it very loudly if so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants