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

Add ability to extend a PEX lockfile without modifying existing entries #15704

Open
danxmoran opened this issue May 31, 2022 · 15 comments
Open
Assignees
Labels
backend: Python Python backend-related issues enhancement

Comments

@danxmoran
Copy link
Contributor

Is your feature request related to a problem? Please describe.

When I add a new python_requirement to my repo, I run ./pants generate-lockfiles. In addition to adding a new entry for the requirement, the goal sometimes updates existing entries in my lockfile (i.e. to bump the patch version of a transitive dependency). Either before or during code review I'll typically end up manually reverting all the "unrelated" changes to keep my commit focused - this is easy to forget to do, so accidental upgrades sometimes creep through.

The fact that generate-lockfiles might update existing entries in-place also prevents us from adding a CI check to assert that the lockfiles are up-to-date - ideally we'd run generate-lockfiles and fail if there was any diff, but we can't do that if there's a chance that unrelated changes might cause the check to fail.

Describe the solution you'd like

I'd like a new option on generate-lockfiles that tells the command to only add new entries to the lock, and not update existing ones. If it's not possible to add a new entry without updating an existing one, I'd like the command to fail with a description of the version conflict.

@jsirois
Copy link
Contributor

jsirois commented May 31, 2022

@danxmoran this amounts to a feature request against Pex.

Today Pex supports:

  1. pex3 lock update my.lock to update a whole lockfile with latest that meet the original requirement constraints.
  2. pex3 lock update my.lock -p just_update_this_project -p oh_and_this_one_too<2.0 to update individual projects already in the lockfile (say you want to work around a CVE).

Pants does not expose these update features. Really it doesn't expose 2 since 1 is effectively just like re-running pex3 lock create ... with the original requirements.

So Pex would need a new update feature:
3. pex3 lock update my.lock -a add_this_dep_if_possible_without_updating_the_rest_of_the_lock

Today, when you try to do this using the existing update -p feature you get:

$ pex3 lock create requests -o lock
$ pex3 lock update -p "charset-normalizer<2.0.10" lock --dry-run
Would update charset-normalizer from 2.0.12 to 2.0.9 in lock generated by cp311-cp311-manylinux_2_35_x86_64.
$ pex3 lock update -p ansicolors lock --dry-run
The following updates were requested but there were no matching locked requirements found in lock:
+ ansicolors

Once Pex implements 3, then Pants would need to tackle the issue of lockfile updates and take these features (2 & 3) on board.

@danxmoran
Copy link
Contributor Author

@jsirois thanks for the context! I opened an issue in the PEX repo to track that piece of it.

@stuhood
Copy link
Member

stuhood commented Jun 1, 2022

The need for lockfile updates as covered above makes sense.

But as to this point:

The fact that generate-lockfiles might update existing entries in-place also prevents us from adding a CI check to assert that the lockfiles are up-to-date - ideally we'd run generate-lockfiles and fail if there was any diff, but we can't do that if there's a chance that unrelated changes might cause the check to fail.

