Refactor incompatiblity tracking for distributions #1298

zanieb · 2024-02-14T17:37:51Z

Extends the "compatibility" types introduced in #1293 to apply to source distributions as well as wheels.

We now track the most-relevant incompatible source distribution
Exclude newer, Python requirements, and yanked versions are all tracked as incompatibilities in the new model (this lets us remove DistMetadata!)

crates/puffin/tests/pip_compile.rs

…1299) A smaller alternative to #1298 patching the bad behavior in #1293

Instead of dropping versions without a compatible distribution, we track them as incompatibilities in the solver. This implementation follows patterns established in #1290. This required some significant refactoring of how we track incompatible distributions. Notably: - `Option<TagPriority>` is now `WheelCompatibility` which allows us to track the reason a wheel is incompatible instead of just `None`. - `Candidate` now has a `CandidateDist` with `Compatible` and `Incompatibile` variants instead of just `ResolvableDist`; candidates are not strictly compatible anymore - `ResolvableDist` was renamed to `CompatibleDist` - `IncompatibleWheel` was given an ordering implementation so we can track the "most compatible" (but still incompatible) wheel. This allows us to collapse the reason a version cannot be used to a single incompatibility. - The filtering in the `VersionMap` is retained, we still only store one incompatible wheel per version. This is sufficient for error reporting. - A `TagCompatibility` type was added for tracking which part of a wheel tag is incompatible - `Candidate::validate_python` moved to `PythonRequirement::validate_dist` I am doing more refactoring in #1298 — I think a couple passes will be necessary to clarify the relationships of these types. Includes improved error message snapshots for multiple incompatible Python tag types from #1285 — we should add more scenarios for coverage of behavior when multiple tags with different levels are present.

crates/uv-resolver/src/resolver/mod.rs

zanieb · 2024-03-07T19:17:48Z

crates/uv-resolver/src/resolver/mod.rs

                                        }
+                                        IncompatibleWheel::RequiresPython(_) => unreachable!(),


Can RequiresPython be refactored to avoid this?

You could consider putting this whole block in here instead of using the continue and the unreachable!():

let python_version = requires_python .iter() .map(PubGrubSpecifier::try_from) .fold_ok(Range::full(), |range, specifier| { range.intersection(&specifier.into()) })?; let package = &next; for kind in [PubGrubPython::Installed, PubGrubPython::Target] { state.add_incompatibility(Incompatibility::from_dependency( package.clone(), Range::singleton(version.clone()), (PubGrubPackage::Python(kind), python_version.clone()), )); } state.partial_solution.add_decision(next.clone(), version); continue;

Then we need to write that whole thing twice which I find less ideal.

My concern is less about the unreachable! and more that it suggests a flaw in the abstraction.

Yeah, the abstraction is awkward, but I guess what I was pointing out (which maybe isn't useful) is that to the degree that the unreachable! itself represents a flaw in the abstraction, that's just a consequence of how the code is structured locally.

👍 yeah that makes sense. This would be partially alleviated by a restructuring to avoid duplication as described in #1298 (comment)

zanieb · 2024-03-07T19:20:01Z

crates/distribution-types/src/prioritized_distribution.rs

+    ExcludeNewer(Option<i64>),
+    RequiresPython(VersionSpecifiers),
+    Yanked(Yanked),


These three overlap with variants in IncompatibleWheel. Should we combine them into a structure like:

Incompatible ExcludeNewer RequiresPython Yanked Wheel NoBinary Tag SourceDist NoBuild

zanieb · 2024-03-07T19:29:52Z

I think this can be improved further, but now that it includes the user-facing improvement from #2066 I think we may want to bias towards merging.

Happy for any suggestions on different structuring.

# Conflicts: # crates/distribution-types/src/prioritized_distribution.rs # crates/puffin-resolver/src/candidate_selector.rs # crates/puffin-resolver/src/version_map.rs # Conflicts: # crates/distribution-types/src/prioritized_distribution.rs # crates/uv-client/src/flat_index.rs # crates/uv-resolver/src/candidate_selector.rs # crates/uv-resolver/src/finder.rs # crates/uv-resolver/src/python_requirement.rs # crates/uv-resolver/src/resolver/mod.rs # crates/uv-resolver/src/resolver/provider.rs

Supersedes #2066 # Conflicts: # crates/uv-resolver/src/candidate_selector.rs

charliermarsh

I think this is a big improvement.

crates/distribution-types/src/prioritized_distribution.rs

charliermarsh · 2024-03-08T01:23:19Z

crates/distribution-types/src/prioritized_distribution.rs

@@ -300,3 +285,64 @@ impl From<TagCompatibility> for WheelCompatibility {
        }
    }
 }
