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.

Sign up for GitHub

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

Jump to bottom

Allow conflicting extras in explicit index assignments #9160

Merged
merged 4 commits into from
Nov 19, 2024
Merged

Allow conflicting extras in explicit index assignments #9160

merged 4 commits into from
Nov 19, 2024

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Nov 16, 2024 •
edited
Loading

Summary

This PR enables something like the "final boss" of PyTorch setups -- explicit support for CPU vs. GPU-enabled variants via extras:

[project]
name = "project"
version = "0.1.0"
requires-python = ">=3.13.0"
dependencies = []

[project.optional-dependencies]
cpu = [
    "torch==2.5.1+cpu",
]
gpu = [
    "torch==2.5.1",
]

[tool.uv.sources]
torch = [
    { index = "torch-cpu", extra = "cpu" },
    { index = "torch-gpu", extra = "gpu" },
]

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

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

[tool.uv]
conflicts = [
    [
        { extra = "cpu" },
        { extra = "gpu" },
    ],
]

It builds atop the conflicting extras work to allow sources to be marked as specific to a dedicated extra being enabled or disabled.

As part of this work, sources now have an extra field. If a source has an extra, it means that the source is only applied to the requirement when defined within that optional group. For example, { index = "torch-cpu", extra = "cpu" } above only applies to "torch==2.5.1+cpu".

The extra field does not mean that the source is "enabled" when the extra is activated. For example, this wouldn't work:

[project]
name = "project"
version = "0.1.0"
requires-python = ">=3.13.0"
dependencies = ["torch"]

[tool.uv.sources]
torch = [
    { index = "torch-cpu", extra = "cpu" },
    { index = "torch-gpu", extra = "gpu" },
]

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

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

In this case, the sources would effectively be ignored. Extras are really confusing... but I think this is correct? We don't want enabling or disabling extras to affect resolution information that's outside of the relevant optional group.

@charliermarsh charliermarsh added the enhancement New feature or improvement to existing functionality label Nov 16, 2024
@charliermarsh charliermarsh force-pushed the charlie/ex branch 2 times, most recently from 7a9cb14 to 479be8f Compare November 16, 2024 02:50
@charliermarsh charliermarsh marked this pull request as ready for review November 16, 2024 02:50
@charliermarsh
Copy link
Member Author

This needs documentation, but I'm gonna wait for approval.

@charliermarsh charliermarsh force-pushed the charlie/ex branch 3 times, most recently from b15ad5b to 2a7e41b Compare November 16, 2024 03:49
@zanieb
Copy link
Member

zanieb commented Nov 16, 2024

I worry about the confusion of

The extra field does not mean that the source is "enabled" when the extra is activated.

But otherwise I'm into it! We could be more verbose and say from-extra to help clarify that case, but I don't know if I love that.

@charliermarsh charliermarsh force-pushed the charlie/ex branch 2 times, most recently from abacdc0 to 08ba726 Compare November 16, 2024 17:29
@charliermarsh
Copy link
Member Author

This now also supports dependency groups equally.

@charliermarsh
Copy link
Member Author

Umm. Maybe from = { extra = "cpu" } and from = { extra = "gpu" }?

@charliermarsh
Copy link
Member Author

I could also try to make it such that we error if we see the dependency requested as a production dependency, when it has a source with an extra on it?

@zanieb
Copy link
Member

zanieb commented Nov 17, 2024

from = { extra = "cpu" } and from = { extra = "gpu" }?

I can't think of a case from from = <package> makes sense which would be a mismatch with our other uses of from. Hm.

I could also try to make it such that we error if we see the dependency requested as a production dependency, when it has a source with an extra on it?

This seems reasonable, or at least a warning.

@charliermarsh
Copy link
Member Author

Ok, I'll start by adding some dedicated errors around this stuff. I think with good error messages (e.g., detect if torch is in project.dependencies but not in the relevant extra) it could be fine.

@charliermarsh charliermarsh force-pushed the charlie/ex branch 4 times, most recently from c3777a5 to d7a9215 Compare November 17, 2024 18:19
@charliermarsh
Copy link
Member Author

Ok, I added some dedicated error messages around this.

@charliermarsh charliermarsh force-pushed the charlie/ex branch 3 times, most recently from ebef8bb to c64d46b Compare November 18, 2024 03:46
konstin
konstin approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a cool application of conflicting extras!

crates/uv-resolver/src/resolver/indexes.rs
#[derive(Debug, Clone)]
struct Entry {
index: IndexUrl,
conflict: Option<ConflictItem>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the subset of forks in which this entry occurs, not a conflict we generated?

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

match extra_expression {
MarkerExpression::Extra { name, .. } => name.into_extra(),
_ => None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that reachable?

BurntSushi
BurntSushi approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I think this all makes sense to me.

crates/uv-distribution/src/metadata/lowering.rs Outdated
pub(crate) fn from_requirement<'data>(
requirement: uv_pep508::Requirement<VerbatimParsedUrl>,
project_name: &'data PackageName,
project_dir: &'data Path,
project_sources: &'data BTreeMap<PackageName, Sources>,
project_indexes: &'data [Index],
extra: Option<ExtraName>,
group: Option<GroupName>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be Option<&ExtraName> and Option<&GroupName>? It looks like the Clippy lint for this was suppressed explicitly, so I'm guessing there's a reason for it, but it doesn't immediately stand out to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should but I can't for the life of me figure out how to get it to work at the call site.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured it out by removing a lifetime.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh yeah I see now. Nice.

crates/uv-resolver/src/resolver/indexes.rs
entry
.conflict
.as_ref()
.map_or(true, |conflict| env.included_by_group(conflict.as_ref()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@zanieb
Copy link
Member

zanieb commented Nov 18, 2024

Just to clarify, this looks like it includes group = as well as extra = support?

@charliermarsh
Copy link
Member Author

It does yes. I mentioned that here but I guess not in the PR summary.

@charliermarsh charliermarsh enabled auto-merge (squash) November 19, 2024 00:59
@charliermarsh charliermarsh merged commit e4fc875 into main Nov 19, 2024
63 checks passed
@charliermarsh charliermarsh deleted the charlie/ex branch November 19, 2024 01:06
@FishAlchemist FishAlchemist mentioned this pull request Nov 19, 2024
Closed
kaxil added a commit to astronomer/airflow that referenced this pull request Nov 19, 2024
@kaxil
Bump uv to 0.5.3
6670e41 
Highlight for me is support for conflicting deps: astral-sh/uv#9160
@kaxil kaxil mentioned this pull request Nov 19, 2024
Merged
@BrewTestBot BrewTestBot mentioned this pull request Nov 19, 2024
Merged
kaxil added a commit to apache/airflow that referenced this pull request Nov 19, 2024
@kaxil
Bump uv to 0.5.3 (#44202)
6131511 
Highlight for me is support for conflicting deps: astral-sh/uv#9160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BurntSushi BurntSushi BurntSushi approved these changes

@konstin konstin konstin approved these changes

@zanieb zanieb Awaiting requested review from zanieb

Assignees
No one assigned
Labels
enhancement New feature or improvement to existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@charliermarsh @zanieb @BurntSushi @konstin