This could/should probably be covered by a ./pants generate-lockfiles --check mode which validates that the inputs for the lockfile are unchanged (in internals terms, that's having the same "lockfile metadata", which is stored in the header of the lockfile). If that is something that you'd be interested in seeing as well, it's almost certainly an easier problem... should probably just be in a different ticket.

@Eric-Arellano
Copy link
Contributor

This could/should probably be covered by a ./pants generate-lockfiles --check mode which validates that the inputs for the lockfile are unchanged

Yeah, I think that is easy for us to do.

@danxmoran
Copy link
Contributor Author

A dedicated "check" mode would be great! I'll file another ticket for that

@jsirois
Copy link
Contributor

jsirois commented Jun 1, 2022

I missed the bit about the check. FWIW, this exists today over in Pex:

Prep a lock:

$ pex3 lock create requests -o lock

Update it right away; so nothing new: no stdout and exit code 0.

$ pex3 lock update lock --dry-run

Prep a simulated old lock

$ pex3 lock create requests==2 -o lock --indent 2
$ cp lock lock-old-simulation
$ vi lock-old-simulation
$ diff -u lock lock-old-simulation 
--- lock	2022-05-31 18:44:41.198081339 -0700
+++ lock-old-simulation	2022-05-31 18:45:10.407617559 -0700
@@ -32,7 +32,7 @@
   "pex_version": "2.1.92",
   "prefer_older_binary": false,
   "requirements": [
-    "requests==2"
+    "requests"
   ],
   "requires_python": [],
   "resolver_version": "pip-legacy-resolver",

Update it: stdout contains updates and exit code 0

$ pex3 lock update lock-old-simulation --dry-run
Would update requests from 2 to 2.27.1 in lock generated by cp311-cp311-manylinux_2_35_x86_64.

So, that gives you --check via --dry-run with the stdout being what is out of date.

Perhaps Pants just running pex2 lock create ... and diffing against the current lock is more what you want, but it's a lot more output and it won't currently work since things like the platform_tag and pex version can change from run-to-run if the Python picked to run the lock changes or Pex has been upgraded even though neither is material to the --check.

@jsirois
Copy link
Contributor

jsirois commented Jun 1, 2022

I guess adding an optional 'check' argument to --dry-run; i.e: --dry-run check which ran a --dry-run but exited non-zero if there are updates might make sense.

@jsirois
Copy link
Contributor

jsirois commented Jun 5, 2022

The pex3 lock update --dry-run check implementation is here: pex-tool/pex#1799

@taylorm0192
Copy link

Had some discussion w/ Stu on this issue on Slack (https://pantsbuild.slack.com/archives/C046T6T9U/p1657905057557149).

Settling on the idea of using as open as possible constraints for python_requirements in generating the python-default resolve, we would like to reduce the testing overhead of adding a new requirement (esp. in the early days), so to us this would look like pants generate-lockfiles --pin-already-resolved-requirements --resolve=python-default. That is, attempt to generate the new lockfile, but replace any requirement constraints with pinned constraints from the current lockfile. If it ends up not being possible, error out.

Would also be helpful to get a diff/migration report when the goal is completed successfully for a pre-existing lock.

@danxmoran
Copy link
Contributor Author

I think my ideal UX for this would be for generate-lockfiles to only update things that have changed in python_requirement targets by default, and to only do a full regeneration if you pass an option. I've almost never wanted generate-lockfiles to bump the versions of floating transitive dependencies - instead I typically want it to either add a brand new dependency or update the version of a top-level dep.

@Eric-Arellano
Copy link
Contributor

I think my ideal UX for this would be for generate-lockfiles to only update things that have changed in python_requirement targets by default, and to only do a full regeneration if you pass an option.

Thanks for the suggestion! I think this would be feasible to pull off because we have the lockfile header with the prior requirement versions written down. If that lockfile header is missing, then we do the next best thing of generating the whole lock.

@cognifloyd
Copy link
Member

This seems related to #12880

@cognifloyd
Copy link
Member

Also related: #15275

@stuhood stuhood self-assigned this Mar 23, 2023
@benjyw benjyw assigned benjyw and unassigned stuhood Mar 28, 2023
kaos pushed a commit that referenced this issue Jul 14, 2023
This regenerates Pants' main `python-default` resolve lock file for the
more restricted `==3.9.*` interpreter constraints that have been
recently instituted (618d627, #19096).
This lock wasn't regenerated as part of that change, but future changes
to dependencies will be using the new scheme, so this PR does it
explicitly as a dedicated step to reduce the spurious changes.

This involves two sort of changes:

- the 'necessary' removal of dependencies that are no longer necessary,
like `importlib-metadata`
- 'unnecessary' other upgrades, to the latest (constraint-satisfying)
version of existing deps
(#15704).

Diffs:

```
Lockfile diff: 3rdparty/python/user_reqs.lock [python-default]
                                                                  
==                    Upgraded dependencies                     ==

  anyio                          3.6.2        -->   3.7.1
  asgiref                        3.6.0        -->   3.7.2
  charset-normalizer             3.1.0        -->   3.2.0
  click                          8.1.3        -->   8.1.4
  httptools                      0.5.0        -->   0.6.0
  pluggy                         1.0.0        -->   1.2.0
  pydantic                       1.10.7       -->   1.10.11
  pyparsing                      3.0.9        -->   3.1.0
  python-dotenv                  0.21.1       -->   1.0.0
  requests                       2.30.0       -->   2.31.0
  ujson                          5.7.0        -->   5.8.0
  urllib3                        2.0.2        -->   2.0.3
                                                                  
==                      Added dependencies                      ==

  exceptiongroup                 1.1.2
                                                                  
==                     Removed dependencies                     ==

  importlib-metadata             6.6.0
  importlib-resources            5.0.7
  zipp                           3.15.0
```
cburroughs added a commit to cburroughs/pants that referenced this issue Jan 16, 2024
Changelogs:
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.157
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.158
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.159

```
Lockfile diff: 3rdparty/python/user_reqs.lock [python-default]

==                    Upgraded dependencies                     ==

  attrs                          23.1.0       -->   23.2.0
  pex                            2.1.156      -->   2.1.159
```

Enables repl tab completion for pantsbuild#20389
Fixes a bug with `pex3 lock update` of relevance for pantsbuild#15704
benjyw pushed a commit that referenced this issue Jan 16, 2024
Changelogs:
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.157
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.158
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.159

```
Lockfile diff: 3rdparty/python/user_reqs.lock [python-default]

==                    Upgraded dependencies                     ==

  attrs                          23.1.0       -->   23.2.0
  pex                            2.1.156      -->   2.1.159
```

Enables repl tab completion for #20389
Fixes a bug with `pex3 lock update` of relevance for #15704
cburroughs added a commit to cburroughs/pants that referenced this issue Jan 26, 2024
Changelogs:
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.160
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.161

```
Lockfile diff: 3rdparty/python/user_reqs.lock [python-default]

==                    Upgraded dependencies                     ==

  cryptography                   41.0.7       -->   42.0.1
  pex                            2.1.159      -->   2.1.161
  pluggy                         1.3.0        -->   1.4.0
  pydantic                       1.10.13      -->   1.10.14
  python-dotenv                  1.0.0        -->   1.0.1
```

Further suport relative to pantsbuild#15704
cburroughs added a commit to cburroughs/pants that referenced this issue Feb 6, 2024
Changelogs:
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.160
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.161
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.162

```
Lockfile diff: 3rdparty/python/user_reqs.lock [python-default]

==                    Upgraded dependencies                     ==

  certifi                        2023.11.17   -->   2024.2.2
  cryptography                   41.0.7       -->   42.0.2
  pex                            2.1.159      -->   2.1.162
  pluggy                         1.3.0        -->   1.4.0
  pydantic                       1.10.13      -->   1.10.14
  python-dotenv                  1.0.0        -->   1.0.1
  urllib3                        2.1.0        -->   2.2.0
```

Further support relative to pantsbuild#15704
Fixes pantsbuild#20474
huonw pushed a commit that referenced this issue Feb 6, 2024
Changelogs:
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.160
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.161
 * https://github.com/pantsbuild/pex/releases/tag/v2.1.162

```
Lockfile diff: 3rdparty/python/user_reqs.lock [python-default]

==                    Upgraded dependencies                     ==

  certifi                        2023.11.17   -->   2024.2.2
  cryptography                   41.0.7       -->   42.0.2
  pex                            2.1.159      -->   2.1.162
  pluggy                         1.3.0        -->   1.4.0
  pydantic                       1.10.13      -->   1.10.14
  python-dotenv                  1.0.0        -->   1.0.1
  urllib3                        2.1.0        -->   2.2.0
```

Further support relative to #15704
Fixes #20474
cburroughs added a commit to cburroughs/pants that referenced this issue Feb 24, 2024
Previously to pass along only/no-binary options Pants would create a
"requirements" file and use Pip's ability to put cli args in said file
[1].  This worked fine to generate the artifacts in a lockfile from
scratch, but meant that the lockfile itself did not contain sufficient
information to carry its own configuration forward.  That is if one
did `pex3 lock update` on a Pants created lockfile that originally
required everything to use wheels, that specification could not be
preserved in the updated lockfile.  Since v2.1.161 [2] Pex has
provided `--only-{wheel,build}` options to support this directly.

[1] https://pip.pypa.io/en/stable/reference/requirements-file-format/#global-options

[2] https://github.com/pex-tool/pex/releases/tag/v2.1.161

ref pantsbuild#15704
huonw pushed a commit that referenced this issue Feb 28, 2024
Previously to pass along only/no-binary options Pants would create a
"requirements" file and use Pip's ability to put cli args in said file
[1]. This worked fine to generate the artifacts in a lockfile from
scratch, but meant that the lockfile itself did not contain sufficient
information to carry its own configuration forward. That is if one did
`pex3 lock update` on a Pants created lockfile that originally required
everything to use wheels, that specification could not be preserved in
the updated lockfile. Since v2.1.161 [2] Pex has provided
`--only-{wheel,build}` options to support this directly.

[1]
https://pip.pypa.io/en/stable/reference/requirements-file-format/#global-options

[2] https://github.com/pex-tool/pex/releases/tag/v2.1.161

ref #15704
cburroughs added a commit to cburroughs/pants that referenced this issue Mar 29, 2024
Changelog: https://github.com/pex-tool/pex/releases/tag/v2.3.0

 - `sync` of interest for pantsbuild#15704
 - error message clarification regarding pantsbuild#15062
 - fix for explicit flags as implemented pantsbuild#20598

```
Lockfile diff: 3rdparty/python/user_reqs.lock [python-default]

==                    Upgraded dependencies                     ==

  asgiref                        3.7.2        -->   3.8.1
  cryptography                   42.0.3       -->   42.0.5
  pex                            2.2.1        -->   2.3.0
  pyparsing                      3.1.1        -->   3.1.2
  python-dateutil                2.8.2        -->   2.9.0.post0
  sniffio                        1.3.0        -->   1.3.1
```
huonw pushed a commit that referenced this issue Mar 31, 2024
Changelog: https://github.com/pex-tool/pex/releases/tag/v2.3.0

 - `sync` of interest for #15704
 - error message clarification regarding #15062
 - fix for explicit flags as implemented #20598

```
Lockfile diff: 3rdparty/python/user_reqs.lock [python-default]

==                    Upgraded dependencies                     ==

  asgiref                        3.7.2        -->   3.8.1
  cryptography                   42.0.3       -->   42.0.5
  pex                            2.2.1        -->   2.3.0
  pyparsing                      3.1.1        -->   3.1.2
  python-dateutil                2.8.2        -->   2.9.0.post0
  sniffio                        1.3.0        -->   1.3.1
```
@jonasrauber
Copy link
Contributor

This would be nice! For comparison, poetry has a --no-update flag on it's lock command that does exactly this: https://python-poetry.org/docs/cli/#lock

@jsirois
Copy link
Contributor

jsirois commented Jul 12, 2024

As has Pex, since 2022. Pants has been exceedingly slow to adopt the Pex support. There was a recent engagement that produced fixes and more features from Pex for all this, but still no discernible motion from Pants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues enhancement
Projects
Status: No status
Development

No branches or pull requests

9 participants