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

Universal resolver fails when multiple divergent constraints are provided #4640

Closed
charliermarsh opened this issue Jun 28, 2024 · 8 comments · Fixed by #5887
Closed

Universal resolver fails when multiple divergent constraints are provided #4640

charliermarsh opened this issue Jun 28, 2024 · 8 comments · Fixed by #5887
Assignees
Labels
bug Something isn't working preview Experimental behavior resolver Related to the package resolver

Comments

@charliermarsh
Copy link
Member

Given:

requests
requests==2.32.3 ; python_version >= '3.12'
requests==2.32.0 ; python_version < '3.12'

Running uv pip compile requirements.txt --universal fails with:

  × No solution found when resolving dependencies:
  ╰─▶ Because requests{python_version >= '3.12'}==2.32.3 depends on requests==2.32.3 and you require requests{python_version >= '3.12'}==2.32.3, we can conclude that your requirements and requests{python_version < '3.12'}==2.32.0 are
      incompatible.
      And because you require requests{python_version < '3.12'}==2.32.0, we can conclude that the requirements are unsatisfiable.

This seems a bit silly, but it's maybe not quite as silly if you view it as constraints. Imagine your requirements.txt is:

requests

And then your constraints.txt is:

requests==2.32.3 ; python_version >= '3.12'
requests==2.32.0 ; python_version < '3.12'

This would also fail.

@charliermarsh charliermarsh added bug Something isn't working preview Experimental behavior lock Related to universal resolution and locking resolver Related to the package resolver and removed lock Related to universal resolution and locking labels Jun 28, 2024
@charliermarsh
Copy link
Member Author

I added a test (that shows the failure) in #4648.

@davidfritzsche
Copy link
Contributor

It would be great if the universal resolver would support situations like this. For a software package at work we would like to support a very broad range of Python versions (Python 3.7 - Python 3.12) and a common problem is the numerical Python stack (numpy, pandas) the software depends on. Quite often there is no numpy version which would offer upstream wheels for all Python versions we want to support. Having support for a forked resolution of numpy in such cases would be great. Maybe the fact of having Python version specific requirements/constraints could be a marker when forking is wished for.

@charliermarsh
Copy link
Member Author

To be clear, the resolver does support divergent requirements, for example this resolves without issue:

requests==2.32.3 ; python_version >= '3.12'
requests==2.32.0 ; python_version < '3.12'

It's the blanket requests that's causing problems (for details related to the internal implementation).

@davidfritzsche
Copy link
Contributor

I have some difficulties with numpy (macOS, uv 0.2.18). With a requirements.in

numpy >=1.26; python_version>="3.9"
numpy <1.26; python_version<"3.9"

I cannot univeral-lock for Python >= 3.8:

$ uv pip compile -p 3.8 --universal requirements.in
  × No solution found when resolving dependencies:
  ╰─▶ Because only the following versions of numpy{python_version >= '3.9'} are available:
          numpy{python_version >= '3.9'}<=1.26.0
          numpy{python_version >= '3.9'}==1.26.1
          numpy{python_version >= '3.9'}==1.26.2
          numpy{python_version >= '3.9'}==1.26.3
          numpy{python_version >= '3.9'}==1.26.4
          numpy{python_version >= '3.9'}==2.0.0
      and the requested Python version (3.8) does not satisfy Python>=3.9, we can conclude that any of:
          numpy{python_version >= '3.9'}>=1.26.0,<1.26.2
          numpy{python_version >= '3.9'}>1.26.2,<1.26.3
          numpy{python_version >= '3.9'}>1.26.3,<1.26.4
          numpy{python_version >= '3.9'}>1.26.4,<2.0.0
          numpy{python_version >= '3.9'}>2.0.0
       are incompatible.
      And because the requested Python version (3.8) does not satisfy Python>=3.9 and you require
      numpy{python_version >= '3.9'}>=1.26, we can conclude that the requirements are unsatisfiable.

@charliermarsh
Copy link
Member Author

Thanks, that should work but likely a separate issue.

@charliermarsh
Copy link
Member Author

charliermarsh commented Jun 30, 2024

It works in the general case, e.g.:

anyio >= 3 ; python_version >= '3.9'
anyio < 3 ; python_version < '3.9'

