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

[internal] Add [python-setup].experimental_resolves_to_lockfiles and hook up to ./pants generate-lockfiles #12703

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Aug 31, 2021

This option does not currently do anything..but it will be where users define their "named resolve":

[python-setup]
resolves_to_lockfiles = { default = "default_lock.txt", data-science = "ds_lock.txt" }

Then, users can set resolve="data-science" on targets.

They can generate the lockfile with ./pants generate-lockfiles --resolve='data-science'.

--

This PR is prework to add the option and wire it up to ./pants generate-lockfiles, including checking that the named resolve is not ambiguous with a tool's resolve name (its option scope).

[ci skip-rust]
[ci skip-build-wheels]

…d hook up to `./pants generate-lockfiles`

# 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]
"`{ default = '3rdparty/default_lockfile.txt', py2 = '3rdparty/py2.txt' }`.\n\n"
"To generate a lockfile, run `./pants generate-lockfiles --resolve=<name>` or "
"`./pants generate-lockfiles` to generate for all resolves (including tool "
"lockfiles).\n\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan to add more info once targets gain the resolve field. So far I have drafted:

"These resolve names allow certain Python targets to choose which lockfile they "
                "should use by setting the field `resolve='<name>'`. If you don't set the field, "
                "they will default to your resolve named `default`.\n\n"
                "Most projects will only need a single resolve for the whole project - this is a "
                "sensible default.\n\n"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm no longer certain we would to force you into setting a default resolve, and for that resolve to be named default. Per discussion in #12714, we may want a dedicated option to set the default resolve, if any.

This PR intentionally punts on committing to default being a special thing. Here, it's just like any other name.

@@ -127,6 +127,19 @@ def register_options(cls, register):
"Mutually exclusive with `[python-setup].requirement_constraints`."
),
)
register(
"--experimental-resolves-to-lockfiles",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I debated calling this only --experimental-resolves. But I think there's value in including the word "lockfile" in the name. "Resolve" on its own is vague to most users.

Copy link
Sponsor Member

@stuhood stuhood Aug 31, 2021

Choose a reason for hiding this comment

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

IMO, just resolves. The name alone will never be enough to skip reading the help, and the help should say that bit very clearly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you okay with waiting a little longer for this rename? #12714 is making me think we might want >1 resolve related option, e.g. default_resolve to be a dedicated option. Unclear.

I'm open to renaming but would like to keep this for now while iterating. It helps me my mental model.

Copy link
Contributor

@chrisjrn chrisjrn left a comment

Choose a reason for hiding this comment

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

This all seems fine; some small clarity things, but no red flags

src/python/pants/backend/python/goals/lockfile.py Outdated Show resolved Hide resolved
@@ -127,6 +127,19 @@ def register_options(cls, register):
"Mutually exclusive with `[python-setup].requirement_constraints`."
),
)
register(
"--experimental-resolves-to-lockfiles",
Copy link
Sponsor Member

@stuhood stuhood Aug 31, 2021

Choose a reason for hiding this comment

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

IMO, just resolves. The name alone will never be enough to skip reading the help, and the help should say that bit very clearly.

The followup PR shows that we only need the resolve name in this helper function, not the lockfile path. This simplifies the code

# 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 Eric-Arellano enabled auto-merge (squash) September 1, 2021 00:16
@Eric-Arellano Eric-Arellano merged commit 8c319ae into pantsbuild:main Sep 1, 2021
@Eric-Arellano Eric-Arellano deleted the generate-lockfiles-wire-up-resolves branch September 1, 2021 23:23
@stuhood stuhood mentioned this pull request Sep 3, 2021
Eric-Arellano added a commit that referenced this pull request Sep 7, 2021
Part of #11165 and builds off of #12703.

Rather than having a single option `[python-setup].experimental_lockfile`, users set `[python-setup].experimental_resolves_to_lockfiles` to define 0-n "named resolves" that associate a lockfile with a name:

```toml
[python-setup]
experimental_resolves_to_lockfiles = { lock1 = "lock1.txt", lock2 = "lock2.txt" }
```

Then, individual `pex_binary` targets can specify which resolve to use:

```python
pex_binary(name="reversion", entry_point="reversion.py", experimental_resolve="lock1")
```

In a followup, we'll add a mechanism to set the default resolve.

Users can generate that lockfile with `./pants generate-lockfiles` (all resolves) or `./pants generate-lockfiles --resolve=<name>`:

```
❯ ./pants generate-lockfiles --resolve=lock1 --resolve=lock2
15:55:56.60 [INFO] Completed: Generate lockfile for lock1
15:55:56.61 [INFO] Completed: Generate lockfile for lock2
15:55:57.02 [INFO] Wrote lockfile for the resolve `lock1` to lock1.txt
15:55:57.02 [INFO] Wrote lockfile for the resolve `lock2` to lock2.txt
```

Then, it will be consumed with `./pants package` and `./pants run`. Pants will extract the proper subset from that lockfile, meaning that the lockfile can safely be a superset of what is used for the particular build.

```
❯ ./pants package build-support/bin:
...
15:56:33.87 [INFO] Completed: Installing lock1.txt for the resolve `lock1`
15:56:34.39 [INFO] Completed: Installing lock2.txt for the resolve `lock2`
15:56:34.48 [INFO] Completed: Extracting 1 requirement to build build-support.bin/generate_user_list.pex from lock1_lockfile.pex: pystache==0.5.4
...
```

If the lockfile is incompatible, we will (soon) warn or error with instructions to either use a new resolve or regenerate the lockfile.

In followups, this field will be hooked up to other targets like `python_awslambda` and `python_tests`. 

We will likely also add a new field `compatible_resolves` to `python_library`, per #12714, which is a list of resolves. "Root targets" like `python_tests` and `pex_binary` will validate that all their dependencies are compatible. When you operate directly on a `python_library` target, like running MyPy on it, we will choose any of the possible resolves. You will be able to set your own default for this field.
 
[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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants