Allow multiple source entries for each package in tool.uv.sources

[charliermarsh]

Summary

This PR enables users to provide multiple source entries in tool.uv.sources, e.g.:

[tool.uv.sources]
httpx = [
  { git = "https://github.com/encode/httpx", tag = "0.27.2", marker = "sys_platform == 'darwin'" },
  { git = "https://github.com/encode/httpx", tag = "0.24.1", marker = "sys_platform == 'linux'" },
]

The implementation is relatively straightforward: when we lower the requirement, we now return an iterator rather than a single requirement. In other words, the above is transformed into two requirements:

httpx @ git+https://github.com/encode/[email protected] ; sys_platform == 'darwin'
httpx @ git+https://github.com/encode/[email protected] ; sys_platform == 'linux'

We verify (at deserialization time) that the markers are non-overlapping.

Closes #3397.

[charliermarsh]

I find this slightly awkward: we return an iterator, but if the "overall" requirement fails, we use once with an error. Perhaps the return type should more truthfully be: Result<impl Iterator<Item = Result<LoweredRequirement, LoweringError>>, LoweringError>

[konstin]

We lower only for workspace packages, there are few items and the iterator looks too complex for this use case, should we collect into a Result<Vec<LoweredRequirement>, LoweringError>?

[charliermarsh]

I suppose that's true -- this does only apply to workspace packages. I'd like @BurntSushi opinion on it too.

[BurntSushi]

I would have probably written this as a Result<Vec, LoweringError> personally. Or a SmallVec if you're concerned about the alloc (although it seems marginal here since I see other allocs below, although I don't know their relative frequency), since I think the vast majority of these would be a single element Vec right?

But I think the nested Result would be totally fine. Logically, it makes sense: you have an iterator of results, and building that iterator may fail.

In a "real" library, I might try to take the approach you have here in order to simplify the API. But it depends on some other factors that probably aren't relevant here.

[charliermarsh]

Ok, I left it as an iterator for now... I may look into rewriting it. I feel like an iterator is actually a simpler API, despite being more complicated "internally".

[charliermarsh]

(This method is run for all dependencies declared in the workspace. It returns a single entry the vast majority of the time.)

[konstin]

In these expressions, do we accept extras and support the following (assuming index api has landed)?

[project.optional-dependencies]
cu118 = []
cu121 = []
cu124 = []

[tool.uv.sources]
torch = [
    { index = "torch-cu118", marker = "extra == 'cu118'"},
    { index = "torch-cu124", marker = "extra == 'cu124'"},
    # cu121 is on pypi
]

[[tool.uv.index]]
name = "torch-cu118"
url = "https://download.pytorch.org/whl/cu118"

[[tool.uv.index]]
name = "torch-cu124"
url = "https://download.pytorch.org/whl/cu124"

[charliermarsh]

We definitely can't support the above since we don't support disjoint extras, and those markers are overlapping. I talk about this at length here: https://www.notion.so/astral-sh/PyTorch-10a48797e1ca803ea593c456e9bf23f8?pvs=4#10a48797e1ca80f4ab5af90ebcd77000.

[charliermarsh]

I need to see if we already propagate extras here though. I.e., this should probably work, right?

[project.optional-dependencies]
cu118 = []

[tool.uv.sources]
torch = [
    { index = "torch-cu118", marker = "extra == 'cu118'"},
]

[[tool.uv.index]]
name = "torch-cu118"
url = "https://download.pytorch.org/whl/cu118"

[charliermarsh]

I do think we need to support it once we support disjoint extras. It makes the mixed CPU/GPU stuff so much better.

[konstin]

We lower only for workspace packages, there are few items and the iterator looks too complex for this use case, should we collect into a Result<Vec<LoweredRequirement>, LoweringError>?

[charliermarsh]

This test is wrong (it doesn't include iniconfig outside of the optional dependencies, but it should). I think it's because the negation of extra == 'cpu' is extra != 'cpu', but when we resolve, we assume all extras are enabled, so that's false? It's like I want the negation of extra == 'cpu' to be, like, just omitting that marker. \cc @konstin

[BurntSushi]

I would have probably written this as a Result<Vec, LoweringError> personally. Or a SmallVec if you're concerned about the alloc (although it seems marginal here since I see other allocs below, although I don't know their relative frequency), since I think the vast majority of these would be a single element Vec right?

But I think the nested Result would be totally fine. Logically, it makes sense: you have an iterator of results, and building that iterator may fail.

In a "real" library, I might try to take the approach you have here in order to simplify the API. But it depends on some other factors that probably aren't relevant here.

[BurntSushi]

These errors are kind of a bummer (and they were before too). Like end users shouldn't be faced with Serde goop like "variant of untagged enum."

[charliermarsh]

I agree. We should fix this. I actually looked at adding https://github.com/dtolnay/serde-untagged but it looked non-trivial. How else could we do it? Could we add a variant that has no fields...? Then show a custom message?

[BurntSushi]

My approach would be to cargo expand the derive and copy that into a custom Deserialize impl. Then modify it as appropriate.

If the cargo expand output isn't reasonable, then yeah, I'd start just trying to hand-write a custom impl.

[konstin]

We can also turn it into a struct with all variants and do our own matching on top, like we do for the git reference variants already.