+
+impl IncompatibleSource {
+    fn is_more_compatible(&self, other: &IncompatibleSource) -> bool {


Why not Ord?

I think my branch just made everything Ord and then I had, like, compatibility > existing or something. But maybe it ends up being more complicated, the logic in here is quite nuanced. (This isn't blocking feedback.)

Satisfying Ord is actually really challenging. For example, when comparing RequiresPython incompatibilities that contain version specifiers we cannot really provide strong ordering. I attempted to do Ord at first as well and after much discussion @BurntSushi recommended this approach instead.

I believe the reason this worked on your branch is that you didn't retain information on the incompatibility enum members. Enum members themselves are trivially ordered, but once we store additional metadata about the incompatibility in them we need ordering in a larger hierarchy of types.

If you have other ideas to get around this please share :)

Separately.. the nuance here is a little frightening to me. Selecting the "most relevant incompatible distribution" feels like it could be brittle.

If there's no clear precedence, can with just stringifying the incompatibility criteria and sort based on that? It's arbitrary yet deterministic and easy to follow.

That's actually what I did to implement Ord originally, but it's hard to be sure we're meeting all of the requirements for the trait and it seems brittle.

I think what you have here is totally fine.

crates/uv-resolver/src/candidate_selector.rs

crates/uv-resolver/src/resolver/mod.rs

charliermarsh · 2024-03-08T01:30:30Z

crates/uv-resolver/src/resolver/mod.rs

                                        }
+                                        IncompatibleWheel::RequiresPython(_) => unreachable!(),


You could consider putting this whole block in here instead of using the continue and the unreachable!():

let python_version = requires_python .iter() .map(PubGrubSpecifier::try_from) .fold_ok(Range::full(), |range, specifier| { range.intersection(&specifier.into()) })?; let package = &next; for kind in [PubGrubPython::Installed, PubGrubPython::Target] { state.add_incompatibility(Incompatibility::from_dependency( package.clone(), Range::singleton(version.clone()), (PubGrubPackage::Python(kind), python_version.clone()), )); } state.partial_solution.add_decision(next.clone(), version); continue;

crates/uv-resolver/src/version_map.rs

konstin

This is a big improvement for the error messages

crates/distribution-types/src/prioritized_distribution.rs

konstin · 2024-03-08T08:53:37Z

crates/distribution-types/src/prioritized_distribution.rs

@@ -300,3 +285,64 @@ impl From<TagCompatibility> for WheelCompatibility {
        }
    }
 }
