Update `help` for python resolves with recent improvements #15228

Eric-Arellano · 2022-04-22T19:25:20Z

This includes the changes from #14775, which were only applied to 2.10.

[ci skip-rust]
[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano

Hats off to @thejcannon for softwrap. This was so much more enjoyable to edit than before.

Eric-Arellano · 2022-04-22T19:25:52Z

src/python/pants/backend/python/subsystems/setup.py

@@ -85,77 +85,30 @@ class PythonSetup(Subsystem):
        ),
        advanced=True,
    )
-    requirement_constraints = FileOption(


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

src/python/pants/backend/python/subsystems/setup.py

Eric-Arellano · 2022-04-22T19:27:11Z

src/python/pants/backend/python/subsystems/setup.py

+                python_sources(
+                    resolve=parametrize("data-science", "web-app"),
+                )


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

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

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).

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

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

Eric-Arellano · 2022-04-22T19:27:49Z

src/python/pants/backend/python/subsystems/setup.py

            Pants will error explaining the incompatibility.

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


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

Eric-Arellano · 2022-04-22T19:28:30Z

src/python/pants/backend/python/subsystems/setup.py

+        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, including due to less supply chain risk thanks to
+            `--hash` support.
+
+            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`.
+            """


This is all the same except for the second paragraph, which is copied from 2.10

Eric-Arellano · 2022-04-22T19:28:47Z

src/python/pants/backend/python/subsystems/setup.py

+        default=True,
+        help=softwrap(
+            """
+            (Only relevant when using `[python].requirement_constraints.`) If enabled, when


This first sentence is new and from 2.10

Eric-Arellano · 2022-04-22T19:29:09Z

src/python/pants/backend/python/subsystems/setup.py

+        "--no-binary",
+        help=softwrap(
+            """
+            Do not use binary packages (i.e., wheels) for these 3rdparty projects.


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

Eric-Arellano · 2022-04-22T19:29:28Z

src/python/pants/backend/python/subsystems/setup.py

+        "--only-binary",
+        help=softwrap(
+            """
+            Do not use source packages (i.e., sdists) for these 3rdparty projects.


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

benjyw

Nice! Some phrasing and grammar comments.

src/python/pants/backend/python/subsystems/setup.py

benjyw · 2022-04-22T21:29:06Z

src/python/pants/backend/python/subsystems/setup.py

-            lockfile/constraints file instead of just the relevant subset. This can improve
+            lockfile file instead of just the relevant subset.
+
+            We generally do not recommend this if `[python].lockfile_generator` is set to `"pex"`


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."

Help should not usually appeal to history

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

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.

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

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.

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.

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

src/python/pants/backend/python/subsystems/setup.py

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

benjyw · 2022-04-24T19:17:03Z

src/python/pants/backend/python/subsystems/setup.py

-            lockfile/constraints file instead of just the relevant subset. This can improve
+            lockfile file instead of just the relevant subset.
+
+            We generally do not recommend this if `[python].lockfile_generator` is set to `"pex"`


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.

benjyw · 2022-04-24T19:18:22Z

src/python/pants/backend/python/subsystems/setup.py

-            lockfile/constraints file instead of just the relevant subset. This can improve
+            lockfile file instead of just the relevant subset.
+
+            We generally do not recommend this if `[python].lockfile_generator` is set to `"pex"`


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.

Update help for python resolves with recent improvements

e03736b

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano added needs-cherrypick category:documentation labels Apr 22, 2022

Eric-Arellano added this to the 2.11.x milestone Apr 22, 2022

Eric-Arellano requested review from stuhood, benjyw and thejcannon April 22, 2022 19:25

Eric-Arellano commented Apr 22, 2022

View reviewed changes

benjyw reviewed Apr 22, 2022

View reviewed changes

Review feedback. Thank you Benjy!

eabee9f

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano mentioned this pull request Apr 22, 2022

Interpreter constraints and multiple resolves are confusing #14863

Closed

benjyw approved these changes Apr 24, 2022

View reviewed changes

Eric-Arellano merged commit 9726c85 into pantsbuild:main Apr 25, 2022

Eric-Arellano deleted the refresh-python-help branch April 25, 2022 19:52

Eric-Arellano removed the needs-cherrypick label Apr 26, 2022

Eric-Arellano removed this from the 2.11.x milestone Apr 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `help` for python resolves with recent improvements #15228

Update `help` for python resolves with recent improvements #15228

Eric-Arellano commented Apr 22, 2022

Eric-Arellano left a comment

Eric-Arellano Apr 22, 2022

Eric-Arellano Apr 22, 2022

thejcannon Apr 22, 2022

kaos Apr 25, 2022

kaos Apr 25, 2022

kaos Apr 25, 2022

Eric-Arellano Apr 22, 2022

Eric-Arellano Apr 22, 2022

Eric-Arellano Apr 22, 2022

Eric-Arellano Apr 22, 2022

Eric-Arellano Apr 22, 2022

benjyw left a comment

benjyw Apr 22, 2022

Eric-Arellano Apr 22, 2022

benjyw Apr 23, 2022

Eric-Arellano Apr 23, 2022

benjyw Apr 24, 2022

benjyw Apr 24, 2022

Eric-Arellano Apr 25, 2022

benjyw Apr 24, 2022

benjyw Apr 24, 2022

	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

Update help for python resolves with recent improvements #15228

Update help for python resolves with recent improvements #15228

Conversation

Eric-Arellano commented Apr 22, 2022

Eric-Arellano 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

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

benjyw 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

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

Update `help` for python resolves with recent improvements #15228

Update `help` for python resolves with recent improvements #15228