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.

Sign up for GitHub

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

Update help for python resolves with recent improvements #15228

Merged
merged 2 commits into from
Apr 25, 2022
Merged

Update help for python resolves with recent improvements #15228

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e03736b
Update `help` for python resolves with recent improvements
Eric-Arellano Apr 22, 2022
eabee9f
Review feedback. Thank you Benjy!
Eric-Arellano Apr 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
226 changes: 125 additions & 101 deletions src/python/pants/backend/python/subsystems/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,12 @@ class PythonSetup(Subsystem):
"""
The Python interpreters your codebase is compatible with.

These constraints are used as the default value for the `interpreter_constraints`
field of Python targets.

Specify with requirement syntax, e.g. 'CPython>=2.7,<3' (A CPython interpreter with
version >=2.7 AND version <3) or 'PyPy' (A pypy interpreter of any version). Multiple
constraint strings will be ORed together.

These constraints are used as the default value for the `interpreter_constraints`
field of Python targets.
"""
),
advanced=True,
Expand Down Expand Up @@ -85,77 +85,33 @@ class PythonSetup(Subsystem):
),
advanced=True,
)
requirement_constraints = FileOption(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to lower in the file, so that they show up later in help. We don't want any newcomers using this.

"--requirement-constraints",
default=None,
help=softwrap(
"""
When resolving third-party requirements for your own code (vs. tools you run),
use this constraints file to determine which versions to use.

This only applies when resolving user requirements, rather than tools you run
like Black and Pytest. To constrain tools, set `[tool].lockfile`, e.g.
`[black].lockfile`.

See https://pip.pypa.io/en/stable/user_guide/#constraints-files for more
information on the format of constraint files and how constraints are applied in
Pex and pip.

Mutually exclusive with `[python].enable_resolves`.
"""
),
advanced=True,
mutually_exclusive_group="lockfile",
)
resolve_all_constraints = BoolOption(
"--resolve-all-constraints",
default=True,
help=softwrap(
"""
If enabled, when resolving requirements, Pants will first resolve your entire
constraints file as a single global resolve. Then, if the code uses a subset of
your constraints file, Pants will extract the relevant requirements from that
global resolve so that only what's actually needed gets used. If disabled, Pants
will not use a global resolve and will resolve each subset of your requirements
independently.

Usually this option should be enabled because it can result in far fewer resolves.

Requires [python].requirement_constraints to be set.
"""
),
advanced=True,
)
enable_resolves = BoolOption(
"--enable-resolves",
default=False,
help=softwrap(
"""
Set to true to enable the multiple resolves mechanism. See
`[python].resolves` for an explanation of this feature.
f"""
Set to true to enable lockfiles for user code. See `[python].resolves` for an
explanation of this feature.

Warning: the `generate-lockfiles` goal does not yet work if you have VCS (Git)
requirements and local requirements. Support is coming in a future Pants release. You
can still use multiple resolves, but you must manually generate your lockfiles rather
than using the `generate-lockfiles` goal, e.g. by running `pip freeze`. Specifically,
set up `[python].resolves` to point to your manually generated lockfile paths, and
then set `[python].resolves_generate_lockfiles = false` in `pants.toml`.
Warning: the `generate-lockfiles` goal does not yet work if you have local
requirements, regardless of using Pex vs. Poetry for the lockfile generator.
Support is coming in a future Pants release. In the meantime, the workaround is to host
the files in a custom repository with `[python-repos]`
({doc_url('3rdparty-dependencies#custom-repositories')}).

You may also run into issues generating lockfiles when using Poetry as the generator,
rather than Pex. See the option `[python].lockfile_generator` for more
information.

The resolves feature offers three major benefits compared to
`[python].requirement_constraints`:
This option is mutually exclusive with `[python].requirement_constraints`. We strongly
recommend using this option because it:

1. Uses `--hash` to validate that all downloaded files are expected, which reduces\
the risk of supply chain attacks.
2. Enforces that all transitive dependencies are in the lockfile, whereas\
constraints allow you to leave off dependencies. This ensures your build is more\
stable and reduces the risk of supply chain attacks.
3. Allows you to have multiple resolves in your repository.

Mutually exclusive with `[python].requirement_constraints`.
3. Allows you to have multiple lockfiles in your repository.
"""
),
advanced=True,
Expand All @@ -169,17 +125,10 @@ class PythonSetup(Subsystem):
A mapping of logical names to lockfile paths used in your project.

Many organizations only need a single resolve for their whole project, which is
a good default and the simplest thing to do. However, you may need multiple
a good default and often the simplest thing to do. However, you may need multiple
resolves, such as if you use two conflicting versions of a requirement in
your repository.

For now, Pants only has first-class support for disjoint resolves, meaning that
you cannot ergonomically set a `python_requirement` or `python_source` target,
for example, to work with multiple resolves. Practically, this means that you
cannot yet ergonomically reuse common code, such as util files, across projects
using different resolves. Support for overlapping resolves is coming in Pants 2.11
through a new 'parametrization' feature.

If you only need a single resolve, run `{bin_name()} generate-lockfiles` to
generate the lockfile.

Expand All @@ -200,6 +149,16 @@ class PythonSetup(Subsystem):
`python_test` / `python_tests`, and `pex_binary` which need to set a non-default\
resolve with the `resolve` field.

If a target can work with multiple resolves, you can either use the `parametrize`
mechanism or manually create a distinct target per resolve. See {doc_url("targets")}
for information about `parametrize`.

For example:

python_sources(
resolve=parametrize("data-science", "web-app"),
)
Comment on lines +158 to +160
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thejcannon I could not find our discussion from last week about this. Do you remember if this is right? No need for back ticks?

Copy link
Member

Choose a reason for hiding this comment

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

I never verified, but according to Markdown it's code because it's indented. The console will just dump whatever.
So depends on if we care about the console

Copy link
Member

Choose a reason for hiding this comment

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

There are examples for how readme treats this from the docker backend (indented block is code).

So depends on if we care about the console

What do you mean?
As you said, the console dumps whatever, so we don't have to "care about the console", just care about the experience the user has when reading this text on the console, which perhaps is what you meant? In which case I'm in favour of excluding the backticks to reduce the noise, on the console. (as the backticks does nothing for the markdown, when the block is also indented).

Copy link
Member

Choose a reason for hiding this comment

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

pants/src/python/pants/backend/docker/subsystems/docker_options.py

Lines 39 to 53 in e8e22d5

help=softwrap(
"""
Configure Docker registries. The schema for a registry entry is as follows:
{
"registry-alias": {
"address": "registry-domain:port",
"default": bool,
"extra_image_tags": [],
"skip_push": bool,
},
...
}
If no registries are provided in a `docker_image` target, then all default

https://www.pantsbuild.org/docs/reference-docker#section-registries

Copy link
Member

Choose a reason for hiding this comment

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

@Eric-Arellano perhaps we want to record this doc nuance in the https://www.pantsbuild.org/docs/style-guide ? :)


You can name the lockfile paths what you would like; Pants does not expect a
certain file extension or location.

Expand Down Expand Up @@ -232,45 +191,21 @@ class PythonSetup(Subsystem):
this option, you can override each resolve to use certain interpreter
constraints, such as `{'data-science': ['==3.8.*']}`.

Warning: this does NOT impact the interpreter constraints used by targets within the
resolve, which is instead set by the option `[python.interpreter_constraints` and the
`interpreter_constraints` field. It only impacts how the lockfile is generated.

Pants will validate that the interpreter constraints of your code using a
resolve are compatible with that resolve's own constraints. For example, if your
code is set to use ['==3.9.*'] via the `interpreter_constraints` field, but it's
also using a resolve whose interpreter constraints are set to ['==3.7.*'], then
using a resolve whose interpreter constraints are set to ['==3.7.*'], then
Pants will error explaining the incompatibility.

The keys must be defined as resolves in `[python].resolves`.
"""
),
advanced=True,
)
no_binary = StrListOption(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to lower in the file. We have a block of about 10 lockfile related options, and this was in the middle.

"--no-binary",
help=softwrap(
"""
Do not use binary packages (i.e., wheels) for these 3rdparty projects. Also
accepts :all: to disable all binary packages. Note that some packages are tricky to
compile and may fail to install when this option is used on them.
See https://pip.pypa.io/en/stable/cli/pip_install/#install-no-binary for details.
Note: Only takes effect if you use lockfiles, and set lockfile_generator = "pex"
in the [python] section of your config file. You must regenerate the lockfile after
changing this option.
"""
),
)
only_binary = StrListOption(
"--only-binary",
help=softwrap(
"""
Do not use source packages (i.e., sdists) for these 3rdparty projects. Also
accepts :all: to disable all source packages. Packages without binary distributions
will fail to install when this option is used on them.
See https://pip.pypa.io/en/stable/cli/pip_install/#install-only-binary for details.
Note: Only takes effect if you use lockfiles, and set lockfile_generator = "pex"
in the [python] section of your config file. You must regenerate the lockfile after
changing this option.
"""
),
)
invalid_lockfile_behavior = EnumOption(
"--invalid-lockfile-behavior",
default=InvalidLockfileBehavior.error,
Expand All @@ -296,8 +231,13 @@ class PythonSetup(Subsystem):
f"""
Whether to use Pex or Poetry with the `generate-lockfiles` goal.

Poetry does not work with `[python-repos]` for custom indexes/cheeseshops. If you use
this feature, you should use Pex.
Poetry does not support these features:

1) `[python-repos]` for custom indexes/cheeseshops.
2) VCS (Git) requirements.
3) `[GLOBAL].ca_certs_path`.

