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

python_requirement uses resolve: str field, not compatible_resolves: list[str] #14420

Merged
merged 1 commit into from
Feb 10, 2022

Conversation

Eric-Arellano
Copy link
Contributor

Generally, we've realized that targets like python_source should use resolve: str: #14299. This solves several problems, particularly how to choose the resolve when operating directly on python_source targets e.g. with MyPy.

We wanted to keep compatible_resovles for python_requirement out of convenience, that it's nice for callers to be able to depend on //:reqs#numpy rather than //:reqs#numpy@resolve=a. The argument was that python_requirement is the "leaf" - it's not directly operated on.

That was a bad assumption. python_requirement is directly operated on with export and repl, e.g. ./pants repl 3rdparty/python::. We were defaulting in that case to [python].default_resolve -- but that doesn't make sense! If you're running ./pants repl pants-plugins/3rdparty::, why would Pants try to use python-default? We don't want to get into the business of trying to disambiguate which resolve you want, either, as that's very magical and confusing.

Beyond repl and export, @stuhood has wisely pointed out that this change is important for clarity. Even though //:reqs#numpy would have the same raw requirement string, the actual pinned version might change depending on the resolve! For example, numpy>=2.0,<3 might result in ==2.3.1 in resolve A, but ==2.7.4 in resolve B. They are not the same thing.

[ci skip-rust]

[ci skip-build-wheels]

…ves: list[str]`

[ci skip-rust]

[ci skip-build-wheels]
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Huzzah!

Comment on lines 127 to +133
"For now, Pants only has first-class support for disjoint resolves, meaning that "
"you cannot ergonomically set a `python_source` target, for example, to work "
"with multiple resolves. Practically, this means that you cannot yet reuse common "
"code, such as util files, across projects using different resolves. Support for "
"overlapping resolves is coming soon.\n\n"
f"If you only need a single resolve, run `{bin_name()} generate-lockfiles` to generate "
"the lockfile.\n\n"
"you cannot ergonomically set a `python_requirement` or `python_source` target, "
"for example, to work with multiple resolves. Practically, this means that you "
"cannot yet ergonomically reuse common code, such as util files, across projects "
"using different resolves. Support for overlapping resolves is coming soon.\n\n"
f"If you only need a single resolve, run `{bin_name()} generate-lockfiles` to "
"generate the lockfile.\n\n"
Copy link
Member

@stuhood stuhood Feb 9, 2022

Choose a reason for hiding this comment

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

This statement is still true, but for a different reason now, maybe? Basically, the same sources could have multiple resolves if you 1) declare two targets (maybe with a macro), 2) (in 2.11) use parametrize.

It might be worth calling out 1 as an option, and saying that 2 is coming...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is dep inference won't work with python_source, so I figured we shouldn't advertise it. But, if I can knock out #14293 in the next week we could cherry-pick it, then multiple targets is totally viable. Wdyt?

Copy link
Member

@stuhood stuhood Feb 9, 2022

Choose a reason for hiding this comment

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

I supposed that if it would still mean writing your own macros, it's probably ok to wait.

@Eric-Arellano Eric-Arellano merged commit e0a4275 into pantsbuild:main Feb 10, 2022
@Eric-Arellano Eric-Arellano deleted the no-compat-resolves branch February 10, 2022 00:07
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Feb 10, 2022
…ves: list[str]` (pantsbuild#14420)

Generally, we've realized that targets like `python_source` should use `resolve: str`: pantsbuild#14299. This solves several problems, particularly how to choose the resolve when operating directly on `python_source` targets e.g. with MyPy.

We wanted to keep `compatible_resovles` for `python_requirement` out of convenience, that it's nice for callers to be able to depend on `//:reqs#numpy` rather than `//:reqs#numpy@resolve=a`. The argument was that `python_requirement` is the "leaf" - it's not directly operated on.

That was a bad assumption. `python_requirement` is directly operated on with `export` and `repl`, e.g. `./pants repl 3rdparty/python::`. We were defaulting in that case to `[python].default_resolve` -- but that doesn't make sense! If you're running `./pants repl pants-plugins/3rdparty::`, why would Pants try to use `python-default`? We don't want to get into the business of trying to disambiguate which resolve you want, either, as that's very magical and confusing.

Beyond `repl` and `export`, @stuhood has wisely pointed out that this change is important for clarity. Even though `//:reqs#numpy` would have the same raw requirement string, the actual pinned version might change depending on the resolve! For example, `numpy>=2.0,<3` might result in `==2.3.1` in resolve A, but `==2.7.4` in resolve B. _They are not the same thing_.

[ci skip-rust]
[ci skip-build-wheels]
Eric-Arellano added a commit that referenced this pull request Feb 10, 2022
…ves: list[str]` (Cherry-pick of #14420) (#14435)

Generally, we've realized that targets like `python_source` should use `resolve: str`: #14299. This solves several problems, particularly how to choose the resolve when operating directly on `python_source` targets e.g. with MyPy.

We wanted to keep `compatible_resovles` for `python_requirement` out of convenience, that it's nice for callers to be able to depend on `//:reqs#numpy` rather than `//:reqs#numpy@resolve=a`. The argument was that `python_requirement` is the "leaf" - it's not directly operated on.

That was a bad assumption. `python_requirement` is directly operated on with `export` and `repl`, e.g. `./pants repl 3rdparty/python::`. We were defaulting in that case to `[python].default_resolve` -- but that doesn't make sense! If you're running `./pants repl pants-plugins/3rdparty::`, why would Pants try to use `python-default`? We don't want to get into the business of trying to disambiguate which resolve you want, either, as that's very magical and confusing.

Beyond `repl` and `export`, @stuhood has wisely pointed out that this change is important for clarity. Even though `//:reqs#numpy` would have the same raw requirement string, the actual pinned version might change depending on the resolve! For example, `numpy>=2.0,<3` might result in `==2.3.1` in resolve A, but `==2.7.4` in resolve B. _They are not the same thing_.

[ci skip-rust]
[ci skip-build-wheels]
stuhood added a commit that referenced this pull request Feb 11, 2022
As designed in #13882 (and elaborated on in #14420 in the context of Python), the `parametrize` builtin means that targets which want to support multiple resolves (or Scala versions) should use `resolve=parametrize(..)`, so that a different target is created per resolve.

This change replaces the `compatible_resolves` field with `resolve` for the JVM. And in order to allow the `resolve` field to be `parametrize`d, it also moves it from the `core_fields` of JVM target generators (which cannot be `parametrize`d) to the `moved_fields`.
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