+
+impl IncompatibleSource {
+    fn is_more_compatible(&self, other: &IncompatibleSource) -> bool {


If there's no clear precedence, can with just stringifying the incompatibility criteria and sort based on that? It's arbitrary yet deterministic and easy to follow.

konstin · 2024-03-08T08:57:24Z

crates/uv-resolver/src/version_map.rs

+    /// Here, we track if each file is yanked separately. If a release is partially yanked, the
+    /// unyanked distributions _can_ be used.


I hope no index implements this, this would be quite confusing to users

Fun fact, the spec is about marking a particular file as yanked: https://peps.python.org/pep-0592/.

PyPI has implemented it as only being able to yank an entire release, but that is a PyPI implementation detail.

This has come up a few times on discussing particular behavior pip could adopt around yanking.

I see why it needs to be like that on the API level (there are no releases in the API, just files), but i'd strongly prefer it if indexes made this an all or nothing operation per release.

While this PEP implements yanking at the file level, that is largely due to the shape the simple repository API takes, not a specific decision made by this PEP.

Agreed, there has been some recent discussion about requiring behavior on indexes, not just the API, and the concern was being able to find and get buy in from non PyPI indexes (Azure, Gitlab, Artifactory, etc.). Perhaps a good case to put forth as a trial to see if it's possible.

zanieb force-pushed the zb/incompatible-dists branch 2 times, most recently from 0d6303b to 7251583 Compare February 14, 2024 17:43

zanieb commented Feb 14, 2024

View reviewed changes

crates/puffin/tests/pip_compile.rs Outdated Show resolved Hide resolved

This was referenced Feb 14, 2024

Patch handling of exclude_newer when reporting incompatible wheels #1299

Merged

Reset all of the snapshots for consistent indentation #1300

Merged

zanieb force-pushed the zb/no-wheel-platform branch from 90afb5e to b5da4ff Compare February 14, 2024 18:54

zanieb force-pushed the zb/incompatible-dists branch from 7251583 to 3fe8381 Compare February 14, 2024 18:56

zanieb added the internal A refactor or improvement that is not user-facing label Feb 14, 2024

zanieb added a commit that referenced this pull request Feb 14, 2024

Patch handling of exclude_newer when reporting incompatible wheels (#…

c79f646

…1299) A smaller alternative to #1298 patching the bad behavior in #1293

zanieb mentioned this pull request Feb 14, 2024

Report incompatible distributions to users #1293

Merged

zanieb force-pushed the zb/incompatible-dists branch 2 times, most recently from cfbe825 to dfa4a5e Compare February 15, 2024 16:17

Base automatically changed from zb/no-wheel-platform to main February 15, 2024 16:48

This comment was marked as outdated.

Sign in to view

zanieb mentioned this pull request Feb 27, 2024

Requirement with --only-binary apache-beam fails to resolve even though solution exists #2003

Closed

charliermarsh mentioned this pull request Feb 29, 2024

Track source incompatibilities in prioritized distribution #2066

Closed

zanieb force-pushed the zb/incompatible-dists branch 2 times, most recently from a7ea828 to cb36cf3 Compare March 7, 2024 18:50

zanieb commented Mar 7, 2024

View reviewed changes

crates/uv-resolver/src/resolver/mod.rs Outdated Show resolved Hide resolved

zanieb commented Mar 7, 2024

View reviewed changes

zanieb force-pushed the zb/incompatible-dists branch from 1701d64 to 5435865 Compare March 7, 2024 19:28

zanieb requested a review from charliermarsh March 7, 2024 19:28

zanieb added 2 commits March 7, 2024 13:30

Track --no-build as an incompatibility

aaa874e

Supersedes #2066 # Conflicts: # crates/uv-resolver/src/candidate_selector.rs

zanieb force-pushed the zb/incompatible-dists branch from 5435865 to aaa874e Compare March 7, 2024 19:30

zanieb marked this pull request as ready for review March 7, 2024 19:30

charliermarsh reviewed Mar 8, 2024

View reviewed changes

konstin approved these changes Mar 8, 2024

View reviewed changes

zanieb added 4 commits March 8, 2024 10:01

Store a subset of AllowedYanks

2ea5ed4

Update incomplete comment in exclude-newer

3e53dcb

Manually format weird clause in resolver/mod.rs

b5af1f1

Ensure ordering of sdist and wheel branches match

f9d31a2

zanieb merged commit 10c4eff into main Mar 8, 2024
7 checks passed

zanieb deleted the zb/incompatible-dists branch March 8, 2024 17:02

zanieb mentioned this pull request Jul 1, 2024

Move Requires-Python incompatibilities out of version map #4705

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor incompatiblity tracking for distributions #1298

Refactor incompatiblity tracking for distributions #1298

zanieb commented Feb 14, 2024 •

edited

Loading

This comment was marked as outdated.

zanieb Mar 7, 2024

charliermarsh Mar 8, 2024

zanieb Mar 8, 2024

charliermarsh Mar 8, 2024

zanieb Mar 8, 2024

zanieb Mar 7, 2024

zanieb commented Mar 7, 2024

charliermarsh left a comment

charliermarsh Mar 8, 2024

charliermarsh Mar 8, 2024

zanieb Mar 8, 2024

zanieb Mar 8, 2024

konstin Mar 8, 2024

zanieb Mar 8, 2024

charliermarsh Mar 8, 2024

charliermarsh Mar 8, 2024

konstin left a comment

konstin Mar 8, 2024

konstin Mar 8, 2024

notatallshaw Mar 8, 2024 •

edited

Loading

konstin Mar 8, 2024

notatallshaw Mar 8, 2024

		/// Here, we track if each file is yanked separately. If a release is partially yanked, the
		/// unyanked distributions _can_ be used.

Refactor incompatiblity tracking for distributions #1298

Refactor incompatiblity tracking for distributions #1298

Conversation

zanieb commented Feb 14, 2024 • edited Loading

This comment was marked as outdated.

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

zanieb commented Mar 7, 2024

charliermarsh left a 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

konstin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notatallshaw Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb commented Feb 14, 2024 •

edited

Loading

notatallshaw Mar 8, 2024 •

edited

Loading