Add [python-setup].disable_mixed_interpreter_constraints for repositories not using the interpreter_constraints field
#12495
Conversation
… performance in repos with a single Python constraint # 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]
Eric-Arellano
changed the title
[python-setup].disable_mixed_interpreter_constraints to improve performance in repos with a single Python constraint[python-setup].disable_mixed_interpreter_constraints to improve performance in repos with a single Python constraint
# 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]
|
As discussed in the thread, I think that this is a lower priority than fixing the actual performance issue, especially if they're both going to require a deprecation cycle. |
# 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]
To fully fix all of the performance issues—particularly how slow MyPy is to partition because it has to look at transitive closures—we need to fix #11270, which is blocked on #11269. This is all before even considering lockfiles, I'm purely talking about status quo perf of MyPy. Certainly, we should fix these underlying performance issues. But in the meantime, a) we can get real wins for users now, and b) even with the best optimizations possible, we would still be doing wasted work for a niche feature most orgs don't use (mixed ICs). I do think this is valuable to land now. Fwit, I'm not turning on this option for pantsbuild.pants so that we still feel the pain of these performance issues and are more motivated to fix them. |
The proposal I made in the thread makes the transitive lookups per-root unnecessary: you'd only do a transitive lookup after partitioning, per-partition. So it makes #11270 much less necessary. |
) ### Shading These tools all run with user requirements. As before, we combine the "tool pex" with the user requirements pex via `--pex-path`. I thought that tool lockfiles would cause issues that it's likely for the tool requirements to conflict with the user requirements. But turns out, this has always been the case in Python land. Before, the tool would still have pinned deps in its PEX file, only, those pins might float over time without a lockfile. This PR changes nothing in terms of the need for shading (#9206). ### Performance concerns As with Flake8, Bandit, and Setuptools, we must consult the codebase to determine which interpreter constraints the tool will be run with. This does have a performance hit, especially for generating Pytest's lockfile because we must consider the transitive deps of each `python_tests` target. There are four proposals floating to improve the performance. All are compatible with the others: 1. When generating the lockfile via `./pants tool-lock`, continue to inspect the whole repo. But when checking at call sites if the lockfile is stale, only inspect the code in question: create what the `PythonToolLockfile` would be if it were just for that code, and ensure the ICs are compatible w/ the checked-in file. 2. Stop treating the IC field as the constraints for the source itself, and start treating it as constraints for the whole transitive closure. This would allow us to look only at the target roots, w/o needing to resolve transitive dependencies. 3. Allow repos to opt-out of mixed interpreter constraints and only set ICs via `[python-setup].interpreter_constraints` (#12495). This has no impact if you are using mixed ICs, but greatly speeds things up for other repos not using the mechanism. 4. Add an ignore option to our check for stale lockfiles. Users could risk ignoring for desktop builds, and then error in CI. For now, this PR provides a common denominator.
Eric-Arellano
changed the title
[python-setup].disable_mixed_interpreter_constraints to improve performance in repos with a single Python constraint[python-setup].disable_mixed_interpreter_constraints for repositories not using the interpreter_constraints field
|
Also, I'm okay with not landing this PR in particular if we decide the temporary option isn't worth the churn. The more important change I care about is no longer activating |
|
To chime in, I think on balance that it's a good idea to reduce niche complexity unless needed. As long as we properly document how to use Pants with mixed interpreters. |
Agreed. Which will be nothing more than moving the snippet It will be really easy for users to activate this niche feature: it's a one-line config change. |
Doesn't this do the opposite thing? To allow for disabling a feature (which we have other pathways to make faster), we're increasing complexity by 200+ lines, and adding more steps for developers. |
Bump on the part below. If I could go back, I would not have put up this PR first and I would have gone straight for the PR to deprecate using
Does that make sense? This PR's 200+ lines and new option are in no way necessary for the proposed change. |
|
Ah, I meant complexity for end users, not for us. |
It doesn't make sense. Most of the new lines here are if statements of the form: And the new option I'm referring to is the enabling of the backend itself: you have to figure out that you need to enable the backend to get the feature (and we'd likely need a pointer if someone tries using the field).
This is more complexity for end users, and for us. For example: on the docsite (and probably also via an assist in the code) we will need to call out that the interpreter constraints field only exists if you've enabled a particular backend, and how to do that. |
Sure, although that is already something we need to solve with #12523. For
I expect the main entry point will be a codebase admin deciding if they want this niche feature while reading https://www.pantsbuild.org/docs/python-interpreter-compatibility#using-multiple-python-versions-in-the-same-project. Once decided, others don't need to think about it. It's one time and done by an admin.
This is the same for using any of our backends like Flake8. Liam already added an error message linking to https://www.pantsbuild.org/docs/enabling-backends#available-backends when you trying using an unrecognized symbol, which users would get when using the And we can continue to improve discoverability with
Counterpoint is that we're imo removing complexity for the organizations that don't want to use this advanced and niche feature. It won't show up in |
The docs question is interesting, but isn't it the case that we need to do this sort of thing in general? |
|
Closing in favor of #12652. |
The
interpreter_constraintsfield allows your repo to support multiple distinct ICs, e.g. some Py2 only code and Py3 only code. Pants has code to then robustly support this mixing, e.g. partitioning Flake8 and Bandit based on interpreter constraints. That's a really neat feature, but not used by many orgs.At the same time, we have the
pants.backend.python.mixed_interpreter_constraintsbackend already, which adds thepy-constraintsgoal.This PR is prework to start requiring activating
pants.backend.python.mixed_interpreter_constraintsfor theinterpreter_constraintsfeature to be registered. Two main reasons:interpreter_constraintsactivated by default, it's easy for an engineer to accidentally use this advanced feature, resulting in functionality the build engineers may have intentionally not wanted to use.Deprecation plan
interpreter_constraintsfield if the backendpants.backend.python.mixed_interpreter_constraintsis not activated.--disable-mixed-interpreter-constraintsto allow getting the performance boosts right away.--disable-mixed-interpreter-constraintsis temporary and will be removed once we switch theinterpreter_constraintsfield to only be registered if the backend is activated.[ci skip-build-wheels]