Use shared resolver state between add and lock (#8146)

## Summary

If you `uv add` a Git dependency, we resolve it twice:

![Screenshot 2024-10-12 at 2 17
27 PM](https://github.com/user-attachments/assets/342e2523-af06-4783-b836-93b6bd9f34bc)

While we need to avoid sharing state between `lock` and `sync` (see the
large TODO that moved in this change), we should prioritize sharing
state between different resolver operations.
This commit is contained in:
Charlie Marsh 2024-10-12 16:58:07 +02:00 committed by GitHub
parent 346050bf99
commit d12d569f24
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 65 additions and 30 deletions

View file

@ -105,6 +105,18 @@ impl<K: Eq + Hash, V: Clone, H: BuildHasher + Clone> OnceMap<K, V, H> {
Value::Waiting(_) => None, Value::Waiting(_) => None,
} }
} }
/// Remove the result of a previous job, if any.
pub fn remove<Q: ?Sized + Hash + Eq>(&self, key: &Q) -> Option<V>
where
K: Borrow<Q>,
{
let entry = self.items.remove(key)?;
match entry {
(_, Value::Filled(value)) => Some(value),
(_, Value::Waiting(_)) => None,
}
}
} }
impl<K: Eq + Hash + Clone, V, H: Default + BuildHasher + Clone> Default for OnceMap<K, V, H> { impl<K: Eq + Hash + Clone, V, H: Default + BuildHasher + Clone> Default for OnceMap<K, V, H> {

View file

@ -1,11 +1,13 @@
use std::collections::hash_map::Entry;
use std::fmt::Write;
use std::path::{Path, PathBuf};
use anyhow::{bail, Context, Result}; use anyhow::{bail, Context, Result};
use itertools::Itertools; use itertools::Itertools;
use owo_colors::OwoColorize; use owo_colors::OwoColorize;
use rustc_hash::{FxBuildHasher, FxHashMap}; use rustc_hash::{FxBuildHasher, FxHashMap};
use std::collections::hash_map::Entry;
use std::fmt::Write;
use std::path::{Path, PathBuf};
use tracing::debug; use tracing::debug;
use url::Url;
use uv_auth::{store_credentials_from_url, Credentials}; use uv_auth::{store_credentials_from_url, Credentials};
use uv_cache::Cache; use uv_cache::Cache;
@ -17,7 +19,7 @@ use uv_configuration::{
}; };
use uv_dispatch::BuildDispatch; use uv_dispatch::BuildDispatch;
use uv_distribution::DistributionDatabase; use uv_distribution::DistributionDatabase;
use uv_distribution_types::UnresolvedRequirement; use uv_distribution_types::{UnresolvedRequirement, VersionId};
use uv_fs::Simplified; use uv_fs::Simplified;
use uv_git::{GitReference, GIT_STORE}; use uv_git::{GitReference, GIT_STORE};
use uv_normalize::PackageName; use uv_normalize::PackageName;
@ -635,6 +637,7 @@ async fn lock_and_sync(
project.workspace(), project.workspace(),
venv.interpreter(), venv.interpreter(),
settings.into(), settings.into(),
&state,
Box::new(DefaultResolveLogger), Box::new(DefaultResolveLogger),
connectivity, connectivity,
concurrency, concurrency,
@ -726,6 +729,14 @@ async fn lock_and_sync(
.with_pyproject_toml(toml::from_str(&content).map_err(ProjectError::TomlParse)?) .with_pyproject_toml(toml::from_str(&content).map_err(ProjectError::TomlParse)?)
.ok_or(ProjectError::TomlUpdate)?; .ok_or(ProjectError::TomlUpdate)?;
// Invalidate the project metadata.
if let VirtualProject::Project(ref project) = project {
let url = Url::from_file_path(project.project_root())
.expect("project root is a valid URL");
let version_id = VersionId::from_url(&url);
debug_assert!(state.index.distributions().remove(&version_id).is_some());
}
// If the file was modified, we have to lock again, though the only expected change is // If the file was modified, we have to lock again, though the only expected change is
// the addition of the minimum version specifiers. // the addition of the minimum version specifiers.
lock = project::lock::do_safe_lock( lock = project::lock::do_safe_lock(
@ -734,6 +745,7 @@ async fn lock_and_sync(
project.workspace(), project.workspace(),
venv.interpreter(), venv.interpreter(),
settings.into(), settings.into(),
&state,
Box::new(SummaryResolveLogger), Box::new(SummaryResolveLogger),
connectivity, connectivity,
concurrency, concurrency,
@ -779,7 +791,6 @@ async fn lock_and_sync(
InstallOptions::default(), InstallOptions::default(),
Modifications::Sufficient, Modifications::Sufficient,
settings.into(), settings.into(),
&state,
Box::new(DefaultInstallLogger), Box::new(DefaultInstallLogger),
connectivity, connectivity,
concurrency, concurrency,

View file

@ -19,7 +19,7 @@ use uv_workspace::{DiscoveryOptions, MemberDiscovery, VirtualProject, Workspace}
use crate::commands::pip::loggers::DefaultResolveLogger; use crate::commands::pip::loggers::DefaultResolveLogger;
use crate::commands::project::lock::do_safe_lock; use crate::commands::project::lock::do_safe_lock;
use crate::commands::project::{ProjectError, ProjectInterpreter}; use crate::commands::project::{ProjectError, ProjectInterpreter};
use crate::commands::{diagnostics, pip, ExitStatus, OutputWriter}; use crate::commands::{diagnostics, pip, ExitStatus, OutputWriter, SharedState};
use crate::printer::Printer; use crate::printer::Printer;
use crate::settings::ResolverSettings; use crate::settings::ResolverSettings;
@ -88,6 +88,9 @@ pub(crate) async fn export(
.await? .await?
.into_interpreter(); .into_interpreter();
// Initialize any shared state.
let state = SharedState::default();
// Lock the project. // Lock the project.
let lock = match do_safe_lock( let lock = match do_safe_lock(
locked, locked,
@ -95,6 +98,7 @@ pub(crate) async fn export(
project.workspace(), project.workspace(),
&interpreter, &interpreter,
settings.as_ref(), settings.as_ref(),
&state,
Box::new(DefaultResolveLogger), Box::new(DefaultResolveLogger),
connectivity, connectivity,
concurrency, concurrency,

View file

@ -101,6 +101,9 @@ pub(crate) async fn lock(
.await? .await?
.into_interpreter(); .into_interpreter();
// Initialize any shared state.
let state = SharedState::default();
// Perform the lock operation. // Perform the lock operation.
match do_safe_lock( match do_safe_lock(
locked, locked,
@ -108,6 +111,7 @@ pub(crate) async fn lock(
&workspace, &workspace,
&interpreter, &interpreter,
settings.as_ref(), settings.as_ref(),
&state,
Box::new(DefaultResolveLogger), Box::new(DefaultResolveLogger),
connectivity, connectivity,
concurrency, concurrency,
@ -153,6 +157,7 @@ pub(super) async fn do_safe_lock(
workspace: &Workspace, workspace: &Workspace,
interpreter: &Interpreter, interpreter: &Interpreter,
settings: ResolverSettingsRef<'_>, settings: ResolverSettingsRef<'_>,
state: &SharedState,
logger: Box<dyn ResolveLogger>, logger: Box<dyn ResolveLogger>,
connectivity: Connectivity, connectivity: Connectivity,
concurrency: Concurrency, concurrency: Concurrency,
@ -160,17 +165,6 @@ pub(super) async fn do_safe_lock(
cache: &Cache, cache: &Cache,
printer: Printer, printer: Printer,
) -> Result<LockResult, ProjectError> { ) -> Result<LockResult, ProjectError> {
// Use isolate state for universal resolution. When resolving, we don't enforce that the
// prioritized distributions match the current platform. So if we lock here, then try to
// install from the same state, and we end up performing a resolution during the sync (i.e.,
// for the build dependencies of a source distribution), we may try to use incompatible
// distributions.
// TODO(charlie): In universal resolution, we should still track version compatibility! We
// just need to accept versions that are platform-incompatible. That would also make us more
// likely to (e.g.) download a wheel that we'll end up using when installing. This would
// make it safe to share the state.
let state = SharedState::default();
if frozen { if frozen {
// Read the existing lockfile, but don't attempt to lock the project. // Read the existing lockfile, but don't attempt to lock the project.
let existing = read(workspace) let existing = read(workspace)
@ -189,7 +183,7 @@ pub(super) async fn do_safe_lock(
interpreter, interpreter,
Some(existing), Some(existing),
settings, settings,
&state, state,
logger, logger,
connectivity, connectivity,
concurrency, concurrency,
@ -215,7 +209,7 @@ pub(super) async fn do_safe_lock(
interpreter, interpreter,
existing, existing,
settings, settings,
&state, state,
logger, logger,
connectivity, connectivity,
concurrency, concurrency,

View file

@ -166,6 +166,9 @@ pub(crate) async fn remove(
) )
.await?; .await?;
// Initialize any shared state.
let state = SharedState::default();
// Lock and sync the environment, if necessary. // Lock and sync the environment, if necessary.
let lock = project::lock::do_safe_lock( let lock = project::lock::do_safe_lock(
locked, locked,
@ -173,6 +176,7 @@ pub(crate) async fn remove(
project.workspace(), project.workspace(),
venv.interpreter(), venv.interpreter(),
settings.as_ref().into(), settings.as_ref().into(),
&state,
Box::new(DefaultResolveLogger), Box::new(DefaultResolveLogger),
connectivity, connectivity,
concurrency, concurrency,
@ -193,9 +197,6 @@ pub(crate) async fn remove(
let extras = ExtrasSpecification::All; let extras = ExtrasSpecification::All;
let install_options = InstallOptions::default(); let install_options = InstallOptions::default();
// Initialize any shared state.
let state = SharedState::default();
project::sync::do_sync( project::sync::do_sync(
InstallTarget::from(&project), InstallTarget::from(&project),
&venv, &venv,
@ -206,7 +207,6 @@ pub(crate) async fn remove(
install_options, install_options,
Modifications::Exact, Modifications::Exact,
settings.as_ref().into(), settings.as_ref().into(),
&state,
Box::new(DefaultInstallLogger), Box::new(DefaultInstallLogger),
connectivity, connectivity,
concurrency, concurrency,

View file

@ -529,6 +529,7 @@ pub(crate) async fn run(
project.workspace(), project.workspace(),
venv.interpreter(), venv.interpreter(),
settings.as_ref().into(), settings.as_ref().into(),
&state,
if show_resolution { if show_resolution {
Box::new(DefaultResolveLogger) Box::new(DefaultResolveLogger)
} else { } else {
@ -576,7 +577,6 @@ pub(crate) async fn run(
install_options, install_options,
Modifications::Sufficient, Modifications::Sufficient,
settings.as_ref().into(), settings.as_ref().into(),
&state,
if show_resolution { if show_resolution {
Box::new(DefaultInstallLogger) Box::new(DefaultInstallLogger)
} else { } else {

View file

@ -103,12 +103,16 @@ pub(crate) async fn sync(
) )
.await?; .await?;
// Initialize any shared state.
let state = SharedState::default();
let lock = match do_safe_lock( let lock = match do_safe_lock(
locked, locked,
frozen, frozen,
target.workspace(), target.workspace(),
venv.interpreter(), venv.interpreter(),
settings.as_ref().into(), settings.as_ref().into(),
&state,
Box::new(DefaultResolveLogger), Box::new(DefaultResolveLogger),
connectivity, connectivity,
concurrency, concurrency,
@ -140,9 +144,6 @@ pub(crate) async fn sync(
Err(err) => return Err(err.into()), Err(err) => return Err(err.into()),
}; };
// Initialize any shared state.
let state = SharedState::default();
// Perform the sync operation. // Perform the sync operation.
do_sync( do_sync(
target, target,
@ -154,7 +155,6 @@ pub(crate) async fn sync(
install_options, install_options,
modifications, modifications,
settings.as_ref().into(), settings.as_ref().into(),
&state,
Box::new(DefaultInstallLogger), Box::new(DefaultInstallLogger),
connectivity, connectivity,
concurrency, concurrency,
@ -179,7 +179,6 @@ pub(super) async fn do_sync(
install_options: InstallOptions, install_options: InstallOptions,
modifications: Modifications, modifications: Modifications,
settings: InstallerSettingsRef<'_>, settings: InstallerSettingsRef<'_>,
state: &SharedState,
logger: Box<dyn InstallLogger>, logger: Box<dyn InstallLogger>,
connectivity: Connectivity, connectivity: Connectivity,
concurrency: Concurrency, concurrency: Concurrency,
@ -187,6 +186,17 @@ pub(super) async fn do_sync(
cache: &Cache, cache: &Cache,
printer: Printer, printer: Printer,
) -> Result<(), ProjectError> { ) -> Result<(), ProjectError> {
// Use isolated state for universal resolution. When resolving, we don't enforce that the
// prioritized distributions match the current platform. So if we lock here, then try to
// install from the same state, and we end up performing a resolution during the sync (i.e.,
// for the build dependencies of a source distribution), we may try to use incompatible
// distributions.
// TODO(charlie): In universal resolution, we should still track version compatibility! We
// just need to accept versions that are platform-incompatible. That would also make us more
// likely to (e.g.) download a wheel that we'll end up using when installing. This would
// make it safe to share the state.
let state = SharedState::default();
// Extract the project settings. // Extract the project settings.
let InstallerSettingsRef { let InstallerSettingsRef {
index_locations, index_locations,

View file

@ -13,7 +13,7 @@ use uv_workspace::{DiscoveryOptions, Workspace};
use crate::commands::pip::loggers::DefaultResolveLogger; use crate::commands::pip::loggers::DefaultResolveLogger;
use crate::commands::pip::resolution_markers; use crate::commands::pip::resolution_markers;
use crate::commands::project::ProjectInterpreter; use crate::commands::project::ProjectInterpreter;
use crate::commands::{project, ExitStatus}; use crate::commands::{project, ExitStatus, SharedState};
use crate::printer::Printer; use crate::printer::Printer;
use crate::settings::ResolverSettings; use crate::settings::ResolverSettings;
@ -59,6 +59,9 @@ pub(crate) async fn tree(
.await? .await?
.into_interpreter(); .into_interpreter();
// Initialize any shared state.
let state = SharedState::default();
// Update the lockfile, if necessary. // Update the lockfile, if necessary.
let lock = project::lock::do_safe_lock( let lock = project::lock::do_safe_lock(
locked, locked,
@ -66,6 +69,7 @@ pub(crate) async fn tree(
&workspace, &workspace,
&interpreter, &interpreter,
settings.as_ref(), settings.as_ref(),
&state,
Box::new(DefaultResolveLogger), Box::new(DefaultResolveLogger),
connectivity, connectivity,
concurrency, concurrency,