add support for resolving without a marker environment#3466
Conversation
7921a83 to
3387f98
Compare
| &self, | ||
| env: &MarkerEnvironment, | ||
| env: Option<&MarkerEnvironment>, | ||
| extras: &[ExtraName], |
There was a problem hiding this comment.
Is the key thing here that we do need to evaluate extras? Otherwise, I could imagine just doing something like env.map(...).unwrap_or(true) instead of encoding this in the API.
There was a problem hiding this comment.
Yes. We specifically want to evaluate extras.
There was a problem hiding this comment.
I believe the basic issue here is that our optional dependencies manifest as additions to a requirement's markers. So when we do resolution, we very specifically want to evaluate the "extra" marker expressions because we're only looking for universality with respect to platform ("marker environment"), and not with respect to the set of possible optional dependencies.
In my prototype, I dropped optional dependencies altogether. I couldn't move forward without that because it was too easy for, e.g., packages to bring in huge trees of dev dependencies.
charliermarsh
left a comment
There was a problem hiding this comment.
This looks right to me.
| // (which does have its Python version set potentially from the CLI, which | ||
| // I think is spiritually equivalent to setting the Python version in | ||
| // pyproject.toml). | ||
| let python_requirement = PythonRequirement::new(&interpreter, &markers.python_full_version); |
There was a problem hiding this comment.
Yeah this seems right. Or, at least, equivalent to our existing behavior.
There was a problem hiding this comment.
Is there any way this method could take &Interpreter and &MarkerEnvironment, so we abstract away the use of markers.python_full_version? (I know this is no different than on main, don't spend much time on it if difficult.)
There was a problem hiding this comment.
Yeah I agree the way I did things isn't quite optimal. Maybe the best thing here is to add an alternate constructor that takes a MarkerEnvironment. But I don't think we make it the only constructor because we fundamentally need a way of constructing a PythonRequirement without a MarkerEnvironment.
ab4cfd3 to
e758c8a
Compare
This doc test seems to fail due to the recent change making `Requirement` generic on its URL type. While the type parameter was given a default of `VerbatimUrl`, this default doesn't always apply. For example, the `FromStr` impl on `Requirement` is still generic on any URL type, and so callers must indicate the type of the URL to return. (An alternative would be to define the `FromStr` impl for just the default URL type.)
We provide a new API on a `Requirement` that specifically ignores the marker environment and only evaluates a requirement's marker expression with respect to extras. Any marker expressions that reference the marker environment automatically evaluate to true. Instead of duplicating the evaluation code, we just make a marker environment optional on the lower level APIs. In theory, we could just writer a separate evaluation routine that ignores everything except extras, but the evaluator has a fair bit of other stuff in it (such as emitting warnings) that would be good to keep DRY IMO.
This commit touches a lot of code, but the conceptual change here is
pretty simple: make it so we can run the resolver without providing a
`MarkerEnvironment`. This also indicates that the resolver should run in
universal mode. That is, the effect of a missing marker environment is
that all marker expressions that reference the marker environment are
evaluated to `true`. That is, they are ignored. (The only markers we
evaluate in that context are extras, which are the only markers that
aren't dependent on the environment.)
One interesting change here is that a `Resolver` no longer needs an
`Interpreter`. Previously, it had only been using it to construct a
`PythonRequirement`, by filling in the installed version from the
`Interpreter` state. But we now construct a `PythonRequirement`
explicitly since its `target` Python version should no longer be tied to
the `MarkerEnvironment`. (Currently, the marker environment is mutated
such that its `python_full_version` is derived from multiple sources,
including the CLI, which I found a touch confusing.)
The change in behavior can now be observed through the
`--unstable-uv-lock-file` flag. First, without it:
```
$ cat requirements.in
anyio>=4.3.0 ; sys_platform == "linux"
anyio<4 ; sys_platform == "darwin"
$ cargo run -qp uv -- pip compile -p3.10 requirements.in
anyio==4.3.0
exceptiongroup==1.2.1
# via anyio
idna==3.7
# via anyio
sniffio==1.3.1
# via anyio
typing-extensions==4.11.0
# via anyio
```
And now with it:
```
$ cargo run -qp uv -- pip compile -p3.10 requirements.in --unstable-uv-lock-file
x No solution found when resolving dependencies:
`-> Because you require anyio>=4.3.0 and anyio<4, we can conclude that the requirements are unsatisfiable.
```
This is expected at this point because the marker expressions are being
explicitly ignored, *and* there is no forking done yet to account for
the conflict.
e758c8a to
8ccf05e
Compare
This PR touches a lot of code, but the conceptual change here is
pretty simple: make it so we can run the resolver without providing a
MarkerEnvironment. This also indicates that the resolver should run inuniversal mode. That is, the effect of a missing marker environment is
that all marker expressions that reference the marker environment are
evaluated to
true. That is, they are ignored. (The only markers weevaluate in that context are extras, which are the only markers that
aren't dependent on the environment.)
One interesting change here is that a
Resolverno longer needs anInterpreter. Previously, it had only been using it to construct aPythonRequirement, by filling in the installed version from theInterpreterstate. But we now construct aPythonRequirementexplicitly since its
targetPython version should no longer be tied tothe
MarkerEnvironment. (Currently, the marker environment is mutatedsuch that its
python_full_versionis derived from multiple sources,including the CLI, which I found a touch confusing.)
The change in behavior can now be observed through the
--unstable-uv-lock-fileflag. First, without it:And now with it:
This is expected at this point because the marker expressions are being
explicitly ignored, and there is no forking done yet to account for
the conflict.
Closes #3352, Closes #3353