-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
[internal] Add experimental per-tool lockfiles with Docformatter as an example #12346
Conversation
# 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]
# 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]
# 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]
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.
A followup will wire up the remaining relevant tools like Flake8 and Black
"Path to a lockfile used for installing the tool.\n\n" | ||
"Set to the string '<default>' to use a lockfile provided by " | ||
"Pants, so long as you have not changed the `--version`, `--extra-requirements`, " | ||
"and `--interpreter-constraints` options. See {} for the default lockfile " |
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 will be a GitHub URL once I add a util to generate that in a follow up.
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.
Another option would be to literally load the resource as the default value of a field. That would allow you to both set it and override it in the option, without putting it on disk...? Not sure if that's actually great, but.
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 override it in the option
Yeah I thought about that, but didn't like it from an ergonomics perspective:
- these files can be huge. No way you'll feasibly set it in CLI or env var, and setting in
pants.toml
is clunky. - Pollutes
./pants help
because the value can be so big.
Instead, I think John's idea of embedding a resource is a good way to get it To Just Work by default, while still being flexible to overriding.
advanced=True, | ||
help=( | ||
"Path to a lockfile used for installing the tool.\n\n" | ||
"Set to the string '<default>' to use a lockfile provided by " |
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.
Should this be a different special string, like <builtin>
?
# 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]
By "it will also work with CLI specs" do you mean that you can use a target to build the tool from? That would be pretty epic. |
# 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]
Possibly! Semantics still need to be worked out, including how it combines with user lockfiles. |
@@ -77,6 +77,7 @@ interpreter_constraints = [">=3.7,<3.10"] | |||
|
|||
[docformatter] | |||
args = ["--wrap-summaries=100", "--wrap-descriptions=100"] | |||
experimental_lockfile = "src/python/pants/backend/python/lint/docformatter/lockfile.txt" |
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.
Bit of a tangent (because this needs to be nested more deeply because we re-generate it when we change the default), but: there are lots of potential conventions to be formed, but something like 3rdparty/lock/${resolve_name}.*.txt
is likely to make the most sense for end users.
For performance reasons, end users are really going to want to share resolves within the repo rather than leaning into a pattern where every single binary has their own unique resolve. Since we're not anticipating "hundreds" of them (a handful, most likely... at most a dozen?) centralizing them seems like it encourages that.
# TODO: Figure out which interpreter constraints to use...Likely get it from the | ||
# transitive closure. When we're doing a single global lockfile, it's fine to do that, | ||
# but we need to figure out how this will work with multiple resolves. |
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.
The same set of @rules
that requests a resolve will eventually also be triggering the sideeffect of creating the lockfile. It has all of the context needed: exactly which constraints/platforms to use, etc.
But that actually suggests an interesting middle ground. Perhaps what we could do (until that codepath can trigger its own sideeffect via #12014), would be to have the warning/error that we render when a lockfile is stale actually render the exact command to run to regenerate the lockfile. Something like:
[ERROR]: The resolve for ${named_resolve} (at $lockfile) was stale. To lock the requirements, please run:
./pants lock --interpreter-constraints="${constraints_requested_in_relative_codepath}" $lockfile
...and then when we're able to do it automatically via #12014, we can change the default from warn/error to "regenerate".
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.
...or it could even specify the entire set of input requirements as an argument...
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 makes sense. I'm envisioning the same thing.
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.
It's not blocking feedback, but this would pretty fundamentally affect the design, and is the reason I didn't immediately shipit: with this design, you don't need a separate tool-lock
command: instead, when you run pants without a lockfile specified, it will warn/error with the precise command to run to generate the lockfile that will make the warning go away.
If you think you want to land this as is and follow up, that's fine though.
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.
Ah to be clear, I want the tool-lock
goal to go away and be unified with the lock
goal. tool-lock
is only to make forward progress.
if docformatter.lockfile == "<none>": | ||
requirements = PexRequirements(docformatter.all_requirements) | ||
elif docformatter.lockfile == "<default>": | ||
requirements = PexRequirements( | ||
file_content=FileContent( | ||
"docformatter_default_lockfile.txt", | ||
importlib.resources.read_binary( | ||
"pants.backend.python.lint.docformatter", "lockfile.txt" | ||
), | ||
) | ||
) | ||
else: | ||
requirements = PexRequirements( | ||
file_path=docformatter.lockfile, | ||
file_path_description_of_origin="the option `[docformatter].experimental_lockfile`", | ||
) |
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.
Rather than being something that a tool should explicitly request, this feels like an argument that should be passed down via PexRequest
. Maybe a Resolve(name=.., file=.., default=..)
parameter, which PexRequest
would switch on behavior with? An end-user resolve would have no default, etc.
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.
Maybe! I want to be careful to not factor too much until we have better insight on what this will all look like.
"Path to a lockfile used for installing the tool.\n\n" | ||
"Set to the string '<default>' to use a lockfile provided by " | ||
"Pants, so long as you have not changed the `--version`, `--extra-requirements`, " | ||
"and `--interpreter-constraints` options. See {} for the default lockfile " |
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.
Another option would be to literally load the resource as the default value of a field. That would allow you to both set it and override it in the option, without putting it on disk...? Not sure if that's actually great, but.
@@ -99,6 +144,7 @@ class PexRequest(EngineAwareParameter): | |||
repository_pex: Pex | None | |||
additional_args: Tuple[str, ...] | |||
pex_path: Tuple[Pex, ...] | |||
is_lockfile: bool |
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.
Should this be encoded in PexRequirements
?
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.
Oh, hm maybe, yeah. I'm currently playing with a couple different factorings for a followup that will add per-tool lockfiles to the rest of the tools. Is it okay to experiment with that in the followup?
|
||
constraint_file_digest = EMPTY_DIGEST | ||
if request.apply_requirement_constraints and python_setup.requirement_constraints is not None: | ||
if ( | ||
not request.is_lockfile and request.apply_requirement_constraints |
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.
Should/does the constructor validate that only one of these is true at once? Alternatively, this seems like another case where a union
more accurately represents which cases can be true simultaneously: resolve_file: Lockfile | ConstraintsFile | None
.
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.
apply_requirement_constraints
defaults to True
. To avoid having to update lots of call sites, I kept that default for now. It felt redundant to set both apply_requirement_constraints=False
and is_lockfile=True
.
A union could make sense, although it's frustrating that Python doesn't have ADTs and we would need to define two new types. I'd like to avoid defining new types until we have more experience with the rest of the project.
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.
See https://github.com/pantsbuild/pants/pull/12346/files#r669986521, but shipit if you think it's better as a followup.
For per-tool lockfiles in #12346, we will be embedding the default lockfile as a resource. We want to allow users to see what is in the lockfile, but don't want to dump all the contents to `./pants help`. Instead, this new `git_url()` util will allow us to link to the URL for the text file on GitHub. [ci skip-rust] [ci skip-build-wheels]
### Internal * [internal] Manually fix Black lockfile to handle interpreter constraints ([#12366](#12366)) * Revert "Prefix the entire setup.py chroot. (#12359)" ([#12370](#12370)) * [internal] Cache native client binary in CI ([#12355](#12355)) * Prefix the entire setup.py chroot. ([#12359](#12359)) * [internal] Fix AWS CLI breaking due to Python 2 usage ([#12364](#12364)) * [Internal] Add `git_url()` helper to `docutil.py` ([#12352](#12352)) * [Internal] Refactor how `PythonToolBase` exposes requirements and interpreter constraints ([#12356](#12356)) * [internal] Add experimental per-tool lockfiles with Docformatter as an example ([#12346](#12346)) * Remove Pants's dpeendency on `requests` ([#12348](#12348)) * [internal] Remove `TwoStepPex` abstraction ([#12343](#12343)) * Add psycopg2-binary to default module mapping ([#12339](#12339)) * [internal] Upgrade toolchain pants plugin to 0.13.1 ([#12338](#12338)) * Add an API to coarsen/partition Targets by their cycles ([#12251](#12251)) * Prepare 2.5.1 ([#12329](#12329)) * Bootstrap fewer JVM versions in Coursier/javac tests to hopefully reduce CI flakiness ([#12325](#12325)) * Native client respects `--concurrent`. ([#12324](#12324)) * Add client lib tests. ([#12322](#12322)) * Special case enum option parse failures. ([#12281](#12281))
Part of #11898.
These lockfiles are only meant for tools that do not load user requirements, like Black and Flake8 but unlike MyPy and Pytest.
Status quo: constraints file and tools
Currently,
[python-setup].requirement_constraints
applies when building tool PEXes, which can be unexpected because the constraints file is typically meant for user requirements, not tool requirements. There is no need to couple these two.The new lockfile support is careful to disable the constraints file when a lockfile is in use.
How users set the lockfile
Each tool will get a new
--lockfile
option. It will default to<default>
, which loads a lockfile provided by Pants. This ensures things work Out Of The Box. For now, it defaults to<none>
, which users will always be able to do to (dangerously) opt out of lockfiles.Otherwise, users can set the option to point to a custom path.
How users can generate a lockfile
If you're using a custom path, you can run
./pants tool-lock
to generate the lockfile. This will look at[tool].version
,[tool].extra_requirements
, and[tool].interpreter_constraints
.Users will first need to update
[tool].lockfile
to the desired destination, then run./pants tool-lock
.In the future, this goal will be unified with
./pants lock
, and it will also work with CLI specs (semantics tbd).How plugins wire up
--lockfile
.ToolSubsystem -> PythonToolLockfileRequest
by pulling from relevant options. This is how we know where and what to generate.PexRequest
and itsPexRequirements
appropriately.Rejected alternative: dedicated targets
Originally, https://docs.google.com/document/d/1bCYb0UQZx9a-9tAagydCN_z3826QRvz_3aVnXKSNTJw/edit proposed having a dedicated target for each tool, like
black_lockfile
andisort_lockfile
. This was largely driven by possibly using targets to model multiple user lockfiles, and wanting parity between per-tool lockfiles and user lockfiles. For example, ideally./pants lock ::
(target specs) will regenerate all lockfiles.This PR de-prioritizes parity with user lockfiles to instead focus on the best semantics possible for per-tool lockfiles. For example, it is imperative that lockfiles Just Work Out Of The Box, and that would be harder to do with a target. It's also really clunky to require N targets for N tools, including polluting
./pants help
.We can get creative to have better parity with user lockfiles, like retconning
./pants lock ::
to still work with these lockfiles.