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.

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

[WIP] Generate lockfiles with each possible Python version #12389

Closed
wants to merge 1 commit into from

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Jul 21, 2021

To *workaround #12200, we generate a unique lockfile for each interpreter in the constraint range. For example, with ['>=3.6', '<3.8'], we generate with 3.6 and 3.7. Then, per #12362, we will be able to merge these all into a single unified lockfile thanks to environment markers like python_version == '3.6'.

This sets up generating a lockfile with each major-minor interpreter version:

18:34:43.86 [INFO] Completed: Build pip_compile.pex with Python 3.9
18:34:44.79 [INFO] Completed: Build pip_compile.pex with Python 3.8
18:34:45.97 [INFO] Completed: Generate lockfile for isort with Python 3.9
18:34:47.96 [INFO] Completed: Generate lockfile for isort with Python 3.8

It also warns or errors if any interpreters are missing:

18:34:42.07 [WARN] Could not find Python interpreters for the following Python versions when generating a lockfile for yapf:

  • Python 3.6 (Constraint: CPython==3.6.*)
  • Python 3.7 (Constraint: CPython==3.7.*)
  • Python 3.10 (Constraint: CPython==3.10.*)

Because some Python dependencies are only needed when using a particular Python version, Pants needs to generate a lockfile with each Python interpreter possibly used by your interpreter constraints. Pants will then merge all the lockfiles into a single, universal lockfile.

To fix this, please either install the missing Python interpreters and ensure they're discoverable via [python-setup].interpreter_search_path, or tighten your interpreter constraints. (Tip: Pyenv can be useful to install multiple interpreter versions.)

However, this does not yet implement the merging - for now, we take the lowest lockfile generated (e.g. 3.6 if constraints are 3.6+).

*As explained in #12463, this approach of "pessessimistic generation" is fallible. It handles interpreter constraints, but not platforms. See #12463 (comment) for a failing case.

[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]
@jsirois
Copy link
Contributor

jsirois commented Jul 21, 2021

Is post-processing resolves for wheel tags and environment markers actually triggered part of the plan? For some resolves, iterating the IC range won't be needed and post processing will prove that and avoid invalid failures / warnings.

@Eric-Arellano
Copy link
Contributor Author

Is post-processing resolves for wheel tags and environment markers actually triggered part of the plan?

I thought we could get away with not doing this, but in drafting a response to you yesterday, I'm not certain.

In that investigation, I also found several deficiencies of pip-compile (Slack thread):

  1. Does not use the new resolver (thread above this)
  2. Cannot do foreign platform resolves currently
  3. FWICT, cannot do this post-analysis of looking at .whl files.

I'm going to close this PR because if we need to use Pex to generate the lockfile, this implementation will likely change.

@Eric-Arellano Eric-Arellano deleted the gen-all-interps branch July 23, 2021 18:29
Eric-Arellano added a commit that referenced this pull request Jul 26, 2021
These tool lockfiles are trickier than #12357 because they get their interpreter constraints from the users' code, rather than subsystems. So, when generating a lockfile, we query all their code to calculate the constraint.

It's even trickier that Bandit and Flake8 partition by interpreter constraints, e.g. Py27 code running separately than Py37. Ideally, the single lockfile will be compatible with all partitions. To do this, we OR each distinct IC encountered. Using the technique from #12389, we'll try to generate a lockfile for each possible interpreter and then will merge into a single lockfile.

Finally, this updates Black's generation to mirror our actual logic: if the user's code is Py38+ only, we automatically default to the ICs being Py38+ instead of Py36+.

--

With this PR, we have now implemented lockfiles for:

- Bandit
- Black
- Docformatter
- Flake8
- isort
- Yapf
- Coverage.py
- Lambdex
- Protobuf MyPy plugin
- setuptools

These remain:
- MyPy
- Pylint
- Pytest
- IPython
 - pip-tools

[ci skip-rust]
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano restored the gen-all-interps branch August 5, 2021 23:33
@Eric-Arellano
Copy link
Contributor Author

The outcome of #12463 is that we almost certainly should implement post-processing, as opposed to this PR's approach of "pessimistic generation". We also are planning to switch from pip-compile to Pex generating lockfiles, per #12470.

However, that project is going to take some time and won't make it into Pants 2.7. We'd like for Pants 2.7 to have at least experimental support for lockfiles, which will allow us to get feedback from early adopters + is generally an improvement over the status quo, even with the current flaws.

In a bid to make Pants 2.7's experimental lockfile support more robust, I'm going to timebox myself in reviving this PR and implementing some type of merging, even if that is as crude as prompting the user to manually fix any discrepencencies. This code will, unfortunately, be temporary, but I think it will be necessary to getting 2.7's lockfile support useable enough for early adopters.

@Eric-Arellano Eric-Arellano reopened this Aug 5, 2021
@Eric-Arellano
Copy link
Contributor Author

Because Poetry already robustly handles environment markers like interpreter constraints (#12470 (comment)), this PR and "pessimistic generation" is not necessary.

@Eric-Arellano Eric-Arellano deleted the gen-all-interps branch August 6, 2021 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants