Remove same-graph merging in resolver
#6077
Conversation
charliermarsh
added
bug
charliermarsh
force-pushed
the
charlie/same-graph
branch
2 times, most recently
from
ca1b896
to
591f126
Compare
crates/uv/tests/pip_compile.rs
Outdated
| # via | ||
| # -r requirements.in | ||
| # cryptography | ||
| cffi==1.17.0rc1 ; os_name != 'linux' or platform_python_implementation != 'PyPy' | ||
| cffi==1.17.0rc1 ; os_name != 'linux' |
There was a problem hiding this comment.
I think this was wrong before...? The requirements are:
cffi ; os_name == 'linux'
cffi >= 1.17.0rc1 ; os_name != 'linux'
cryptography
There was a problem hiding this comment.
Or is it just that these are redundant...? Since os_name == 'linux' or os_name != 'linux' already covers the full space?
There was a problem hiding this comment.
It is definitely redundant in terms of the marker expressions. But it's not immediately obvious to me why this change dropped that part of the marker.
There was a problem hiding this comment.
I would say we should merge this. I believe the original motivation was as an optimization. Because if we can prune forks then we probably should. But removing this optimization results in a bug fix from what I can see, and I think is potentially confounding our instability investigation.
It does seem like this class of optimization ought to be sound though. But I think we should chop it out for now and revisit it later. (i.e., Prioritize correctness.)
crates/uv/tests/pip_compile.rs
Outdated
| # via | ||
| # -r requirements.in | ||
| # example | ||
| torch==2.0.0+cu118 ; os_name == 'Linux' | ||
| torch==2.0.0+cu118 ; os_name == 'Linux' and platform_machine == 'x86_64' |
crates/uv/tests/pip_compile.rs
Outdated
| # via | ||
| # -r requirements.in | ||
| # cryptography | ||
| cffi==1.17.0rc1 ; os_name != 'linux' or platform_python_implementation != 'PyPy' | ||
| cffi==1.17.0rc1 ; os_name != 'linux' |
There was a problem hiding this comment.
It is definitely redundant in terms of the marker expressions. But it's not immediately obvious to me why this change dropped that part of the marker.
charliermarsh
force-pushed
the
charlie/ex
branch
2 times, most recently
from
b540a6a
to
acb3f31
Compare
charliermarsh
force-pushed
the
charlie/same-graph
branch
from
591f126
to
8e835f6
Compare
|
Ok this fixes two of the three instabilities that emerged after #6065. |
charliermarsh
force-pushed
the
charlie/same-graph
branch
from
8e835f6
to
44e880e
Compare
Summary
This was added in #5405 but is now the cause of an instability in
github_wikidata_bot. Specifically, on the initial run, we fork inpydantic==2.8.2, via:In the end, we resolve a single version of
typing-extensions(4.12.2)... But we don't recognize the two resolutions as the "same graph", because we propagate the fork markers, and so the "edges" have different markers on them...In the second run through, when we have the forks in advance, we don't split on Pydantic... We just try to solve from the root with the current forks. This is fundamentally different and I fear it will be the cause of many instabilities. But removing this graph check fixes the proximate issue.
I don't really understand why this was added since there was no test coverage in the PR.