Add experimental lockfile support to IPython, Pytest, and Pylint #12491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ci skip-rust] [ci skip-build-wheels]
Eric-Arellano
force-pushed
the
pytest-lockfile
branch
from
af4fa99
to
44d4680
Compare
# 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
# 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]
stuhood
approved these changes
Aug 6, 2021
Comment on lines
+143
to
+148
| # While Flake8 will run in partitions, we need a single lockfile that works with every | ||
| # partition. | ||
| # | ||
| # This ORs all unique interpreter constraints. When paired with | ||
| # `InterpreterConstraints.partition_by_major_minor_versions`, the net effect is that every | ||
| # possible Python interpreter used will be covered. |
There was a problem hiding this comment.
Is it worth deduping these IC calculations now across Bandit/Flake8/etc, or are you expecting more change soon that might cause them to need to be separate rules like this?
There was a problem hiding this comment.
I wanted to dedupe, but we can't because of the skip_field options. I think the correctness could be important here. Let's say you don't run Pylint on your Py2 only code: we need to ignore those targets when determining the ICs for the lockfile.
Maybee we decide to give up on that correctness, but I want to start with a baseline of what is fully correct.
There was a problem hiding this comment.
Sure... but that filtering feels like it could be generalized?
Merged
Eric-Arellano
added a commit
that referenced
this pull request
Aug 18, 2021
#12491 did not properly add lockfiles to Pylint. The option was registered, but not being consumed. This was trickier to implement than other tool lockfiles because we must consider `[pylint].source_plugins`, whose requirement strings and interpreter constraints must be considered. [ci skip-rust] [ci skip-build-wheels]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_teststarget.There are four proposals floating to improve the performance. All are compatible with the others:
./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 thePythonToolLockfilewould be if it were just for that code, and ensure the ICs are compatible w/ the checked-in file.[python-setup].interpreter_constraints(Add[python-setup].disable_mixed_interpreter_constraintsfor repositories not using theinterpreter_constraintsfield #12495). This has no impact if you are using mixed ICs, but greatly speeds things up for other repos not using the mechanism.For now, this PR provides a common denominator.