Skip to content

Commit

Permalink
Model Python version as a PubGrub package (#745)
Browse files Browse the repository at this point in the history
## Summary

This PR modifies the resolver to treat the Python version as a package,
which allows for better error messages (since we no longer treat
incompatible packages as if they "don't exist at all").

There are a few tricky pieces here...

First, we need to track both the interpreter's Python version and the
_target_ Python version, because we support resolving for other versions
via `--python 3.7`.

Second, we allow using incompatible wheels during resolution, as long as
there's a compatible source distribution. So we still need to test for
`requires-python` compatibility when selecting distributions.

This could use more testing, but it feels like an area where `packse`
would be more productive than writing PyPI tests.

Closes #406.
  • Loading branch information
charliermarsh authored Jan 3, 2024
1 parent 5a98add commit fd556cc
Show file tree
Hide file tree
Showing 20 changed files with 294 additions and 80 deletions.
12 changes: 10 additions & 2 deletions crates/puffin-cli/src/commands/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,16 @@ pub(crate) async fn pip_compile(
);

// Resolve the dependencies.
let resolver = Resolver::new(manifest, options, &markers, tags, &client, &build_dispatch)
.with_reporter(ResolverReporter::from(printer));
let resolver = Resolver::new(
manifest,
options,
&markers,
&interpreter,
tags,
&client,
&build_dispatch,
)
.with_reporter(ResolverReporter::from(printer));
let resolution = match resolver.resolve().await {
Err(puffin_resolver::ResolveError::NoSolution(err)) => {
#[allow(clippy::print_stderr)]
Expand Down
16 changes: 13 additions & 3 deletions crates/puffin-cli/src/commands/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use puffin_dispatch::BuildDispatch;
use puffin_installer::{
BuiltEditable, Downloader, InstallPlan, Reinstall, ResolvedEditable, SitePackages,
};
use puffin_interpreter::Virtualenv;
use puffin_interpreter::{Interpreter, Virtualenv};
use puffin_normalize::PackageName;
use puffin_resolver::{
Manifest, PreReleaseMode, ResolutionGraph, ResolutionMode, ResolutionOptions, Resolver,
Expand Down Expand Up @@ -173,6 +173,7 @@ pub(crate) async fn pip_install(
&editables,
&site_packages,
reinstall,
&interpreter,
tags,
markers,
&client,
Expand Down Expand Up @@ -320,6 +321,7 @@ async fn resolve(
editables: &[BuiltEditable],
site_packages: &SitePackages<'_>,
reinstall: &Reinstall,
interpreter: &Interpreter,
tags: &Tags,
markers: &MarkerEnvironment,
client: &RegistryClient,
Expand Down Expand Up @@ -361,8 +363,16 @@ async fn resolve(
);

// Resolve the dependencies.
let resolver = Resolver::new(manifest, options, markers, tags, client, build_dispatch)
.with_reporter(ResolverReporter::from(printer));
let resolver = Resolver::new(
manifest,
options,
markers,
interpreter,
tags,
client,
build_dispatch,
)
.with_reporter(ResolverReporter::from(printer));
let resolution = resolver.resolve().await?;

let s = if resolution.len() == 1 { "" } else { "s" };
Expand Down
9 changes: 5 additions & 4 deletions crates/puffin-cli/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,8 +669,9 @@ fn compile_python_37() -> Result<()> {
----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because there is no version of black available matching ==23.10.1 and
root depends on black==23.10.1, version solving failed.
╰─▶ Because there is no version of Python available matching >=3.8 and
black==23.10.1 depends on Python>=3.8, black==23.10.1 is forbidden.
And because root depends on black==23.10.1, version solving failed.
"###);
});

Expand Down Expand Up @@ -1552,8 +1553,8 @@ fn conflicting_transitive_url_dependency() -> Result<()> {
----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because there is no version of werkzeug available matching >=3.0.0 and
flask==3.0.0 depends on werkzeug>=3.0.0, flask==3.0.0 is forbidden.
╰─▶ Because flask==3.0.0 depends on werkzeug>=3.0.0 and there is no version
of werkzeug available matching >=3.0.0, flask==3.0.0 is forbidden.
And because root depends on flask==3.0.0, version solving failed.
"###);
});
Expand Down
8 changes: 4 additions & 4 deletions crates/puffin-cli/tests/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,11 @@ fn no_solution() -> Result<()> {
----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because flask==3.0.0 depends on werkzeug>=3.0.0 and there is no
version of flask available matching >3.0.0, flask>=3.0.0 depends on
╰─▶ Because there is no version of flask available matching >3.0.0 and
flask==3.0.0 depends on werkzeug>=3.0.0, flask>=3.0.0 depends on
werkzeug>=3.0.0.
And because root depends on werkzeug<1.0.0 and root depends on
flask>=3.0.0, version solving failed.
And because root depends on flask>=3.0.0 and root depends on
werkzeug<1.0.0, version solving failed.
"###);

Ok(())
Expand Down
1 change: 1 addition & 0 deletions crates/puffin-dev/src/resolve_cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ pub(crate) async fn resolve_cli(args: ResolveCliArgs) -> Result<()> {
Manifest::simple(args.requirements.clone()),
ResolutionOptions::default(),
venv.interpreter().markers(),
venv.interpreter(),
tags,
&client,
&build_dispatch,
Expand Down
2 changes: 1 addition & 1 deletion crates/puffin-dispatch/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,14 @@ impl<'a> BuildContext for BuildDispatch<'a> {
self.no_build
}

//#[instrument(skip(self, requirements), fields(requirements = requirements.iter().map(ToString::to_string).join(", ")))]
async fn resolve<'data>(&'data self, requirements: &'data [Requirement]) -> Result<Resolution> {
let markers = self.interpreter.markers();
let tags = self.interpreter.tags()?;
let resolver = Resolver::new(
Manifest::simple(requirements.to_vec()),
self.options,
markers,
self.interpreter,
tags,
self.client,
self,
Expand Down
26 changes: 26 additions & 0 deletions crates/puffin-resolver/src/candidate_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ use pubgrub::range::Range;
use rustc_hash::FxHashMap;

use distribution_types::{Dist, DistributionMetadata, IndexUrl, Name};
use pep440_rs::VersionSpecifiers;
use pep508_rs::{Requirement, VersionOrUrl};
use puffin_normalize::PackageName;
use pypi_types::BaseUrl;

use crate::file::DistFile;
use crate::prerelease_mode::PreReleaseStrategy;
use crate::pubgrub::PubGrubVersion;
use crate::python_requirement::PythonRequirement;
use crate::resolution_mode::ResolutionStrategy;
use crate::version_map::{ResolvableFile, VersionMap};
use crate::{Manifest, ResolutionOptions};
Expand Down Expand Up @@ -254,6 +256,30 @@ impl<'a> Candidate<'a> {
self.file.install()
}

/// If the candidate doesn't the given requirement, return the version specifiers.
pub(crate) fn validate(&self, requirement: &PythonRequirement) -> Option<&VersionSpecifiers> {
// Validate against the _installed_ file. It's fine if the _resolved_ file is incompatible,
// since it could be an incompatible wheel. (If the resolved file is an incompatible source
// distribution, then the resolved and installed file will be the same anyway.)
let requires_python = self.install().requires_python.as_ref()?;

// If the candidate doesn't support the target Python version, return the failing version
// specifiers.
if !requires_python.contains(requirement.target()) {
return Some(requires_python);
}

// If the candidate is a source distribution, and doesn't support the installed Python
// version, return the failing version specifiers, since we won't be able to build it.
if self.install().is_sdist() {
if !requires_python.contains(requirement.installed()) {
return Some(requires_python);
}
}

None
}

/// Return the [`Dist`] to use when resolving the candidate.
pub(crate) fn into_distribution(self, index: IndexUrl, base: BaseUrl) -> Dist {
Dist::from_registry(
Expand Down
35 changes: 27 additions & 8 deletions crates/puffin-resolver/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ use puffin_traits::OnceMap;
use pypi_types::BaseUrl;

use crate::candidate_selector::CandidateSelector;
use crate::pubgrub::{PubGrubHints, PubGrubPackage, PubGrubReportFormatter, PubGrubVersion};
use crate::pubgrub::{
PubGrubHints, PubGrubPackage, PubGrubPython, PubGrubReportFormatter, PubGrubVersion,
};
use crate::python_requirement::PythonRequirement;
use crate::version_map::VersionMap;

#[derive(Error, Debug)]
Expand Down Expand Up @@ -159,21 +162,37 @@ impl NoSolutionError {
#[must_use]
pub(crate) fn with_available_versions(
mut self,
python_requirement: &PythonRequirement,
package_versions: &OnceMap<PackageName, (IndexUrl, BaseUrl, VersionMap)>,
) -> Self {
let mut available_versions = FxHashMap::default();
for package in self.derivation_tree.packages() {
if let PubGrubPackage::Package(name, ..) = package {
if let Some(entry) = package_versions.get(name) {
let (_, _, version_map) = entry.value();
match package {
PubGrubPackage::Root(_) => {}
PubGrubPackage::Python(PubGrubPython::Installed) => {
available_versions.insert(
package.clone(),
version_map
.iter()
.map(|(version, _)| version.clone())
.collect(),
vec![PubGrubVersion::from(python_requirement.installed().clone())],
);
}
PubGrubPackage::Python(PubGrubPython::Target) => {
available_versions.insert(
package.clone(),
vec![PubGrubVersion::from(python_requirement.target().clone())],
);
}
PubGrubPackage::Package(name, ..) => {
if let Some(entry) = package_versions.get(name) {
let (_, _, version_map) = entry.value();
available_versions.insert(
package.clone(),
version_map
.iter()
.map(|(version, _)| version.clone())
.collect(),
);
}
}
}
}
self.available_versions = available_versions;
Expand Down
7 changes: 7 additions & 0 deletions crates/puffin-resolver/src/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ impl DistFile {
Self::Sdist(sdist) => sdist.filename.as_str(),
}
}

pub(crate) fn is_sdist(&self) -> bool {
match self {
Self::Wheel(_) => false,
Self::Sdist(_) => true,
}
}
}

impl From<DistFile> for File {
Expand Down
2 changes: 0 additions & 2 deletions crates/puffin-resolver/src/finder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ impl<'a> DistFinder<'a> {
// This is relevant for source dists which give no other indication of their
// compatibility and wheels which may be tagged `py3-none-any` but
// have `requires-python: ">=3.9"`
// TODO(konstin): https://github.com/astral-sh/puffin/issues/406
if !file
.requires_python
.as_ref()
Expand Down Expand Up @@ -170,7 +169,6 @@ impl<'a> DistFinder<'a> {
// This is relevant for source dists which give no other indication of their
// compatibility and wheels which may be tagged `py3-none-any` but
// have `requires-python: ">=3.9"`
// TODO(konstin): https://github.com/astral-sh/puffin/issues/406
if !file
.requires_python
.as_ref()
Expand Down
1 change: 1 addition & 0 deletions crates/puffin-resolver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ mod overrides;
mod pins;
mod prerelease_mode;
mod pubgrub;
mod python_requirement;
mod resolution;
mod resolution_mode;
mod resolution_options;
Expand Down
3 changes: 3 additions & 0 deletions crates/puffin-resolver/src/pubgrub/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,9 @@ fn merge_package(
// Either package is `root`.
(PubGrubPackage::Root(_), _) | (_, PubGrubPackage::Root(_)) => Ok(None),

// Either package is the Python installation.
(PubGrubPackage::Python(_), _) | (_, PubGrubPackage::Python(_)) => Ok(None),

// Left package has a URL. Propagate the URL.
(PubGrubPackage::Package(name, extra, Some(url)), PubGrubPackage::Package(.., None)) => {
Ok(Some(PubGrubPackage::Package(
Expand Down
3 changes: 2 additions & 1 deletion crates/puffin-resolver/src/pubgrub/mod.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
pub(crate) use crate::pubgrub::dependencies::PubGrubDependencies;
pub(crate) use crate::pubgrub::distribution::PubGrubDistribution;
pub(crate) use crate::pubgrub::package::PubGrubPackage;
pub(crate) use crate::pubgrub::package::{PubGrubPackage, PubGrubPython};
pub(crate) use crate::pubgrub::priority::{PubGrubPriorities, PubGrubPriority};
pub(crate) use crate::pubgrub::report::{PubGrubHints, PubGrubReportFormatter};
pub(crate) use crate::pubgrub::specifier::PubGrubSpecifier;
pub(crate) use crate::pubgrub::version::{PubGrubVersion, MIN_VERSION};

mod dependencies;
Expand Down
13 changes: 13 additions & 0 deletions crates/puffin-resolver/src/pubgrub/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ use puffin_normalize::{ExtraName, PackageName};
#[derive(Debug, Clone, Eq, Derivative)]
#[derivative(PartialEq, Hash)]
pub enum PubGrubPackage {
/// The root package, which is used to start the resolution process.
Root(Option<PackageName>),
/// A Python version.
Python(PubGrubPython),
/// A Python package.
Package(
PackageName,
Option<ExtraName>,
Expand Down Expand Up @@ -70,6 +74,14 @@ pub enum PubGrubPackage {
),
}

#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub enum PubGrubPython {
/// The Python version installed in the current environment.
Installed,
/// The Python version for which dependencies are being resolved.
Target,
}

impl std::fmt::Display for PubGrubPackage {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Expand All @@ -80,6 +92,7 @@ impl std::fmt::Display for PubGrubPackage {
write!(f, "root")
}
}
PubGrubPackage::Python(_) => write!(f, "Python"),
PubGrubPackage::Package(name, None, ..) => write!(f, "{name}"),
PubGrubPackage::Package(name, Some(extra), ..) => {
write!(f, "{name}[{extra}]")
Expand Down
1 change: 1 addition & 0 deletions crates/puffin-resolver/src/pubgrub/priority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ impl PubGrubPriorities {
pub(crate) fn get(&self, package: &PubGrubPackage) -> Option<PubGrubPriority> {
match package {
PubGrubPackage::Root(_) => Some(Reverse(0)),
PubGrubPackage::Python(_) => Some(Reverse(0)),
PubGrubPackage::Package(name, _, _) => self
.0
.get(name)
Expand Down
37 changes: 37 additions & 0 deletions crates/puffin-resolver/src/python_requirement.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
use pep440_rs::Version;
use pep508_rs::MarkerEnvironment;
use puffin_interpreter::Interpreter;

#[derive(Debug, Clone)]
pub struct PythonRequirement<'a> {
/// The installed version of Python.
installed: &'a Version,
/// The target version of Python; that is, the version of Python for which we are resolving
/// dependencies. This is typically the same as the installed version, but may be different
/// when specifying an alternate Python version for the resolution.
target: &'a Version,
}

impl<'a> PythonRequirement<'a> {
pub fn new(interpreter: &'a Interpreter, markers: &'a MarkerEnvironment) -> Self {
Self {
installed: interpreter.version(),
target: &markers.python_version.version,
}
}

/// Return the installed version of Python.
pub(crate) fn installed(&self) -> &'a Version {
self.installed
}

/// Return the target version of Python.
pub(crate) fn target(&self) -> &'a Version {
self.target
}

/// Returns an iterator over the versions of Python to consider when resolving dependencies.
pub(crate) fn versions(&self) -> impl Iterator<Item = &'a Version> {
std::iter::once(self.installed).chain(std::iter::once(self.target))
}
}
4 changes: 2 additions & 2 deletions crates/puffin-resolver/src/resolution_options.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use crate::{PreReleaseMode, ResolutionMode};
use chrono::{DateTime, Utc};

use crate::{PreReleaseMode, ResolutionMode};

/// Options for resolving a manifest.
#[derive(Debug, Default, Copy, Clone)]
pub struct ResolutionOptions {
// TODO(konstin): These should be pub(crate) again
pub resolution_mode: ResolutionMode,
pub prerelease_mode: PreReleaseMode,
pub exclude_newer: Option<DateTime<Utc>>,
Expand Down
Loading

0 comments on commit fd556cc

Please sign in to comment.