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

charliermarsh · 2024-09-27T18:29:43Z

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 · 2024-09-27T23:24:16Z

crates/uv-workspace/src/pyproject.rs

+            }
+        }
+    }
+}


@BurntSushi -- I'd love your eyes on this file in particular. Should I be enforcing these at deserialization time? I could easily enforce them when we actually use the lowered requirement. I'm not certain what's best.

charliermarsh · 2024-09-27T23:24:36Z

crates/uv-workspace/src/pyproject.rs

@@ -427,6 +513,7 @@ pub enum Source {
        rev: Option<String>,
        tag: Option<String>,
        branch: Option<String>,
+        marker: Option<MarkerTreeContents>,


@ibraheemdev -- Is it correct to use Option<MarkerTreeContents> for this? It's a field that users can provide in TOML.

charliermarsh · 2024-09-27T23:25:25Z

crates/uv-distribution/src/metadata/lowering.rs

-            return Err(LoweringError::UndeclaredWorkspacePackage);
+            return Either::Left(std::iter::once(Err(
+                LoweringError::UndeclaredWorkspacePackage,
+            )));


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>

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>?

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

charliermarsh · 2024-09-27T23:25:43Z

crates/uv-distribution/src/metadata/lowering.rs

+                marker,
+                source,
+                origin: requirement.origin.clone(),
+            }))


This file is mostly just the code being indented and put under an iterator. Very minimal changes.

charliermarsh · 2024-09-27T23:27:40Z

crates/pep508-rs/src/marker/tree.rs

+            .ok_or_else(|| de::Error::custom("expected at least one marker expression"))?;
+        Ok(marker)
+    }
+}


\cc @ibraheemdev -- Does this make sense for a user-provided marker field?

Hmm I'm not sure if this (and Option<MarkerTreeContents>) works. If the user wrote x or not x, we simplify to MarkerTree::TRUE, so this deserialize would fail? I don't think serde falls back to None in that case without serde(default).

What would you suggest? Just use String and parse these as needed?

charliermarsh · 2024-09-27T23:29:41Z

uv.schema.json

+      "items": {
+        "$ref": "#/definitions/Source"
+      }
+    },


I think this is wrong (it thinks it has to be a list).

It looks correct to and check-jsonschema does only complain about "tree" below:

[tool.uv.sources] tqdm = { git = "https:/github.com/tqdm/tqdm" } 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'" }, ] foo = "tree"

(I think I fixed it between the time I wrote this comment and your testing. Thanks so much for testing it.)

konstin · 2024-09-28T14:22:44Z

docs/concepts/dependencies.md

+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'" },
+]


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"

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.

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"

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

konstin · 2024-09-28T14:44:49Z

uv.schema.json

+      "items": {
+        "$ref": "#/definitions/Source"
+      }
+    },


It looks correct to and check-jsonschema does only complain about "tree" below:

[tool.uv.sources] tqdm = { git = "https:/github.com/tqdm/tqdm" } 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'" }, ] foo = "tree"

konstin · 2024-09-28T14:51:34Z

crates/uv/src/commands/project/run.rs

+                    .map(move |requirement| match requirement {
+                        Ok(requirement) => Ok(requirement.into_inner()),
+                        Err(err) => Err(err),
+                    })


nit:

Suggested change

})

.map_ok(move |requirement| requirement.into_inner())

konstin · 2024-09-28T14:55:37Z

crates/uv/tests/lock.rs

+      |                     ^
+    Source markers must be disjoint, but the following markers overlap: `python_full_version == '3.12.*' and sys_platform == 'win32'` and `sys_platform == 'win32'`.
+
+    hint: replace `sys_platform == 'win32'` with `python_full_version != '3.12.*' and sys_platform == 'win32'`.


konstin · 2024-09-28T14:58:27Z

crates/uv-distribution/src/metadata/lowering.rs

-            return Err(LoweringError::UndeclaredWorkspacePackage);
+            return Either::Left(std::iter::once(Err(
+                LoweringError::UndeclaredWorkspacePackage,
+            )));


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>?

docs/concepts/dependencies.md

charliermarsh · 2024-09-28T18:35:50Z

crates/uv/tests/lock.rs

+        [package.optional-dependencies]
+        cpu = [
+            { name = "iniconfig" },
+        ]


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

charliermarsh requested review from BurntSushi, zanieb and konstin September 27, 2024 23:23

charliermarsh added the enhancement New feature or request label Sep 27, 2024

charliermarsh marked this pull request as ready for review September 27, 2024 23:23

charliermarsh commented Sep 27, 2024

View reviewed changes

charliermarsh force-pushed the charlie/multi-sources branch from 8a8b588 to 312a2eb Compare September 27, 2024 23:26

charliermarsh commented Sep 27, 2024

View reviewed changes

charliermarsh mentioned this pull request Sep 27, 2024

Accept list of sources with markers in uv.tool.sources #3397

Open

charliermarsh commented Sep 27, 2024

View reviewed changes

charliermarsh force-pushed the charlie/multi-sources branch 2 times, most recently from 320be30 to 73703fc Compare September 28, 2024 01:41

konstin approved these changes Sep 28, 2024

View reviewed changes

relativityhd mentioned this pull request Sep 28, 2024

Support for selection of specific versions per user / sync #7763

Closed

charliermarsh force-pushed the charlie/multi-sources branch from 73703fc to 6026428 Compare September 28, 2024 18:33

charliermarsh commented Sep 28, 2024

View reviewed changes

charliermarsh force-pushed the charlie/multi-sources branch from 6026428 to 4b3f1e6 Compare September 28, 2024 19:03

charliermarsh added 3 commits September 28, 2024 19:06

Expand multiple sources in tool.uv.sources

2536c90

Respect markers on them

9a6d9a3

Grab remainder

5488719

charliermarsh force-pushed the charlie/multi-sources branch from 4b3f1e6 to 5488719 Compare September 28, 2024 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

charliermarsh commented Sep 27, 2024 •

edited

Loading

charliermarsh Sep 27, 2024

charliermarsh Sep 27, 2024

charliermarsh Sep 27, 2024

konstin Sep 28, 2024

charliermarsh Sep 28, 2024

charliermarsh Sep 27, 2024

charliermarsh Sep 27, 2024

ibraheemdev Sep 28, 2024 •

edited

Loading

charliermarsh Sep 29, 2024

charliermarsh Sep 27, 2024

konstin Sep 28, 2024

charliermarsh Sep 28, 2024

konstin Sep 28, 2024

charliermarsh Sep 28, 2024

charliermarsh Sep 28, 2024

charliermarsh Sep 28, 2024

konstin Sep 28, 2024

konstin Sep 28, 2024

konstin Sep 28, 2024

konstin Sep 28, 2024

charliermarsh Sep 28, 2024

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

Are you sure you want to change the base?

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

Conversation

charliermarsh commented Sep 27, 2024 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibraheemdev Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

charliermarsh commented Sep 27, 2024 •

edited

Loading

ibraheemdev Sep 28, 2024 •

edited

Loading