-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
python_distribution editable installs in exports #18639
Conversation
ce9a9fc
to
a03b7e5
Compare
#18701) In #18639, I have a change to allow pants to do editable installs of `python_distribution`s for in exported-mutable virtualenvs of user code. I need all of the dist chroot setup that happens in the `package_python_dist` rule, so I extracted everything that creates `DistBuildRequest` into a separate helper function.
…d (Cherry-pick of #18701) (#18703) In #18639, I have a change to allow pants to do editable installs of `python_distribution`s for in exported-mutable virtualenvs of user code. I need all of the dist chroot setup that happens in the `package_python_dist` rule, so I extracted everything that creates `DistBuildRequest` into a separate helper function.
I made a copy of many of these sources so I could run them as a plugin with (st2export is my lightweight copy of the export goal, so I could wire up my changes from this PR)
|
c24c940
to
b660c57
Compare
….py` + `subsystems/setup_py_generation.py` (#18702) In pants v1, there was a [`setup-py` goal](https://v1.pantsbuild.org/options_reference.html#setup-py). In pants v2, that is now part of the more generic [`package` goal](https://www.pantsbuild.org/docs/python-package-goal). So, the name `setup_py` doesn't make sense any more. Also, most of the rules in that file are used in contexts other than the `package` goal. For instance, I'm using them in #18639 in the `export` goal. And importing something from `goals` in `util_rules` feels rather odd. So, I would like to see most of these rules under `util_rules`. This change does the following: - renames: `pants.backends.python.goals.setup_py` to `pants.backends.python.util_rules.package_dists` - adds a minimal: `pants.backends.python.goals.package_dists` - moves `SetupPyGeneration(Subsystem)` to `pants.backends.python.subsystems.setup_py_generation` - adjusts imports to use the new locations as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🚀
Only left some minor nits.
Some of which certainly comes from my ignorance of Python build/packaging particulars. :)
@@ -123,6 +128,30 @@ class ExportPluginOptions: | |||
removal_hint="Set the `[export].py_resolve_format` option to 'symlinked_immutable_virtualenv'", | |||
) | |||
|
|||
py_editables_in_resolves = StrListOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option name reads weird to me, but I've not yet been able to come up with something better..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I have also failed to come up with anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through a lot of different names - I tried putting it on a PythonSetup(Subsystem)
but that felt weird because this is specifically about export. So, then I moved it here and went with the py_
prefix similar to py_resolve_format
. I'm still not entirely happy with it. But, this is the least bad I've come up with so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about py_editable_by_resolve
?
I know it's a list; so plural. But it is used on a "one by one" basis, so this reads better to me. It answers: "Are python distributions to be installed in editable
mode in this resolve?" for a given resolve. The "editable" thing is not plural in that question, nor is the resolve, but the source for the answer is used for multiple instances of that question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each dist in the resolve is installed as editable, which is why I was thinking of using editables
-- each resolve has more than one editable wheel installed.
As I struggle with naming this, one thing I'm worried about is: You can do an editable install of ~anything. If you request an editable install of a git repo, for example, pip will clone it and then you'll be able to see where it is cloned in pip list
output. So, I want to make it clear that this is only editable for first party code.
That said, Hmm. by
makes this sound like a map. I could however drop the plural and do py_editable_in_resolve
. Is that any better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, Hmm.
by
makes this sound like a map. I could however drop the plural and dopy_editable_in_resolve
. Is that any better?
To me, yes. As I see the editable
as an adjective describing how something(s) are installed it looks weird to pluralize it. Maybe I'm overanalyzing this however.. language is.. rather subjective, perhaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I just pushed a change with py_editable_in_resolve
.
src/python/pants/backend/python/util_rules/local_dists_pep660.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/util_rules/local_dists_pep660.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/util_rules/local_dists_pep660.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/util_rules/local_dists_pep660.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/util_rules/local_dists_pep660.py
Outdated
Show resolved
Hide resolved
Before I even review, thanks for the very detailed and helpful PR description! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! That is a lot of new machinery, but the tests are good and it's off by default, so that's OK.
LGTM mod @kaos's comments, which I agree with.
@@ -123,6 +128,30 @@ class ExportPluginOptions: | |||
removal_hint="Set the `[export].py_resolve_format` option to 'symlinked_immutable_virtualenv'", | |||
) | |||
|
|||
py_editables_in_resolves = StrListOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I have also failed to come up with anything.
src/python/pants/backend/python/util_rules/local_dists_pep660.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/util_rules/local_dists_pep660.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/util_rules/local_dists_pep660.py
Outdated
Show resolved
Hide resolved
editable_wheel_path: str | None | ||
|
||
|
||
# PEP 660 defines `prepare_metadata_for_build_editable`. If PEP 660 is not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If PEP 660 is not supported by whom? This is for in-repo, first-party code, right? So we could require that you support PEP 660 (and maybe the PEP 517 fallback)? It is in the user's power to make that so.
I'm not necessarily proposing changes, more trying to understand the context, and how much bad stuff in the wild this is expected to handle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP 660 is fairly new - it was marked as final only 10 months ago, which is a fairly short period of time for python packaging.
I'm still using python3.6 (sadly), and the version of setuptools that supports python3.6 does not support PEP 660. And, that older version of setuptools generated dist-info directories named pkg_name.dist-info
instead of pkg_name-version.dist-info
. So, I needed to handle most of these quirks for st2 where I want to use this feature.
The one quirk I didn't encounter was falling back to build_wheel
. I have not reviewed the implementations of anything other than setuptools, so if there is a version (prob an older version) of poetry, pdm, flint, or whatever that people have configured as their PEP-517 backend, then this is required to fully support the PEP-517 and PEP-660 specs. In this section of PEP 517, it highlights:
One specific consequence of this is that in this PEP, we’re able to make the
prepare_metadata_for_build_wheel
command optional. In our design, this can be readily handled by build frontends, which can put code in their subprocess runner like:
And then it shows a code block that falls back to build_wheel
and then runs an undefined unzip_metadata
function to get the metadata.
So we could require that you support PEP 660 (and maybe the PEP 517 fallback)? It is in the user's power to make that so.
The backend does not actually have to support PEP 660 because we cannot delegate building the wheel to an external backend. We cannot delegate building the editable wheel because we're running in a sandbox and the spec doesn't give us a way to provide which directory should be used as the editable sources. So, all we're using the backend for is to get the wheel metadata. Though it does not need to implement PEP 660, it must support PEP 517. Basically, Pants becomes the PEP 660-compliant frontend + backend, delegating metadata creation to the user-requested PEP-517 backend.
src/python/pants/backend/python/util_rules/local_dists_pep660.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/util_rules/local_dists_pep660.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. 👍🏽
I've got a option name suggestion, perhaps an improvement? Otherwise I think this is good to land.
@@ -123,6 +128,30 @@ class ExportPluginOptions: | |||
removal_hint="Set the `[export].py_resolve_format` option to 'symlinked_immutable_virtualenv'", | |||
) | |||
|
|||
py_editables_in_resolves = StrListOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about py_editable_by_resolve
?
I know it's a list; so plural. But it is used on a "one by one" basis, so this reads better to me. It answers: "Are python distributions to be installed in editable
mode in this resolve?" for a given resolve. The "editable" thing is not plural in that question, nor is the resolve, but the source for the answer is used for multiple instances of that question.
@@ -146,6 +146,7 @@ class DistBuildResult: | |||
_BACKEND_SHIM_BOILERPLATE = """ | |||
# DO NOT EDIT THIS FILE -- AUTOGENERATED BY PANTS | |||
|
|||
import errno |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flake8 complained about missing errno in my backend wrapper script - and I copied the use of errno from this shim script, so this needs the errno import as well.
7db357b
to
9090edf
Compare
# The backend_wrapper has its own digest for cache reuse. | ||
Get( | ||
Digest, | ||
CreateDigest([FileContent(backend_wrapper_path, backend_wrapper_content)]), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this isn't part of the previous CreateDigest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and now I see the comment--lol :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how big impact it has, if any, though.. curious what @stuhood thinks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how much of an impact it has. For ST2 we have 18 dists, each of which will have a different set of settings to pass in (in the conf_digest
) but the backend_wrapper
is the same for all of them. So, I figured I'd split the 2 digests up here.
We do merge the digests below, but we would have to do that merge anyway to include any input digest.
I can combine them. I don't know what impact it has one way or another, but I suspect this is at least marginally better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I've no clue either. Guess it'll comer down to the performance of materializing an extra file in a digest vs running an additional (with cached result) workunit.
About Editable Installs
Editable installs were traditionally done via pip with
pip install --editable
. It is primarily useful during development when software needs access to the entry points metadata.When PEP 517 was adopted, they punted on how to allow for editable installs. PEP 660 extended the PEP 517 backend API to support building "editable" wheels. Therefore, there is now a standard way to collect and install the metadata for "editable" installs, using the "editable" wheel as an interchange between the backend (which collects the metadata + builds the editable wheel) and the frontend (which marshals the backend to perform a user-requested "editable" install).
Why would we need editable installs in pants?
I need editable installs in pants-exported virtualenvs so that dev tools outside of pants have access to:
dist-info
. Entry points is the biggest most impactful example)I need to point all the external dev tooling at a virtualenv, and technically I could export a pex that includes all of the python-distributions pre-installed and use pex-tools to create a virtualenv, but then I would have to recreate that venv for every dev change wouldn't be a good dev experience.
One of those dev tools is
nosetest
. I considred usingrun
to support running that, but I am leary of adding all the complex BUILD machinery to support running a tool that I'm trying to get rid of. Editable installs is a more generic solution that serves my current needs, while allowing for using it in other scenarios.This PR comes in part from #16621 (comment)
Overview of this PR
Scope & Future work
This PR focuses on adding editable installs to exported virtualenvs. Two other issues ask for editable installs while running tests:
We can probably reuse a significant portion of this to generate editable wheels for use in testing as well. Parts of this code will need to be refactored to support that use case. But we also have to figure out the UX for how users will define dependencies on a
python_distribution
when they want an editable install instead of the built wheel to show up in the sandbox. Anyway, addressing that is out of scope for this PR.New option
[export].py_editable_in_resolve
(aStrListOption
)This option allows user resolves to opt in to using the editable installs. After consulting with @kaos, I decided to add an option for this instead of always trying to generate/install the editable wheels.
For StackStorm, I plan to set this in
pants.toml
for the default resolve that has python_distributions.Even without this option, I tried to bail out early if there were no
python_distribution
s to install.Installing editable wheels for exports
I added this feature to the new export code path, which requires using
export --resolve=
. The legacy codepath, which uses cli specsexport <address specs>
did not change at all. I also ignored thetool
resolves which cannot have any relevant dists (andtool
resolves are deprecated anyway). Also, this is only formutable_virtualenv
exports, as we need modify the virtualenv to install the editable wheels in the venv after pex creates it from the lockfile.When exporting a user resolve, we do a
Get(EditableLocalDists, EditableLocalDistsRequest(resolve=resolve))
: I'll skip over exactly how this builds the wheels for now so this section can focus on how installing works.pants/src/python/pants/backend/python/goals/export.py
Lines 373 to 379 in f3a4620
As described in the commit message of b5aa26a, I tried a variety of methods for installing the editable wheels using pex. Ultimately, the best I came up with is telling pex that the digest containing our editable wheels are
sources
when building therequirements.pex
used to populate the venv, so that they would land in the virtualenv (even though they land as plain wheel files.Then we run
pex-tools
in aPostProcessingCommand
to create and populate the virtualenv, just as we did before this PR.Once the virtualenv is created, we add 3 more
PostProcessingCommands
to actually do the editable install. In this step, Pants is essentially acting as the PEP-660 front end, though we use pip for some of the heavy lifting. These commands:.pth
file that injects the source dir intosys.path
and a.dist-info
directory with dist metadata such as entry points).*.dist-info/direct_url.json
) with our own so that we comply with PEP-660 and mark the install as editable with a file url pointing the the sources in the build_root (vs in a sandbox).Now, anything that uses the exported venv should have access to the standardized package metadata (in
.dist-info
) and the relevant source roots should be automatically included insys.path
.Building PEP-660 editable wheels
The logic that actually builds the editable wheels is in
pants.backend.python.util_rules.local_dists_pep660
. Building these wheels requires the same chroot that pants uses to build regular wheels and sdists. So, I refactored the rule inutil_rules.setup_py
so that I could reuse the part that builds theDistBuildRequest
.These
local_dists_pep660
rules do approx this, starting with the rule called in export:Get(EditableLocalDists, EditableLocalDistsRequest(resolve=resolve))
uses rulebuild_editable_local_dists
ResolveSortedPythonDistributionTargets
comes from rule:sort_all_python_distributions_by_resolve
AllPythonDistributionTargets
comes from rule:find_all_python_distributions
Get(LocalDistPEP660Wheels, PythonDistributionFieldSet.create(dist))
for each dist in the resolve uses rule:isolate_local_dist_pep660_wheels
DistBuildRequest
using thecreate_dist_build_request
method I exposed inutil_rules.setup_py
Get(PEP660BuildResult, DistBuildRequest)
uses rule:run_pep660_build
.pth
file that goes in the editable wheelpython_distribution
to generate the.dist-info
directoryWHEEL
andRECORD
file to build a conformant wheel file.pth
file previously generated (and placed in the sandbox with the wrapper script)zipfile
to build the wheel (using a vendored+modified function from thewheel
package).local_dists
rules do.python_distribution
targets. This gets wrapped inEditableLocalDists
Much of the rule logic was based on (copied then modified):
pants.backend.python.util_rules.dists
andpants.backend.python.util_rules.local_dists
.