add conflict markers to the lock file #9370
Conversation
|
The relevant change to the lock file from #9289 (comment) is here: [[package]]
name = "anyio"
version = "4.6.2.post1"
source = { registry = "https://pypi.org/simple" }
dependencies = [
{ name = "idna", version = "3.9", source = { registry = "https://pypi.org/simple" }, conflict-marker = "extra == 'bar' or extra != 'foo'" },
{ name = "idna", version = "3.10", source = { registry = "https://pypi.org/simple" }, conflict-marker = "extra == 'foo'" },
{ name = "sniffio" },
]Notice the new |
crates/uv/tests/it/lock.rs
Outdated
| @@ -3867,7 +3867,7 @@ fn lock_conflicting_mixed() -> Result<()> { | |||
|
|
|||
| [package.dev-dependencies] | |||
| project1 = [ | |||
| { name = "sortedcontainers", version = "2.3.0", source = { registry = "https://pypi.org/simple" } }, | |||
| { name = "sortedcontainers", version = "2.3.0", source = { registry = "https://pypi.org/simple" }, conflict-marker = "extra != 'project2'" }, | |||
There was a problem hiding this comment.
So these are going to show up in the lockfile? In that case, I remain somewhat biased towards using group = rather than encoding the names in the string value.
There was a problem hiding this comment.
Encoding the names in the string value would be an internal detail not exposed to the lock file. It would be written like it is here, and when groups are added, it would be group != 'foobar'.
(My three choices given on Discord all have the same behavior with respect to the lock file and how the conflict markers are written. The only differences between them are how we implement it.)
There was a problem hiding this comment.
To follow up here: when I wrote the above comment, I was still thinking we'd have "normal PEP 508 marker" and "conflict marker" as separate things in the lock file. And in that scenario, it seemed more justifiable to have our own little custom syntax for the conflict marker that we mapped back to PEP 508 markers internally. But since then, I've come to understand that these markers need to be combined since the conflict markers themselves can be dependent on the PEP 508 markers.
I didn't want to make the markers no longer parseable by PEP 508... Maybe that's something we should do in the future, but I felt it was good not to break that property at least for now. But that does mean that we end up with some funny looking markers that I was originally trying to avoid. (The comments and commit messages give more detail.)
crates/uv/tests/it/lock.rs
Outdated
| @@ -19421,8 +19421,6 @@ fn lock_multiple_sources_index_disjoint_extras_with_marker() -> Result<()> { | |||
| version = "3.1.3" | |||
| source = { registry = "https://download.pytorch.org/whl/cu124" } | |||
| resolution-markers = [ | |||
| "sys_platform == 'darwin'", | |||
| "sys_platform != 'darwin'", | |||
There was a problem hiding this comment.
Is this a bug, perhaps? Why write it when it's empty?
There was a problem hiding this comment.
I think writing it when it's empty is probably an extant bug, perhaps exacerbated by conflicting extras. (Or maybe even only made possible by it.) I can look into it.
But as for deleting the markers here, I believe it's correct. I wrote up an explanation for it in the commit message.
crates/uv-resolver/src/graph_ops.rs
Outdated
| /// For example, given an edge like `foo[x1] -> bar`, then it is known that | ||
| /// `x1` is activated. This in turn can be used to simplify any downstream | ||
| /// conflict markers with `extra == "x1"` in them. | ||
| pub(crate) fn simplify_conflict_markers(graph: &mut Graph<ResolutionGraphNode, UniversalMarker>) { |
There was a problem hiding this comment.
I think I need to see an example of this.
|
OK, this PR should actually be ready for some very rough testing, but still only with extras, not groups. At the very least, this PR does currently fix #9289: If we can get some confidence in this technique with extras, then I'm pretty sure I know how to extend it to groups. I think the main thing I'm worried about right now is that the logic in this PR might be treating extras as a global resource. So if your top-level package I believe this can be resolved by enhancing conflict markers to include package names instead of just extra and group names. But this also doesn't feel right to me, for reasons that I'm finding difficult to articulate. Another hiccup here is |
|
OK, I think I found a case where treating extras as more "global" than they ought to be leads to incorrect behavior. Consider this [project]
name = "project"
version = "0.1.0"
requires-python = ">=3.11,<3.12"
dependencies = [
"proxy1",
"anyio>=4",
]
[tool.uv.workspace]
members = ["proxy1"]
[project.optional-dependencies]
x1 = ["idna==3.9"]
[tool.uv.sources]
proxy1 = { workspace = true }
[tool.uv]
conflicts = [
[
{package = "project", extra = "x1"},
{package = "proxy1", extra = "x2"},
{package = "proxy1", extra = "x3"},
],
]And this at [project]
name = "proxy1"
version = "0.1.0"
requires-python = ">=3.11,<3.12"
dependencies = []
[project.optional-dependencies]
x2 = ["idna==3.8"]
x3 = ["idna==3.7"]And now do a and notice that In this case, So I guess our conflict markers need to include not just the extra/group name, but the package activating it. I think that would solve this. |
|
Nice. |
BurntSushi
force-pushed
the
ag/i9289
branch
4 times, most recently
from
e3ef041 to
f0fa231
Compare
|
OK, I finally have #9289 fixed in this PR (including all examples in that issue and some other examples that I came up with). I do not believe there are any remaining known cases of ambiguity for extras (I haven't handled groups yet), so if anyone is willing to try and "break" this PR with just conflicting extras, that would be great. This PR is not in a mergeable state. So my next focus is to get it into one. One major change in this PR is that, while the markers being written to the lock file are still PEP 508 compatible, they may now include some very odd |
BurntSushi
force-pushed
the
ag/i9289
branch
2 times, most recently
from
1fa5245 to
94059e8
Compare
BurntSushi
force-pushed
the
ag/i9289
branch
3 times, most recently
from
e3da37a to
79ade56
Compare
| /// then its "universal" marker looks like this: | ||
| /// | ||
| /// ```text | ||
| /// sys_platform == 'Darwin' and extra == 'extra-3-foo-x1' |
There was a problem hiding this comment.
I thought I explained this somewhere, but it might have only been in a commit message. So I added a code comment here too:
/// Why `extra-3-foo-x1`?
///
/// * The `extra` prefix is there to distinguish it from `group`.
/// * The `3` is there to indicate the length of the package name,
/// in bytes. This isn't strictly necessary for encoding, but is required
/// if we were ever to need to decode a package and extra/group name from a
/// conflict marker.
/// * The `foo` package name ensures we namespace the extra/group name,
/// since multiple packages can have the same extra/group name.
Does that help?
| // character in `package` or `extra` values. But if we know the length of | ||
| // the package name, we can always parse each field unambiguously. | ||
| let package_len = package.as_str().len(); | ||
| ExtraName::new(format!("extra-{package_len}-{package}-{extra}")).unwrap() |
There was a problem hiding this comment.
Ohh ok. I see now. It's sort of unfortunate that this representation is "user-facing" in the lockfile.
There was a problem hiding this comment.
I don't know that I have a better answer. We could do something that's legal but not normalized...? Like use dashes for one piece?
There was a problem hiding this comment.
And yes, I 100% agree that this is unfortunate. I just don't know of a better way to go about it without inventing our own marker system.
There was a problem hiding this comment.
Are we bound to PEP 508, or could we do something like adding a sigil that only allowed here and in the lockfile but gets rejected everywhere else, or even as a specific new marker type? I see the problem with the MarkerTree being nested and such, but this looks like a potential collision and it's user facing
There was a problem hiding this comment.
but this looks like a potential collision and it's user facing
I find this very difficult to reason about. On the one hand, it is certainly true that someone could use extra-3-pkg-foo as an extra name, and thus potentially collide with a separately defined extra named foo for the package pkg. (I think such cases would be pathological given the baroqueness of extra-3-pkg-foo. You not only need an extra with that name but it also has to be declared as a conflicting extra.) On the other hand, even if a collision does occur, it's actually not totally clear to me that it would cause any problems. That's because we tend to strip the "normal" extra == 'foo' from markers at various points.
Are we bound to PEP 508, or could we do something like adding a sigil that only allowed here and in the lockfile but gets rejected everywhere else, or even as a specific new marker type?
A new marker type is possible (I think I suggested a new marker type or even something that isn't a marker somewhere), but I believe it's costly in terms of implementation effort. And even if we do have a new marker type, there is enough inter-operation going on that we'd need to be able to convert between the marker types. I think it's doable, but yeah, a fair bit of work.
Some extra sigil that isn't typically allowed in extra names is also possible... I think it's probably less work than adding a new marker type. And the implementation kinda-sorta already makes room for it. There's definitely some refactoring cost involved here because we wouldn't be able to use ExtraName or GroupName. So that means new APIs for evaluating markers with something looser than &[ExtraName]. And new parsing APIs to "allow" the sigil in certain cases but continue to reject it in others. Overall possible, but I'd say it's at least a couple days of work.
Since I think that, in order for a collision to occur, it has to be pathological, I'm not sure it's worth doing. One thing I can do as a mitigation is add a test that specifically provokes a collision and ensure behavior is reasonable.
There was a problem hiding this comment.
Are we bound to PEP 508
I think we are bound to PEP 508 insomuch as we want to be. While these conflict markers are technically valid PEP 508 markers, I think they are definitely a step toward PEP 508 incompatibility since they require an encoding scheme to use properly.
There was a problem hiding this comment.
I've added a test that instigates a collision and things seem to be okay:
uv/crates/uv/tests/it/lock_conflict.rs
Lines 4795 to 5015 in cc1d8e6
I think it's because we don't tend to have both extra == "foo" and extra == "pkg-3-foo-extra" in the same marker at the same time. (And I use the weasel words "tend to" because I find it very hard to reason about this.)
|
@konstin I'm going to hold off merging this until you're able to review it since this is making user visible changes to the lock file. I want to give folks a chance to give input before this gets merged, because once this goes out to end users, making changes at that point will become more annoying (or provoke churn). |
|
Confirmed to resolve #9701 as well. |
| )?; | ||
|
|
||
| let mut cmd = context.sync(); | ||
| // I guess --exclude-newer doesn't work with the torch indices? |
There was a problem hiding this comment.
The torch indices are missing the upload date
| .collect(); | ||
|
|
||
| let mut seen: FxHashSet<NodeIndex> = FxHashSet::default(); | ||
| while let Some(parent_index) = queue.pop() { |
There was a problem hiding this comment.
Aren't the propagated value set order dependent? Below, the green and the red items from A and C are propagated through, but B -> D happens before E -> D, so B -> D isn't propagated. Numbers indicate the order of edge traversals.
There was a problem hiding this comment.
Would it be easier if we started at the conflicting nodes and propagated knowledge about the "side" of the conflict through from them? E.g.
[tool.uv]
conflicts = [
[
{ extra = "extra1" },
{ extra = "extra2" },
],
]
we could start at project[extra1] and project[extra2] respectively, mark every node with (project, extra1) or (project, extra2) respectively and then simplify the outgoing edges of all nodes that only have either marker. This might make the world-knowledge easier to move in? (Caveat: That's from reading code only)
There was a problem hiding this comment.
This should be fixed in 8a5ce95
I opted to fix this by revisiting nodes when the set of extras gets updated.
| // character in `package` or `extra` values. But if we know the length of | ||
| // the package name, we can always parse each field unambiguously. | ||
| let package_len = package.as_str().len(); | ||
| ExtraName::new(format!("extra-{package_len}-{package}-{extra}")).unwrap() |
There was a problem hiding this comment.
Are we bound to PEP 508, or could we do something like adding a sigil that only allowed here and in the lockfile but gets rejected everywhere else, or even as a specific new marker type? I see the problem with the MarkerTree being nested and such, but this looks like a potential collision and it's user facing
BurntSushi
force-pushed
the
ag/i9289
branch
3 times, most recently
from
ba47e18 to
b5ef4c6
Compare
Some of these are regression tests or related regression tests for #9289. Others are regression tests for bugs I found while working on #9289. Most of the outputs for these tests are wrong. For example, some tests are asserting that multiple versions of the same package are installed. But we add them now before our fixes so that we can see how they change.
This commit adds a new concept to uv: conflict markers. Conflict markers resolve the ambiguity created in lock files, in some cases, when conflicting extras/groups are activated. Conflict markers allow us to make edges in the dependency graph conditional based only on which extras are and aren't enabled. Adding conflict markers themselves turned out to be somewhat easy. But what was hard was simplifying them. A good portion of the change in this commit is dedicated to simplification. We do two different kinds: 1. We imbibe "world knowledge" into the marker. That is, we take what we know about which extras/groups cannot be enabled simultaneously, then we simplify the markers based on this. 2. We walk the dependency graph and annotate which extras/groups are activated for each node for all possible paths into that node. When an extra (e.g., `foo`) is activated (or not activated) for all possible such paths, then we can replace `extra == 'foo'` (or `extra != 'foo'`) with `true` in any corresponding conflict marker expessions. Without these simplifications, it's very easy for conflict markers to appear almost everywhere in a lock file (when conflicting extras/groups are declared). Many of which are trivially true and aren't actually resolving any ambiguity and thus aren't needed.
These are all of the fixes to what's installed and the lock files that we expected.
These are mostly just copying the existing tests for extras, but replacing extras with groups. And then creating another set (where it makes sense to) for tests with a conflict between an extra and a group.
This fixes a bug found by Konsti[1] where not all extras would propagate correctly. This could lead to improper simplification steps. The fix here is to revisit nodes in the graph unless no changes have been made to the set of enabled extras. We also add a regression test whose snapshot changes without this fix. I tried writing a test case by hand but couldn't figure it out. The original pytorch MRE in #9289 also has a different lock file with this fix, but we really shouldn't be adding more pytorch tests given how beefy they are. So I found this case using airflow, which is also beefy, but hopefully good enough. [1]: #9370 (comment)
This fixes a bug found by Konsti[1] where not all extras would propagate correctly. This could lead to improper simplification steps. The fix here is to revisit nodes in the graph unless no changes have been made to the set of enabled extras. We also add a regression test whose snapshot changes without this fix. I tried writing a test case by hand but couldn't figure it out. The original pytorch MRE in #9289 also has a different lock file with this fix, but we really shouldn't be adding more pytorch tests given how beefy they are. So I found this case using airflow, which is also beefy, but hopefully good enough. [1]: #9370 (comment)
|
We probably want to replace |
|
Note: I squashed and merged this PR because the commits were more optimized for review and didn't individually each pass tests. |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.5.7` -> `0.5.8` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.5.8`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#058) [Compare Source](astral-sh/uv@0.5.7...0.5.8) **This release does not include the `powerpc64le-unknown-linux-musl` target due to a build issue. See [#​9793](astral-sh/uv#9793) for details. If this change affects you, please file an issue with your use-case.** ##### Enhancements - Omit empty resolution markers in lockfile ([#​9738](astral-sh/uv#9738)) - Add `--install-dir` to to `uv python install` and `uninstall` commands ([#​7920](astral-sh/uv#7920)) - Add `--show-urls` and `--only-downloads` to `uv python list` ([#​8062](astral-sh/uv#8062)) - Add `uv python list --all-arches` ([#​9782](astral-sh/uv#9782)) - Add `uv run --gui-script` flag for running Python scripts with `pythonw.exe` ([#​9152](astral-sh/uv#9152)) - Allow `--gui-script` on Unix ([#​9787](astral-sh/uv#9787)) - Allow download of Python distribution variants optimized for newer x86\_64 microarchitectures ([#​9781](astral-sh/uv#9781)) - Allow execution of `pyw` files on Unix ([#​9759](astral-sh/uv#9759)) - Allow users to specify URLs in `project.dependencies` and `tool.uv.sources` ([#​9718](astral-sh/uv#9718)) - Encode mutually-incompatible pairs of markers ([#​9444](astral-sh/uv#9444)) - Improve the error message when a Python install request is not valid ([#​9783](astral-sh/uv#9783)) - Preserve directory-level standalone build symlinks ([#​9723](astral-sh/uv#9723)) - Add support for `uv publish --index <name>` ([#​9694](astral-sh/uv#9694)) - Reframe `--locked` and `--frozen` as `--check` operations for `uv lock` ([#​9662](astral-sh/uv#9662)) - Rename Python install scratch directory from `.cache` -> `.temp` ([#​9756](astral-sh/uv#9756)) - Enable `uv tool uninstall uv` on Windows ([#​8963](astral-sh/uv#8963)) - Improve self-dependency hint to make shadowing clear ([#​9716](astral-sh/uv#9716)) - Refactor unavailable metadata to shrink the resolver ([#​9769](astral-sh/uv#9769)) - Show 'depends on itself' for proxy packages ([#​9717](astral-sh/uv#9717)) - Show a dedicated error for missing subdirectories ([#​9761](astral-sh/uv#9761)) - Show a dedicated hint for missing `git+` prefixes ([#​9789](astral-sh/uv#9789)) ##### Performance - Eagerly error when parsing `pyproject.toml` requirements ([#​9704](astral-sh/uv#9704)) - Use copy-on-write when normalizing paths ([#​9710](astral-sh/uv#9710)) ##### Bug fixes - Avoid enforcing non-conflicts in `uv export` ([#​9751](astral-sh/uv#9751)) - Don't drop comments between items in TOML tables ([#​9784](astral-sh/uv#9784)) - Don't fail with `--no-build` when static metadata is available ([#​9785](astral-sh/uv#9785)) - Don't filter non-patch registry version ([#​9736](astral-sh/uv#9736)) - Don't read metadata from stale `.egg-info` files ([#​9760](astral-sh/uv#9760)) - Enforce correctness of self-dependencies ([#​9705](astral-sh/uv#9705)) - Fix projects's typo in resolver error messages ([#​9708](astral-sh/uv#9708)) - Ignore `.` prefixed directories during managed Python installation discovery ([#​9786](astral-sh/uv#9786)) - Improve handling of invalid virtual environments during interpreter discovery ([#​8086](astral-sh/uv#8086)) - Normalize relative paths when `--project` is specified ([#​9709](astral-sh/uv#9709)) - Respect self-constraints on recursive extras ([#​9714](astral-sh/uv#9714)) - Respect user settings for tracing coloring ([#​9733](astral-sh/uv#9733)) - Retry on tar extraction errors ([#​9753](astral-sh/uv#9753)) - Add conflict markers to the lock file ([#​9370](astral-sh/uv#9370)) - De-duplicate resolution markers ([#​9780](astral-sh/uv#9780)) - Avoid 403 error hint for PyTorch URLs ([#​9750](astral-sh/uv#9750)) - Avoid treating non-existent `--find-links` as relative URLs ([#​9720](astral-sh/uv#9720)) - Omit Windows Store `python3.13.exe` et al ([#​9679](astral-sh/uv#9679)) - Replace executables with broken symlinks during `uv python install` ([#​9706](astral-sh/uv#9706)) ##### Documentation - Fix build failure links ([#​9740](astral-sh/uv#9740)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
uv v0.5.8 lands [this PR](astral-sh/uv#9370), which fixes issues where the `uv.lock` file generated when syncing could change between invocations because of the use of conflicting dependencies to manage CUDA versions (as recommended in uv's documentation)
This PR adds a notion of "conflict markers" to the lock file as an
attempt to address #9289. The idea is to encode a new kind of boolean
expression indicating how to choose dependencies based on which extras
are activated.
As an example of what conflict markers look like, consider one of the cases
brought up in #9289, where
anyiohad unconditional dependencies ontwo different versions of
idna. Now, those are gated by markers, like this:The odd extra values like
extra-7-project-fooare an encoding of not just the conflicting extra (foo) but also the package it's declared for (project). We need both bits of information because different packages may have the same extra name, even if they are completely unrelated. Theextra-part is a prefix to distinguish it from groups (which, in this case, would be encoded asgroup-7-project-fooiffoowere a dependency group). And the7part indicates the length of the package name which makes it possible to parse out the package and extra name from this encoding. (We don't actually utilize that property, but it seems like good sense to do it in case we do need to extra information from these markers.)While this preserves PEP 508 compatibility at a surface level, it does require utilizing this encoding scheme in order
to evaluate them when they're present (which only occurs when conflicting extras/groups are declared).
My sense is that the most complex part of this change is not just adding conflict markers, but their simplification. I tried to address this in the code comments and commit messages.
Reviewers should look at this commit-by-commit.
Fixes #9289, Fixes #9546, Fixes #9640, Fixes #9622, Fixes #9498, Fixes #9701, Fixes #9734