python_distribution editable installs in exports #18639
Conversation
cognifloyd
added
category:new feature
backend: Python
cognifloyd
force-pushed
the
editables-in-exports
branch
4 times, most recently
from
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) |
cognifloyd
force-pushed
the
editables-in-exports
branch
from
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.
| @@ -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.
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.
Yeah, I have also failed to come up with anything.
There was a problem hiding this comment.
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.
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.
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.
That said, Hmm.
bymakes 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.
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! |
| @@ -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.
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.
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.
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_wheelcommand 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
| @@ -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.
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.
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.
cognifloyd
force-pushed
the
editables-in-exports
branch
from
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.
Any reason this isn't part of the previous CreateDigest?
There was a problem hiding this comment.
I wonder how big impact it has, if any, though.. curious what @stuhood thinks.
There was a problem hiding this comment.
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.
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 usingrunto 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_distributionwhen 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.tomlfor the default resolve that has python_distributions.Even without this option, I tried to bail out early if there were no
python_distributions 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 thetoolresolves which cannot have any relevant dists (andtoolresolves are deprecated anyway). Also, this is only formutable_virtualenvexports, 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
sourceswhen building therequirements.pexused to populate the venv, so that they would land in the virtualenv (even though they land as plain wheel files.Then we run
pex-toolsin aPostProcessingCommandto create and populate the virtualenv, just as we did before this PR.Once the virtualenv is created, we add 3 more
PostProcessingCommandsto 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:.pthfile that injects the source dir intosys.pathand a.dist-infodirectory 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_pyso that I could reuse the part that builds theDistBuildRequest.These
local_dists_pep660rules do approx this, starting with the rule called in export:Get(EditableLocalDists, EditableLocalDistsRequest(resolve=resolve))uses rulebuild_editable_local_distsResolveSortedPythonDistributionTargetscomes from rule:sort_all_python_distributions_by_resolveAllPythonDistributionTargetscomes from rule:find_all_python_distributionsGet(LocalDistPEP660Wheels, PythonDistributionFieldSet.create(dist))for each dist in the resolve uses rule:isolate_local_dist_pep660_wheelsDistBuildRequestusing thecreate_dist_build_requestmethod I exposed inutil_rules.setup_pyGet(PEP660BuildResult, DistBuildRequest)uses rule:run_pep660_build.pthfile that goes in the editable wheelpython_distributionto generate the.dist-infodirectoryWHEELandRECORDfile to build a conformant wheel file.pthfile previously generated (and placed in the sandbox with the wrapper script)zipfileto build the wheel (using a vendored+modified function from thewheelpackage).local_distsrules do.python_distributiontargets. This gets wrapped inEditableLocalDistsMuch of the rule logic was based on (copied then modified):
pants.backend.python.util_rules.distsandpants.backend.python.util_rules.local_dists.