If you use any of these features, you should use Pex.

Several users have also had issues with how Poetry's lockfile generation handles
environment markers for transitive dependencies; certain dependencies end up with
Expand All @@ -311,7 +251,8 @@ class PythonSetup(Subsystem):
`generate-lockfiles` goal. Alternatively, use Pex for generation.

Finally, installing from a Poetry-generated lockfile is slower than installing from a
Pex lockfile.
Pex lockfile. When using a Pex lockfile, Pants will only install the subset needed
for the current task.

However, Pex lockfile generation is a new feature. Given how vast the Python packaging
ecosystem is, it is possible you may experience edge cases / bugs we haven't yet
Expand All @@ -323,6 +264,7 @@ class PythonSetup(Subsystem):
interoperability with tools like IDEs.
"""
),
advanced=True,
)
resolves_generate_lockfiles = BoolOption(
"--resolves-generate-lockfiles",
Expand All @@ -333,7 +275,8 @@ class PythonSetup(Subsystem):
running the `generate-lockfiles` goal.

This is intended to allow you to manually generate lockfiles as a workaround for the
issues described in the `[python].enable_resolves` option.
issues described in the `[python].lockfile_generator` option, if you are not yet ready
to use Pex.

If you set this to False, Pants will not attempt to validate the metadata headers
for your user lockfiles. This is useful so that you can keep
Expand All @@ -349,8 +292,14 @@ class PythonSetup(Subsystem):
help=softwrap(
"""
If enabled, when running binaries, tests, and repls, Pants will use the entire
lockfile/constraints file instead of just the relevant subset. This can improve
performance and reduce cache size, but has two consequences: 1) All cached test
lockfile file instead of just the relevant subset.