My guess is that the issue in your case is that numpy >=1.26 doesn't support Python 3.8, and we're still trying to lock for your requires-python in that branch (i.e., we're still trying to enforce that every dependency supports Python 3.8 and later, instead of narrowing the requirement in that branch). I will create a separate issue.

@charliermarsh
Copy link
Member Author

Conceptually, could we model this as:

requests ; sys_platform != 'darwin' and sys_platform != 'linux'
requests==2.32.3 ; sys_platform == 'darwin'
requests==2.32.0 ; sys_platform == 'linux'

@BurntSushi?

@BurntSushi
Copy link
Member

The way forking works currently is that it looks for conflicting dependency specifications with non-overlapping markers. If conflicting dependency specifications are found but have overlapping markers, then a fork doesn't occur. So I think that if you start with something like:

requests
requests==2.32.3 ; python_version >= '3.12'
requests==2.32.0 ; python_version < '3.12'

Then because requests has no markers, it is considered overlapping with any other dependency specification on requests. So in this case, I believe no fork occurs at all. (I haven't actually confirmed that though.)

The suggestion of treating the requests specification as if its markers are non-overlapping by construction with each other specification is interesting. I'm not sure we can do that in the general case, but if we have some information like "requests was from requirements.in/pyproject.toml" and "the others are extra constraints added," then maybe it makes sense to massage the specification from requirements.in to be non-overlapping with the constraints given? Are there cases where we would do this and it would be undesirable?

Another option, perhaps, is to improve the error message and require the user to change their dependency specification to add the non-overlapping marker. But I'm not sure how well that works with specifying constraints.

@BurntSushi BurntSushi self-assigned this Jul 8, 2024
BurntSushi added a commit that referenced this issue Jul 26, 2024
Consider the following packse scenario:

```toml
[root]
requires = [
  "a>=1.0.0 ; python_version < '3.10'",
  "a>=1.1.0 ; python_version >= '3.10'",
  "a>=1.2.0 ; python_version >= '3.11'",
]

[packages.a.versions."1.0.0"]
[packages.a.versions."1.1.0"]
[packages.a.versions."1.2.0"]
```

On current `main`, this produces a dependency on `a` that looks like
this:

```toml
dependencies = [
    { name = "fork-overlapping-markers-basic-a", marker = "python_version < '3.10' or python_version >= '3.11'" },
]
```

But the marker expression is clearly wrong here, since it implies that
`a` isn't installed at all for Python 3.10. With this PR, the above
dependency becomes:

```toml
dependencies = [
    { name = "fork-overlapping-markers-basic-a" },
]
```

That is, it's unconditional. Which is I believe correct here since there
aren't any other constraints on which version to select.

The specific bug here is that when we found overlapping dependency
specifications for the same package *within* a pre-existing fork, we
intersected all of their marker expressions instead of unioning them.
That in turn resulted in incorrect marker expressions.

While this doesn't fix any known bug on the issue tracker (like #4640),
it does appear to fix a couple of our snapshot tests. And fixes a basic
test case I came up with while working on #4732.

For the packse scenario test: astral-sh/packse#206
BurntSushi added a commit that referenced this issue Jul 26, 2024
Consider the following packse scenario:

```toml
[root]
requires = [
  "a>=1.0.0 ; python_version < '3.10'",
  "a>=1.1.0 ; python_version >= '3.10'",
  "a>=1.2.0 ; python_version >= '3.11'",
]

[packages.a.versions."1.0.0"]
[packages.a.versions."1.1.0"]
[packages.a.versions."1.2.0"]
```

On current `main`, this produces a dependency on `a` that looks like
this:

```toml
dependencies = [
    { name = "fork-overlapping-markers-basic-a", marker = "python_version < '3.10' or python_version >= '3.11'" },
]
```

But the marker expression is clearly wrong here, since it implies that
`a` isn't installed at all for Python 3.10. With this PR, the above
dependency becomes:

```toml
dependencies = [
    { name = "fork-overlapping-markers-basic-a" },
]
```

That is, it's unconditional. Which is I believe correct here since there
aren't any other constraints on which version to select.

The specific bug here is that when we found overlapping dependency
specifications for the same package *within* a pre-existing fork, we
intersected all of their marker expressions instead of unioning them.
That in turn resulted in incorrect marker expressions.

While this doesn't fix any known bug on the issue tracker (like #4640),
it does appear to fix a couple of our snapshot tests. And fixes a basic
test case I came up with while working on #4732.

For the packse scenario test: astral-sh/packse#206
BurntSushi added a commit that referenced this issue Jul 30, 2024
This PR represents a different approach to marker propagation in an
attempt to unblock #4640. In particular, instead of propagating markers
when forks are created, we wait until resolution is complete to
propagate all markers to all dependencies in each fork. This ends up
being both more robust (we should never miss anything) and simpler to
implement because it doesn't require mutating a `PubGrubPackage` (which
was pretty annoying). I think the main downside here is that this can
sometimes add markers where they aren't needed.

This actually winds up making quite a few snapshot changes. I went
through each of them. Some of them look like legitimate bug fixes. Some
of them look like superfluous additions. And some of them look like they
would be removed if we had perfect marker normalization. But I don't
think any of the changes are _wrong_.
BurntSushi added a commit that referenced this issue Jul 30, 2024
This PR represents a different approach to marker propagation in an
attempt to unblock #4640. In particular, instead of propagating markers
when forks are created, we wait until resolution is complete to
propagate all markers to all dependencies in each fork. This ends up
being both more robust (we should never miss anything) and simpler to
implement because it doesn't require mutating a `PubGrubPackage` (which
was pretty annoying). I think the main downside here is that this can
sometimes add markers where they aren't needed.

This actually winds up making quite a few snapshot changes. I went
through each of them. Some of them look like legitimate bug fixes. Some
of them look like superfluous additions. And some of them look like they
would be removed if we had perfect marker normalization. But I don't
think any of the changes are _wrong_.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working preview Experimental behavior resolver Related to the package resolver
Projects
No open projects
Status: Done
3 participants