Skip to content

Commit

Permalink
Remove same-graph merging in resolver
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Aug 14, 2024
1 parent 4a902a7 commit 8e835f6
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 60 deletions.
12 changes: 6 additions & 6 deletions crates/uv-resolver/src/resolution/graph.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use indexmap::IndexSet;
use petgraph::{
graph::{Graph, NodeIndex},
Directed, Direction,
Directed,
Direction, graph::{Graph, NodeIndex},
};
use pubgrub::Range;
use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet};
Expand All @@ -18,17 +18,17 @@ use uv_distribution::Metadata;
use uv_git::GitResolver;
use uv_normalize::{ExtraName, GroupName, PackageName};

use crate::{
InMemoryIndex, MetadataResponse, Options, PythonRequirement, RequiresPython, ResolveError,
ResolverMarkers, VersionsResponse,
};
use crate::pins::FilePins;
use crate::preferences::Preferences;
use crate::python_requirement::PythonTarget;
use crate::redirect::url_to_precise;
use crate::resolution::AnnotatedDist;
use crate::resolution_mode::ResolutionStrategy;
use crate::resolver::{Resolution, ResolutionDependencyEdge, ResolutionPackage};
use crate::{
InMemoryIndex, MetadataResponse, Options, PythonRequirement, RequiresPython, ResolveError,
ResolverMarkers, VersionsResponse,
};

pub(crate) type MarkersForDistribution = FxHashMap<(Version, Option<VerbatimUrl>), Vec<MarkerTree>>;

Expand Down
38 changes: 0 additions & 38 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -400,29 +400,6 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
);
}

// If another fork had the same resolution, merge into that fork instead.
if let Some(existing_resolution) = resolutions
.iter_mut()
.find(|existing_resolution| resolution.same_graph(existing_resolution))
{
let ResolverMarkers::Fork(existing_markers) = &existing_resolution.markers
else {
panic!("A non-forking resolution exists in forking mode")
};
let mut new_markers = existing_markers.clone();
new_markers.or(resolution
.markers
.fork_markers()
.expect("A non-forking resolution exists in forking mode")
.clone());
existing_resolution.markers = if new_markers.is_true() {
ResolverMarkers::universal(vec![])
} else {
ResolverMarkers::Fork(new_markers)
};
continue 'FORK;
}

Self::trace_resolution(&resolution);
resolutions.push(resolution);
continue 'FORK;
Expand Down Expand Up @@ -2516,21 +2493,6 @@ pub(crate) struct ResolutionDependencyEdge {
pub(crate) marker: MarkerTree,
}

impl Resolution {
/// Whether we got two identical resolutions in two separate forks.
///
/// Ignores pins since the which distribution we prioritized for each version doesn't matter.
fn same_graph(&self, other: &Self) -> bool {
// TODO(konsti): The edges being equal is not a requirement for the graph being equal. While
// an exact solution is too much here, we should ignore different in edges that point to
// nodes that are always installed. Example: root requires foo, root requires bar. bar
// forks, and one for the branches has bar -> foo while the other doesn't. The resolution
// is still the same graph since the presence or absence of the bar -> foo edge cannot
// change which packages and versions are installed.
self.nodes == other.nodes && self.edges == other.edges
}
}

impl ResolutionPackage {
pub(crate) fn is_base(&self) -> bool {
self.extra.is_none() && self.dev.is_none()
Expand Down
4 changes: 1 addition & 3 deletions crates/uv/tests/ecosystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ fn packse() -> Result<()> {
// Source: https://github.com/konstin/github-wikidata-bot/blob/8218d20985eb480cb8633026f9dabc9e5ec4b5e3/pyproject.toml
#[test]
fn github_wikidata_bot() -> Result<()> {
// TODO(charlie): This test became non-deterministic in https://github.com/astral-sh/uv/pull/6065.
// However, that fix is _correct_, and the non-determinism itself is an existing bug.
lock_ecosystem_package_non_deterministic("3.12", "github-wikidata-bot")
lock_ecosystem_package("3.12", "github-wikidata-bot")
}

// Source: https://github.com/psf/black/blob/9ff047a9575f105f659043f28573e1941e9cdfb3/pyproject.toml
Expand Down
24 changes: 11 additions & 13 deletions crates/uv/tests/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3626,19 +3626,17 @@ fn lock_python_version_marker_complement() -> Result<()> {
);
});

// TODO(charlie): This test became non-deterministic in https://github.com/astral-sh/uv/pull/6065.
// But that fix is correct, and the non-determinism itself is a bug.
// // Re-run with `--locked`.
// uv_snapshot!(context.filters(), context.lock().arg("--locked"), @r###"
// success: false
// exit_code: 2
// ----- stdout -----
//
// ----- stderr -----
// warning: `uv lock` is experimental and may change without warning
// Resolved 4 packages in [TIME]
// error: The lockfile at `uv.lock` needs to be updated, but `--locked` was provided. To update the lockfile, run `uv lock`.
// "###);
// Re-run with `--locked`.
uv_snapshot!(context.filters(), context.lock().arg("--locked"), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
warning: `uv lock` is experimental and may change without warning
Resolved 4 packages in [TIME]
error: The lockfile at `uv.lock` needs to be updated, but `--locked` was provided. To update the lockfile, run `uv lock`.
"###);

Ok(())
}
Expand Down

0 comments on commit 8e835f6

Please sign in to comment.