We generally do not recommend this if `[python].lockfile_generator` is set to `"pex"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Help should not usually appeal to history, so the whole performance enhancements story is more complicating than helpful. How about just:

"This yields better performance in some cases, but we generally do not recommend this if ..., as pex lockfiles have equivalent performance, and better cache utilization, in most cases."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Help should not usually appeal to history

Good point. That book I've been reading on technical writing made the same point

Copy link
Contributor

Choose a reason for hiding this comment

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

You've deleted the performance rationale for this option, so readers now have no clue why they might use this. There may still be perf benefits in some cases. We have not yet really proven that subsetting is now better in all cases. So some users may still need this option for performance, and we should mention that this is so. E.g., the phrasing I suggested above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I did? Read the next paragraph

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, weird sequencing. You have to read through the arguments against using this option before you see any arguments for why you might want to try it. But It's probably fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still wouldn't reference "performance enhancements we've made". The reader won't have a baseline of what that means, i.e., performance of what compared to what.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have to read through the arguments against using this option before you see any arguments for why you might want to try it.

Yeah. Josh is pretty sure we will want to deprecate this once the dust settles. So I'm trying to discourage usage

thanks to performance enhancements we've made. When using Pex lockfiles, you should
get similar performance to using this option but without the downsides mentioned below.

Otherwise, if not using Pex lockfiles, this option can improve
performance and reduce cache size. But it has two consequences: 1) All cached test
results will be invalidated if any requirement in the lockfile changes, rather
than just those that depend on the changed requirement. 2) Requirements unneeded
by a test/run/repl will be present on the sys.path, which might in rare cases
Expand All @@ -363,6 +312,81 @@ class PythonSetup(Subsystem):
),
advanced=True,
)
requirement_constraints = FileOption(
"--requirement-constraints",
default=None,
help=softwrap(
"""
When resolving third-party requirements for your own code (vs. tools you run),
use this constraints file to determine which versions to use.

Mutually exclusive with `[python].enable_resolves`, which we generally recommend as an
improvement over constraints file.

See https://pip.pypa.io/en/stable/user_guide/#constraints-files for more
information on the format of constraint files and how constraints are applied in
Pex and pip.

This only applies when resolving user requirements, rather than tools you run
like Black and Pytest. To constrain tools, set `[tool].lockfile`, e.g.
`[black].lockfile`.
"""
),
advanced=True,
mutually_exclusive_group="lockfile",
)
resolve_all_constraints = BoolOption(
"--resolve-all-constraints",
default=True,
help=softwrap(
"""
(Only relevant when using `[python].requirement_constraints.`) If enabled, when
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This first sentence is new and from 2.10

resolving requirements, Pants will first resolve your entire
constraints file as a single global resolve. Then, if the code uses a subset of
your constraints file, Pants will extract the relevant requirements from that
global resolve so that only what's actually needed gets used. If disabled, Pants
will not use a global resolve and will resolve each subset of your requirements
independently.

Usually this option should be enabled because it can result in far fewer resolves.
"""
),
advanced=True,
)
no_binary = StrListOption(
"--no-binary",
help=softwrap(
"""
Do not use binary packages (i.e., wheels) for these 3rdparty projects.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The text is the same, but I added paragraph breaks. I also re-worded the last paragraph to be more accurate and concise


Also accepts `:all:` to disable all binary packages.

Note that some packages are tricky to compile and may fail to install when this option
is used on them. See https://pip.pypa.io/en/stable/cli/pip_install/#install-no-binary
for details.

Note: Only takes effect if you use Pex lockfiles. Set
`[python].lockfile_generator = "pex"` and run the `generate-lockfiles` goal.
"""
),
)
only_binary = StrListOption(
"--only-binary",
help=softwrap(
"""
Do not use source packages (i.e., sdists) for these 3rdparty projects.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto. I added paragraph breaks and re-worded the last paragraph.


Also accepts `:all:` to disable all source packages.

Packages without binary distributions will fail to install when this option is used on
them. See https://pip.pypa.io/en/stable/cli/pip_install/#install-only-binary for
details.

Note: Only takes effect if you use Pex lockfiles. Set
`[python].lockfile_generator = "pex"` and run the `generate-lockfiles` goal.
"""
),
)
resolver_manylinux = StrOption(
"--resolver-manylinux",
default="manylinux2014",
Expand Down