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 mode duplicates requirements lines #4885

Closed
A5rocks opened this issue Jul 8, 2024 · 9 comments · Fixed by #4902
Closed

Universal mode duplicates requirements lines #4885

A5rocks opened this issue Jul 8, 2024 · 9 comments · Fixed by #4902
Assignees
Labels
bug Something isn't working lock Related to universal resolution and locking

Comments

@A5rocks
Copy link

A5rocks commented Jul 8, 2024

I just noticed the --universal flag and it looks like just what we need with trio! However, when we try this out (though it's not shown in that PR), we run into an issue where requirement lines are duplicated.

Here's a reproducer, I can't explain this well. x.in:

pylint
sphinx

And here's what is spit out in my terminal when I run uv pip compile --universal --python-version=3.8 x.in:

Resolved 42 packages in 40ms
# This file was autogenerated by uv via the following command:
#    uv pip compile --universal --python-version=3.8 x.in
alabaster==0.7.16
    # via sphinx
alabaster==0.7.13
    # via sphinx
astroid==3.2.2
    # via pylint
babel==2.15.0
    # via sphinx
certifi==2024.7.4
    # via requests
charset-normalizer==3.3.2
    # via requests
colorama==0.4.6 ; sys_platform == 'win32'
    # via
    #   pylint
    #   sphinx
dill==0.3.8
    # via pylint
docutils==0.20.1
    # via sphinx
docutils==0.21.2
    # via sphinx
idna==3.7
    # via requests
imagesize==1.4.1
    # via sphinx
importlib-metadata==8.0.0 ; python_version < '3.10'
    # via sphinx
isort==5.13.2
    # via pylint
jinja2==3.1.4
    # via sphinx
markupsafe==2.1.5
    # via jinja2
mccabe==0.7.0
    # via pylint
packaging==24.1
    # via sphinx
platformdirs==4.2.2
    # via pylint
pygments==2.18.0
    # via sphinx
pylint==3.2.5
    # via -r x.in
pytz==2024.1 ; python_version < '3.9'
    # via babel
requests==2.32.3
    # via sphinx
snowballstemmer==2.2.0
    # via sphinx
sphinx==7.3.7
    # via -r x.in
sphinx==7.1.2
    # via -r x.in
sphinxcontrib-applehelp==1.0.8
    # via sphinx
sphinxcontrib-applehelp==1.0.4
    # via sphinx
sphinxcontrib-devhelp==1.0.6
    # via sphinx
sphinxcontrib-devhelp==1.0.2
    # via sphinx
sphinxcontrib-htmlhelp==2.0.5
    # via sphinx
sphinxcontrib-htmlhelp==2.0.1
    # via sphinx
sphinxcontrib-jsmath==1.0.1
    # via sphinx
sphinxcontrib-qthelp==1.0.7
    # via sphinx
sphinxcontrib-qthelp==1.0.3
    # via sphinx
sphinxcontrib-serializinghtml==1.1.10
    # via sphinx
sphinxcontrib-serializinghtml==1.1.5
    # via sphinx
tomli==2.0.1 ; python_version < '3.11'
    # via pylint
tomlkit==0.12.5
    # via pylint
typing-extensions==4.12.2 ; python_version < '3.11'
    # via
    #   astroid
    #   pylint
urllib3==2.2.2
    # via requests
zipp==3.19.2 ; python_version < '3.10'
    # via importlib-metadata

See how the sphinx lines are duplcated! Note that essentially everything above (x.in, command run) is minimized.

I'd love to poke at uv's internals a bit and maybe fix this! nevermind, I don't think I'm able to given some investigation, as I think this requires a larger change.

Platform information:

$ uv --version
uv 0.2.22
$ python --version
Python 3.12.2
$ python -m platform
Linux-6.1.0-21-amd64-x86_64-with-glibc2.36

I'm running Debian Linux but I also reproduced on Windows.

@zanieb zanieb added bug Something isn't working lock Related to universal resolution and locking labels Jul 8, 2024
@A5rocks
Copy link
Author

A5rocks commented Jul 8, 2024

Ok this doesn't happen in uv==0.2.18. The bad change is bisected to d9f389a ... which isn't too surprising TBH.

@zanieb
Copy link
Member

zanieb commented Jul 8, 2024

Thank you for bisecting, that's really helpful. cc @charliermarsh

@A5rocks
Copy link
Author

A5rocks commented Jul 8, 2024

Based on some investigation: when uv hits pylint, it forks into <3.11 and >=3.11 (or something like that). This means it then continues with >=3.8,<3.11 and >=3.8,>=3.11: the second simplifies to >=3.11. Then uv processes the sphinx dependency, with >=3.11 first it finds sphinx==7.3.7 but with >=3.8<3.11 it finds sphinx==7.1.2 (because sphinx>=7.2.0 dropped Python 3.8).

I think what uv should be doing here is adding markers that apply to everything within the forked state. But ATM it's not clear to me how to do that.

@charliermarsh
Copy link
Member

Yeah there should already be markers on the forked state that get propagated here. I'll take a look!

@charliermarsh charliermarsh self-assigned this Jul 8, 2024
@charliermarsh
Copy link
Member

Will prioritize this of course, hopefully fixed today but I have to travel in the afternoon.

@charliermarsh
Copy link
Member

I'll probably revert if we don't figure it out today -- better to be strict than wrong.

@A5rocks
Copy link
Author

A5rocks commented Jul 8, 2024

Yeah based on my investigation a very trivial fix is to comment out narrowing the forked state's Python version. (Which is essentially reverting any improvements)

BurntSushi added a commit that referenced this issue Jul 8, 2024
The PR #4707 introduced the notion of "version narrowing," where a
Requires-Python constraint was _possibly_ narrowed whenever the
universal resolver created a fork. The version narrowing would occur
when the fork was a result of a marker expression on `python_version`
that is *stricter* than the configured `Requires-Python` (via, say,
`pyproject.toml`).

The crucial conceptual change made by #4707 is therefore that
`Requires-Python` is no longer an invariant configuration of resolution,
but rather a mutable constraint that can vary from fork to fork. This in
turn can result in some cases, such as in #4885, where different
versions of dependencies are selected. We aren't sure whether we can fix
those or not, with version narrowing, so for now, we do this revert to
restore the previous behavior and we'll try to address the version
narrowing some other time.

This also adds the case from #4885 as a regression test, ensuring that
we don't break that in the future. I confirmed that with version
narrowing, this test outputs duplicate distributions. Without narrowing,
there are no duplicates.

Ref #4707, Fixes #4885
@BurntSushi BurntSushi self-assigned this Jul 8, 2024
BurntSushi added a commit that referenced this issue Jul 8, 2024
The PR #4707 introduced the notion of "version narrowing," where a
Requires-Python constraint was _possibly_ narrowed whenever the
universal resolver created a fork. The version narrowing would occur
when the fork was a result of a marker expression on `python_version`
that is *stricter* than the configured `Requires-Python` (via, say,
`pyproject.toml`).

The crucial conceptual change made by #4707 is therefore that
`Requires-Python` is no longer an invariant configuration of resolution,
but rather a mutable constraint that can vary from fork to fork. This in
turn can result in some cases, such as in #4885, where different
versions of dependencies are selected. We aren't sure whether we can fix
those or not, with version narrowing, so for now, we do this revert to
restore the previous behavior and we'll try to address the version
narrowing some other time.

This also adds the case from #4885 as a regression test, ensuring that
we don't break that in the future. I confirmed that with version
narrowing, this test outputs duplicate distributions. Without narrowing,
there are no duplicates.

Ref #4707, Fixes #4885
@notatallshaw-gts
Copy link

Is this supposed to be fixed for uv 0.2.24?

I still see it, but in a very complex example, so I wanted to check before trying to create a more minimal reproducer.

@charliermarsh
Copy link
Member

I believe so — feel free to file an issue. There are some known errors that we’re working on already, it may end up being a dupe but it’s totally fine.

CoolCat467 added a commit to CoolCat467/trio that referenced this issue Jul 12, 2024
CoolCat467 added a commit to python-trio/trio that referenced this issue Jul 20, 2024
* Use `--universal` flag with uv pip compile

* Upgrade to uv `0.2.26`, fixed astral-sh/uv#4885

* Update uv and remove `--no-strip-markers`
Apparently `--universal` implies `--no-strip-markers`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lock Related to universal resolution and locking
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants