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

Lock updates support deleting & replacing reqs. #2335

Merged
merged 3 commits into from Jan 22, 2024

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Jan 18, 2024

Previously you could only add new top-level requirements to a lock or
refine constraints against an existing locked item. Now you can -d / --delete-project a top-level requirement from the lock as well as
replace a top-level requirement with -R / `--replace-project.

Fixes #2332 Fixes #2334

Previously you could only add new top-level requirements to a lock or
refine constraints against an existing locked item. Now you can `-d` /
`--delete` a top-level requirement from the lock as well as replace a
top-level requirement with `-p =<req>`, where the leading `=` prefix to
the requirement indicates replacement.

Fixes pex-tool#2332
Fixes pex-tool#2334
@jsirois
Copy link
Member Author

jsirois commented Jan 19, 2024

Ok, another pretty big one. Thanks again for your review efforts.

The real changes are in:

@jsirois jsirois marked this pull request as ready for review January 19, 2024 02:19
pex/cli/commands/lock.py Outdated Show resolved Hide resolved
@jsirois
Copy link
Member Author

jsirois commented Jan 19, 2024

One note to draw attention to is here: #2334 (comment)

Basically, this change introduces everything needed to implement minimal lock auto-update, even for Pants. IOW ~like cargo where if you change the input requirements, the lock just gets auto-minimally updated implicitly. That said, it would be clunky to poke around in the existing lock to come up with the full set of -R <new-req-1> ... -R <new-req-N> -d <removed-proj-1> ... -d <removed-proj-M> to affect that style of update. This change was large; so I'd like to work through this PR and likely release, then circle back and add the lock update with bare requirements / -r requirements feature that will enable doing this automatically.

Copy link
Collaborator

@kaos kaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only done a cursory read through..

pex/asserts.py Outdated
except Exception:
pass

raise AssertionError("\n".join((message, "---", assert_advice)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the pex info potentially large..?
If so, I'd suggest considering having all the details above the message, so that you can read the advice in the terminal directly without having to scroll up to see it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - that makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, now fixed.

When running from the Pex CLI (not inside a PEX):

$ python -c 'from pex.asserts import production_assert; production_assert(False, "Whoops!")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/jsirois/dev/pantsbuild/jsirois-pex/pex/asserts.py", line 67, in production_assert
    raise AssertionError("\n".join(message))
AssertionError: Whoops!
---
Pex 2.1.159
platform: Linux-5.15.90.4-microsoft-standard-WSL2-x86_64-with-glibc2.35
python: 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0]
argv: ['-c']
---
The error reported above resulted from an unexpected programming error which
you should never encounter.

Firstly, please accept our apology!

If you could file an issue with the error and details above, we'd be
grateful. You can do that at https://github.com/pantsbuild/pex/issues/new and
redact or amend any details that expose sensitive information.

When running from inside a PEX:

diff --git a/pex/pex_bootstrapper.py b/pex/pex_bootstrapper.py
index 5fb1ce5b..e9eefe3a 100644
--- a/pex/pex_bootstrapper.py
+++ b/pex/pex_bootstrapper.py
@@ -589,6 +589,10 @@ def bootstrap_pex(
 ):
     # type: (...) -> None

+    from pex.asserts import production_assert
+
+    production_assert(False, "Whoops!")
+
     pex_info = _bootstrap(entry_point)

     # ENV.PEX_ROOT is consulted by PythonInterpreter and Platform so set that up as early as
$ python -mpex -oempty.pex
jsirois@Gill-Windows:~/dev/pantsbuild/jsirois-pex (issues/2332 *) $ ./empty.pex
Traceback (most recent call last):
  File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/home/jsirois/.pex/unzipped_pexes/1d14ae7381aa17f6fbe3bb38e94143068a89091f/__main__.py", line 106, in <module>
    bootstrap_pex(__entry_point__, execute=__execute__, venv_dir=__venv_dir__)
  File "/home/jsirois/.pex/unzipped_pexes/1d14ae7381aa17f6fbe3bb38e94143068a89091f/.bootstrap/pex/pex_bootstrapper.py", line 594, in bootstrap_pex
    production_assert(False, "Whoops!")
  File "/home/jsirois/.pex/unzipped_pexes/1d14ae7381aa17f6fbe3bb38e94143068a89091f/.bootstrap/pex/asserts.py", line 67, in production_assert
    raise AssertionError("\n".join(message))
AssertionError: Whoops!
---
Pex 2.1.159
platform: Linux-5.15.90.4-microsoft-standard-WSL2-x86_64-with-glibc2.35
python: 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0]
argv: ['/home/jsirois/dev/pantsbuild/jsirois-pex/./empty.pex']
PEX-INFO:
{
  "bootstrap_hash": "8b3fa58512e1e46fbf845a9258d0fc83af041aff",
  "build_properties": {
    "pex_version": "2.1.159"
  },
  "code_hash": "c7af8ab820d0b72cc617d52b81c7ee6da79d1858",
  "deps_are_wheel_files": false,
  "distributions": {},
  "emit_warnings": true,
  "excluded": [],
  "ignore_errors": false,
  "includes_tools": false,
  "inherit_path": "false",
  "inject_args": [],
  "inject_env": {},
  "interpreter_constraints": [],
  "max_install_jobs": 1,
  "pex_hash": "1d14ae7381aa17f6fbe3bb38e94143068a89091f",
  "pex_path": "",
  "pex_paths": [],
  "requirements": [],
  "strip_pex_env": true,
  "venv": false,
  "venv_bin_path": "false",
  "venv_copies": false,
  "venv_hermetic_scripts": true,
  "venv_site_packages_copies": false
}
---
The error reported above resulted from an unexpected programming error which
you should never encounter.

Firstly, please accept our apology!

If you could file an issue with the error and details above, we'd be
grateful. You can do that at https://github.com/pantsbuild/pex/issues/new and
redact or amend any details that expose sensitive information.

Also fix constraint update to be skipped when the constraint is just a
project name (i.e.: not actually a constraint).
Copy link
Collaborator

@benjyw benjyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a non-cursory review, and it all looks good to me. I've verified via eyeballing that this should work in the presence of user-provided inconsistently non-canonical project names.

Users will love this (post #2334 (comment) anyway). And once we have that, Pants should be able to auto-update lockfiles when requirements change, a la Cargo.

@jsirois
Copy link
Member Author

jsirois commented Jan 22, 2024

Users will love this (post #2334 (comment) anyway). And once we have that, Pants should be able to auto-update lockfiles when requirements change, a la Cargo.

As that comment alluded, nothing blocks that functionality as soon as this is merged. Its clunky for a human, but Pants is not that. IIUC it is easy for pants to create a lock if none present, or else update the present lock by gathering requirements, compare to prior requirements in lock, separate in 2 groups of changed + new and deleted, call pex3 lock update with the corresponding set of -R and -d. That said, I can see if you want to wait instead of writing a bunch of logic you then turn around and rip out shortly after.

@jsirois jsirois merged commit f44e078 into pex-tool:main Jan 22, 2024
26 checks passed
@jsirois jsirois deleted the issues/2332 branch January 22, 2024 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants