diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d7055f..8c6ed47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,19 @@ page. ### Added +- **`version_range` hint on the `load_family` callback** (issue #92) — + `pyrer.solve(..., load_family=cb)` now invokes `cb` as + `cb(name, version_range="2+<3")` when the callback's signature can + accept a second `version_range` argument (named param or `**kwargs`). + The hint is a rez-syntax range string the shim can pass directly to + `rez.packages.iter_packages(range_=...)` to skip on-disk version + directories outside the request. Backward-compatible: 1-arg + callbacks (`def cb(name):`) keep working unchanged — pyrer detects + the signature via `inspect.signature` once per `solve()` call. + Targets the 95% load-fan-out waste documented in #92 (2,637 + packages loaded for 132 used on a typical Fortiche resolve); + projected 6-20× cut to `_load_family` wall time. The 188-case rez + differential still passes 188/188. - **`PackageData.from_strings(name, version, requires=None, variants=None)`** — classmethod constructor for raw-string callers, symmetric with `from_rez(pkg)`. Skips rez's `AttributeForwardMeta` chain, the diff --git a/Cargo.toml b/Cargo.toml index e1c771b..a39b689 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ members = ["crates/*"] resolver = "2" [workspace.package] -version = "0.1.0-rc.9" +version = "1.0.0-rc.1" authors = [ "Lorenzo Montant ", "Maxim Doucet ", @@ -23,8 +23,8 @@ lazy_static = "1.5.0" rand = "0.8.5" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" -rer-version = { path = "crates/rer-version", version = "0.1.0-rc.9" } -rer-resolver = { path = "crates/rer-resolver", version = "0.1.0-rc.9" } +rer-version = { path = "crates/rer-version", version = "1.0.0-rc.1" } +rer-resolver = { path = "crates/rer-resolver", version = "1.0.0-rc.1" } pyo3 = { version = "0.23.5", features = ["extension-module"] } # `mimalloc` is wired into the bench binary as a `#[global_allocator]`. # Callgrind shows ~33 % of cycles in libc malloc/free; mimalloc has measurably diff --git a/crates/rer-python/Cargo.toml b/crates/rer-python/Cargo.toml index e810c5b..de09874 100644 --- a/crates/rer-python/Cargo.toml +++ b/crates/rer-python/Cargo.toml @@ -26,3 +26,4 @@ crate-type = ["cdylib", "lib"] [dependencies] pyo3 = { workspace = true, features = ["abi3-py39"] } rer-resolver = { workspace = true } +rer-version = { workspace = true } diff --git a/crates/rer-python/src/lib.rs b/crates/rer-python/src/lib.rs index c6792be..8eb5d81 100644 --- a/crates/rer-python/src/lib.rs +++ b/crates/rer-python/src/lib.rs @@ -331,20 +331,48 @@ fn packages_to_map(packages: Vec) -> Result, load_err: Rc>>) -> FamilyLoader { +fn make_loader( + callback: Py, + load_err: Rc>>, + takes_range: bool, +) -> FamilyLoader { Box::new( - move |name: &str| -> Vec<(String, rer_resolver::PackageData)> { + move |name: &str, + hint: Option<&rer_version::VersionRange>| + -> Vec<(String, rer_resolver::PackageData)> { // Already errored on a previous call — short-circuit so we don't // pile up errors and don't keep calling a broken callback. if load_err.borrow().is_some() { return Vec::new(); } + let hint_str: Option = hint.map(|r| r.to_string()); let result: PyResult> = Python::with_gil(|py| -> PyResult<_> { - let ret = callback.bind(py).call1((name,))?; + let ret = if takes_range { + // Pass hint as a rez-style range string via the + // `version_range` keyword — works with both + // `def f(name, version_range=None)` and + // `def f(name, **kwargs)`. `None` → Python + // `None`, signalling "unconstrained". + let py_hint = match hint_str.as_deref() { + Some(s) => s.into_pyobject(py)?.into_any(), + None => py.None().into_bound(py), + }; + let kwargs = pyo3::types::PyDict::new(py); + kwargs.set_item("version_range", py_hint)?; + callback.bind(py).call((name,), Some(&kwargs))? + } else { + callback.bind(py).call1((name,))? + }; let pkgs: Vec = ret.extract()?; let mut out: Vec<(String, rer_resolver::PackageData)> = Vec::with_capacity(pkgs.len()); @@ -382,6 +410,82 @@ fn make_loader(callback: Py, load_err: Rc>>) -> Fa ) } +/// Inspect the Python callable's signature and return `true` if it can +/// accept a second `version_range` argument — either as a named parameter +/// or via `**kwargs` / `*args`. False means the legacy 1-arg shape. +/// +/// Errs on the side of `false` (legacy) if introspection fails for any +/// reason; the loader then keeps calling with just `name` and existing +/// shims keep working. +fn callback_takes_range(py: Python<'_>, callback: &Py) -> bool { + let inspect = match py.import("inspect") { + Ok(m) => m, + Err(_) => return false, + }; + let sig = match inspect.getattr("signature").and_then(|f| f.call1((callback,))) { + Ok(s) => s, + Err(_) => return false, + }; + let params = match sig.getattr("parameters") { + Ok(p) => p, + Err(_) => return false, + }; + let len: usize = params.len().unwrap_or(0); + if len == 0 { + return false; + } + // Walk the parameters' kinds. A second positional parameter, a + // `version_range` keyword parameter, or any `*args`/`**kwargs` + // signals support. + let Ok(items) = params.call_method0("items") else { + return false; + }; + let Ok(iter) = items.try_iter() else { + return false; + }; + let mut positional_count = 0usize; + let Ok(inspect_mod) = py.import("inspect") else { + return false; + }; + let Ok(parameter_cls) = inspect_mod.getattr("Parameter") else { + return false; + }; + let pkw = parameter_cls.getattr("VAR_KEYWORD").ok(); + let pvar = parameter_cls.getattr("VAR_POSITIONAL").ok(); + for item in iter { + let Ok(item) = item else { continue }; + let Ok(name_val) = item.get_item(0) else { + continue; + }; + let Ok(param) = item.get_item(1) else { continue }; + let Ok(name_s) = name_val.extract::() else { + continue; + }; + if name_s == "version_range" { + return true; + } + let Ok(kind) = param.getattr("kind") else { + continue; + }; + if let Some(p) = &pkw { + if kind.eq(p).unwrap_or(false) { + return true; + } + } + if let Some(p) = &pvar { + if kind.eq(p).unwrap_or(false) { + return true; + } + } + // Plain positional / positional-or-keyword: counts toward arity. + positional_count += 1; + if positional_count >= 2 { + return true; + } + } + false +} + // --------------------------------------------------------------------------- // solve // --------------------------------------------------------------------------- @@ -458,7 +562,17 @@ fn solve( let load_err: Rc>> = Rc::new(RefCell::new(None)); let repo = if let Some(callback) = load_family { - let lazy = PackageRepo::with_loader(make_loader(callback, Rc::clone(&load_err))); + // One-time signature inspection (issue #92): if the callback can + // take a `version_range` argument, we pass the current solver + // range as a rez-style string so the shim can pre-filter. + // Backward-compatible: callbacks with the 1-arg shape keep + // working unchanged. + let takes_range = Python::with_gil(|py| callback_takes_range(py, &callback)); + let lazy = PackageRepo::with_loader(make_loader( + callback, + Rc::clone(&load_err), + takes_range, + )); // Seed the eager set so the loader is never called for families // the caller already supplied. for (name, fam) in initial_map { diff --git a/crates/rer-resolver/src/rez_solver/context.rs b/crates/rer-resolver/src/rez_solver/context.rs index 378bb35..3658051 100644 --- a/crates/rer-resolver/src/rez_solver/context.rs +++ b/crates/rer-resolver/src/rez_solver/context.rs @@ -14,14 +14,81 @@ use std::rc::Rc; pub type FamilyMap = HashMap; /// Callback invoked on the first lookup for a family that is not already in -/// the repo. Returns `(version_string, PackageData)` pairs — every version of -/// the family. An empty result means "no such family"; the repo caches that -/// answer and never calls the loader for the same name again. +/// the repo, with an optional version-range hint indicating the *current* +/// solver constraint on that family. Returns `(version_string, PackageData)` +/// pairs. /// -/// Mirrors the lazy-load behaviour rez gets from its `Package` resource -/// wrapper (each `package.py` is AST-evaluated on first attribute access). -/// `pyrer` builds one of these from a Python callable for issue #86. -pub type FamilyLoader = Box Vec<(String, PackageData)>>; +/// **Hint semantics (issue #92):** +/// +/// - `None` — the solver needs *every* version of the family (e.g. an +/// unbounded request, or a backtrack-widen has invalidated a narrower +/// prior load). The shim must return all versions. +/// - `Some(range)` — the solver only needs versions inside `range`. The +/// shim **may** filter to versions intersecting it (rez's +/// `iter_packages(range_=...)` does exactly that). The shim is **allowed +/// to return a superset** — pyrer re-validates against current +/// constraints, so extra versions are merely wasted parse time. +/// - The shim must **not** silently drop versions outside the hint without +/// pyrer asking — pyrer caches the loaded range and re-calls the loader +/// with a widened range if the solver backtracks and needs more. +/// +/// An empty result means "no such family" *under the supplied hint*. The +/// repo memoises this answer paired with the hint that produced it; a +/// later wider hint will retry the loader. +/// +/// `pyrer` builds one of these from a Python callable. See the load_family +/// callback in `pyrer.solve()` for the Python-side contract. +pub type FamilyLoader = + Box) -> Vec<(String, PackageData)>>; + +/// Records which version-range was passed to the loader when a family was +/// last loaded. Used by [`PackageRepo`] to decide whether a cached family +/// map can serve a fresh request without re-calling the loader. +#[derive(Clone, Debug)] +enum LoadedRange { + /// Loaded with `None` hint — every version is in the cached map. + /// Always sufficient for any subsequent request. + Unconstrained, + /// Loaded with `Some(range)` — only versions inside this range are + /// guaranteed to be in the cached map. + Bounded(VersionRange), +} + +impl LoadedRange { + /// True if `hint` is fully covered by the cached load — i.e. every + /// version the caller could care about is already in our map. + fn covers(&self, hint: Option<&VersionRange>) -> bool { + match (self, hint) { + (LoadedRange::Unconstrained, _) => true, + (LoadedRange::Bounded(_), None) => false, + (LoadedRange::Bounded(loaded), Some(want)) => { + // want ⊆ loaded ⟺ loaded ∩ want == want + loaded.intersection(want).as_ref() == Some(want) + } + } + } + + /// The union of two load ranges, used when widening to satisfy a + /// hint that wasn't covered by the previous load. + fn widened_with(&self, hint: Option<&VersionRange>) -> LoadedRange { + match (self, hint) { + (LoadedRange::Unconstrained, _) | (_, None) => LoadedRange::Unconstrained, + (LoadedRange::Bounded(loaded), Some(want)) => { + LoadedRange::Bounded(loaded.union(want)) + } + } + } +} + +#[derive(Clone, Debug)] +struct FamilyEntry { + loaded_range: LoadedRange, + /// `Some(map)` if the family is known to exist; `None` if the loader + /// returned empty under `loaded_range` (i.e. "no such family within + /// this range, possibly absent entirely if `loaded_range` is + /// `Unconstrained`"). + map: Option>, +} /// The package repository — `family -> version -> PackageData`. /// @@ -31,19 +98,19 @@ pub type FamilyLoader = Box Vec<(String, PackageData)>>; /// own solver does. /// /// Lookups are routed through [`Self::get_family`], which: -/// 1. Returns the cached `Rc` if the family has been seen. -/// 2. Otherwise calls the loader (if any), memoising both the hit and the -/// "no such family" answer. -/// 3. Otherwise returns `None`. +/// 1. Returns the cached `Rc` if the family has been loaded with +/// a range that covers the request. +/// 2. Otherwise calls the loader (if any) with the widened range, replaces +/// the cache entry, and returns the new map. +/// 3. Returns `None` if there's no loader and the family isn't cached, or +/// if the loader returns empty under the widened range. /// /// Construction: /// - [`Self::from_map`] / `impl From>` — eager, no loader. /// - [`Self::with_loader`] — lazy; the loader is consulted on miss. #[derive(Default)] pub struct PackageRepo { - /// `Some(map)` for present families, `None` for families the loader - /// confirmed as absent (so we don't re-call it on miss). - families: RefCell>>>, + families: RefCell>, loader: Option, } @@ -65,10 +132,19 @@ impl PackageRepo { /// Eager repo from a `family -> version -> PackageData` map. The loader /// is `None`, so any family not in `map` is reported as absent on lookup. + /// Eager-seeded families count as fully loaded (any range hint covered). pub fn from_map(map: HashMap) -> Self { let families = map .into_iter() - .map(|(name, fam)| (name, Some(Rc::new(fam)))) + .map(|(name, fam)| { + ( + name, + FamilyEntry { + loaded_range: LoadedRange::Unconstrained, + map: Some(Rc::new(fam)), + }, + ) + }) .collect(); PackageRepo { families: RefCell::new(families), @@ -78,8 +154,10 @@ impl PackageRepo { /// Repo backed by a loader. The loader is called the first time the /// solver asks for a family that isn't already cached — both hits and - /// "no such family" answers are memoised, so the loader fires at most - /// once per family per repo. + /// "no such family" answers are memoised. With the issue #92 + /// version-range hint, the loader may be re-called for the same family + /// if a later request needs a wider range than the cached load + /// covered; otherwise the cache hit is one-and-done. /// /// Use [`Self::insert_family`] to pre-seed families that are already /// in memory (e.g. ones produced by the caller's BFS seed pass). @@ -90,42 +168,85 @@ impl PackageRepo { } } - /// Pre-populate a family. Useful with [`Self::with_loader`] to skip - /// the loader for families already in memory. + /// Pre-populate a family. Counts as a full load (any range hint + /// covered). Useful with [`Self::with_loader`] to skip the loader for + /// families already in memory. pub fn insert_family(&self, name: String, fam: FamilyMap) { - self.families.borrow_mut().insert(name, Some(Rc::new(fam))); + self.families.borrow_mut().insert( + name, + FamilyEntry { + loaded_range: LoadedRange::Unconstrained, + map: Some(Rc::new(fam)), + }, + ); } - /// Number of families currently cached in the repo. With a loader - /// attached this grows as the solve progresses — it only reflects the - /// eager-seeded set + whatever the loader has been asked for so far. + /// Number of present families currently cached in the repo. With a + /// loader attached this grows as the solve progresses — it only + /// reflects the eager-seeded set + whatever the loader has been + /// asked for so far. pub fn family_count(&self) -> usize { self.families .borrow() .values() - .filter(|v| v.is_some()) + .filter(|entry| entry.map.is_some()) .count() } /// `Some(family map)` if the family exists (cached or lazily loaded); /// `None` if there's no loader and it isn't cached, or if the loader /// returned no entries for it. - pub fn get_family(&self, name: &str) -> Option> { - if let Some(slot) = self.families.borrow().get(name) { - return slot.clone(); + /// + /// The `hint` is the range the solver currently needs — passed through + /// to the loader, which may use it to pre-filter (e.g. via rez's + /// `iter_packages(range_=...)`). The repo tracks which range each + /// family was loaded under and reloads (with a widened range) when a + /// later request can't be served from the cache. + pub fn get_family( + &self, + name: &str, + hint: Option<&VersionRange>, + ) -> Option> { + // Cache hit + range covered → return directly. + if let Some(entry) = self.families.borrow().get(name) { + if entry.loaded_range.covers(hint) { + return entry.map.clone(); + } } - let loaded = self.loader.as_ref().and_then(|load| { - let entries = load(name); + // Either uncached, or cached with a range that doesn't cover the + // request. Reload with the widened range. + let new_range = { + let families = self.families.borrow(); + match families.get(name) { + Some(entry) => entry.loaded_range.widened_with(hint), + None => match hint { + None => LoadedRange::Unconstrained, + Some(r) => LoadedRange::Bounded(r.clone()), + }, + } + }; + + let new_map = self.loader.as_ref().and_then(|load| { + let widened_hint = match &new_range { + LoadedRange::Unconstrained => None, + LoadedRange::Bounded(r) => Some(r), + }; + let entries = load(name, widened_hint); if entries.is_empty() { None } else { Some(Rc::new(entries.into_iter().collect::>())) } }); - self.families - .borrow_mut() - .insert(name.to_string(), loaded.clone()); - loaded + + self.families.borrow_mut().insert( + name.to_string(), + FamilyEntry { + loaded_range: new_range, + map: new_map.clone(), + }, + ); + new_map } } diff --git a/crates/rer-resolver/src/rez_solver/solver.rs b/crates/rer-resolver/src/rez_solver/solver.rs index 50deba4..74e7a8a 100644 --- a/crates/rer-resolver/src/rez_solver/solver.rs +++ b/crates/rer-resolver/src/rez_solver/solver.rs @@ -306,7 +306,7 @@ mod tests { let calls: Rc>> = Rc::new(RefCell::new(Vec::new())); let calls_inner = Rc::clone(&calls); - let repo = crate::rez_solver::PackageRepo::with_loader(Box::new(move |name: &str| { + let repo = crate::rez_solver::PackageRepo::with_loader(Box::new(move |name: &str, _hint: Option<&rer_version::VersionRange>| { calls_inner.borrow_mut().push(name.to_string()); match name { "app" => vec![("1.0".to_string(), pkg(&["lib-2"], &[]))], @@ -344,7 +344,7 @@ mod tests { // A diamond: app -> lib & util; util -> lib. lib is reached twice // but the loader must only be invoked once. - let repo = crate::rez_solver::PackageRepo::with_loader(Box::new(move |name: &str| { + let repo = crate::rez_solver::PackageRepo::with_loader(Box::new(move |name: &str, _hint: Option<&rer_version::VersionRange>| { calls_inner.borrow_mut().push(name.to_string()); match name { "app" => vec![("1.0".into(), pkg(&["lib", "util"], &[]))], @@ -367,11 +367,84 @@ mod tests { ); } + #[test] + fn test_loader_receives_version_range_hint() { + // Issue #92: the loader should be invoked with the solver's current + // range constraint as a hint. + use rer_version::VersionRange; + use std::cell::RefCell; + + let calls: Rc)>>> = + Rc::new(RefCell::new(Vec::new())); + let calls_inner = Rc::clone(&calls); + + let repo = crate::rez_solver::PackageRepo::with_loader(Box::new( + move |name: &str, hint: Option<&VersionRange>| { + calls_inner + .borrow_mut() + .push((name.to_string(), hint.cloned())); + match name { + "lib" => vec![ + ("1.0".to_string(), pkg(&[], &[])), + ("2.0".to_string(), pkg(&[], &[])), + ("3.0".to_string(), pkg(&[], &[])), + ], + _ => Vec::new(), + } + }, + )); + + let reqs = vec![Requirement::parse("lib-2+<3")]; + let mut solver = Solver::new(reqs, Rc::new(repo)).expect("solver construction"); + solver.solve(); + assert_eq!(solver.status(), SolverStatus::Solved); + + let calls = calls.borrow(); + assert_eq!(calls.len(), 1, "loader called more times than expected"); + let (name, hint) = &calls[0]; + assert_eq!(name, "lib"); + let hint = hint.as_ref().expect("loader should have seen a hint"); + assert_eq!(hint.to_string(), "2+<3"); + } + + #[test] + fn test_loader_eager_seed_no_loader_call() { + // A pre-seeded family must not trigger the loader, even when + // a range-hint request is made for it. The seed counts as a + // full load. + use std::cell::RefCell; + use std::collections::HashMap; + + let calls: Rc> = Rc::new(RefCell::new(0)); + let calls_inner = Rc::clone(&calls); + + let repo = crate::rez_solver::PackageRepo::with_loader(Box::new( + move |_name: &str, _hint: Option<&rer_version::VersionRange>| { + *calls_inner.borrow_mut() += 1; + Vec::new() + }, + )); + let mut fam: HashMap = HashMap::new(); + fam.insert("1.0".into(), pkg(&[], &[])); + fam.insert("2.0".into(), pkg(&[], &[])); + repo.insert_family("lib".into(), fam); + + let reqs = vec![Requirement::parse("lib-2")]; + let mut solver = Solver::new(reqs, Rc::new(repo)).expect("solver construction"); + solver.solve(); + assert_eq!(solver.status(), SolverStatus::Solved); + assert_eq!( + *calls.borrow(), + 0, + "loader must not be called for pre-seeded families" + ); + } + #[test] fn test_loader_empty_means_missing_family() { // The loader returns no entries for an unknown name; the solver // treats that as a missing family (failed resolve), not a panic. - let repo = crate::rez_solver::PackageRepo::with_loader(Box::new(|_| Vec::new())); + let repo = crate::rez_solver::PackageRepo::with_loader(Box::new(|_: &str, _: Option<&rer_version::VersionRange>| Vec::new())); let reqs = vec![Requirement::parse("doesnotexist")]; let solver = Solver::new(reqs, Rc::new(repo)); // Either Solver::new returns a ScopeError or the solve fails; diff --git a/crates/rer-resolver/src/rez_solver/variant.rs b/crates/rer-resolver/src/rez_solver/variant.rs index 27110e0..b65f39d 100644 --- a/crates/rer-resolver/src/rez_solver/variant.rs +++ b/crates/rer-resolver/src/rez_solver/variant.rs @@ -372,8 +372,16 @@ impl PackageVariantList { /// Build the (lazy) variant list for a family, or `None` if the family is /// absent from the repository. Only version strings are parsed here — the /// requirement strings are parsed on demand by [`Self::get_intersection`]. - pub fn new(ctx: &SolverContext, package_name: &str) -> Option { - let versions = ctx.repo.get_family(package_name)?; + /// + /// `hint` is the current solver range constraint, forwarded to the repo's + /// loader so the shim can pre-filter versions (issue #92). `None` means + /// "unconstrained — every version". + pub fn new( + ctx: &SolverContext, + package_name: &str, + hint: Option<&VersionRange>, + ) -> Option { + let versions = ctx.repo.get_family(package_name, hint)?; let mut entries: Vec = versions .keys() .map(|version_str| { @@ -877,14 +885,21 @@ impl PackageVariantCache { /// Get a slice of `package_name`'s variants intersected with `range`. /// /// `None` means either the family is absent from the repository or no - /// version falls within `range` — Phase 4/5 distinguishes the two. + /// version falls within `range` — Phase 4/5 distinguishes the two via + /// [`Self::family_missing`]. + /// + /// `range` is also forwarded to the repo's loader as the hint, so a + /// `load_family` callback can pre-filter to versions intersecting it + /// (issue #92). If a later request asks for a wider range than the + /// cached load covered, both the repo and this cache transparently + /// reload. pub fn get_variant_slice( &mut self, ctx: &Rc, package_name: &str, range: &VersionRange, ) -> Option { - let list = self.get_or_build(ctx, package_name)?; + let list = self.get_or_build(ctx, package_name, Some(range))?; let entries = list.get_intersection(range)?; Some(PackageVariantSlice::new( Rc::clone(ctx), @@ -894,22 +909,69 @@ impl PackageVariantCache { } /// True if the family is known to be absent from the repository. + /// Asks with `None` hint to force a full load — definitive absence + /// can't be answered from a range-bounded cached result. pub fn family_missing(&mut self, ctx: &Rc, package_name: &str) -> bool { - self.get_or_build(ctx, package_name).is_none() + self.get_or_build(ctx, package_name, None).is_none() } /// Look up the cached `PackageVariantList` for `package_name`, building it /// on first access. Lookup is by `&str` (`Borrow` on `Rc`), so a /// cache hit avoids allocating a fresh `Name` key. + /// + /// The cached list is invalidated and rebuilt if the underlying family + /// map in [`PackageRepo`] has been reloaded (detected via `Rc::ptr_eq` + /// on the family map). That happens when a later request needs a + /// wider range than the previous load covered — see the issue #92 + /// backtrack-widen path in [`PackageRepo::get_family`]. fn get_or_build( &mut self, ctx: &Rc, package_name: &str, + hint: Option<&VersionRange>, ) -> Option> { + // Fast path: cached list whose underlying family map is still + // the one the repo would return. Compare Rcs. if let Some(slot) = self.variant_lists.get(package_name) { - return slot.clone(); + match slot.as_ref() { + Some(cached_list) => { + // Repo's get_family is cheap on a covered hint (no + // reload). If it returns the same Rc we + // already built the list against, the list is still + // valid. + let fresh_map = ctx.repo.get_family(package_name, hint); + if let Some(fresh_map) = fresh_map.as_ref() { + if Rc::ptr_eq(fresh_map, &cached_list.versions) { + return Some(Rc::clone(cached_list)); + } + // The map changed under us (widen-reload). Fall + // through to rebuild against the fresh map. + } else { + // Repo lost the family between calls — treat as + // absent. + self.variant_lists.insert(Name::from(package_name), None); + return None; + } + } + None => { + // Previously absent. If the hint is wider than what + // produced the absence answer (or absence answer was + // produced with None hint), we'd see the change via + // the repo. But the repo encodes the previous hint + // and only retries when widening, so a fresh call + // here is the right gate. + let fresh = ctx.repo.get_family(package_name, hint); + if fresh.is_none() { + return None; + } + // Repo reloaded and now has the family — fall through + // to build a list from it. + } + } } - let built = PackageVariantList::new(ctx, package_name).map(Rc::new); + // Slow path: build a fresh PackageVariantList from whatever the + // repo currently has. + let built = PackageVariantList::new(ctx, package_name, hint).map(Rc::new); self.variant_lists .insert(Name::from(package_name), built.clone()); built @@ -958,7 +1020,7 @@ mod tests { fn test_build_variants_no_variants() { let r = repo(vec![("foo", vec![("1.0", pkg(&["bar-2"], &[]))])]); let ctx = ctx_with(r, &["foo"]); - let list = PackageVariantList::new(&ctx, "foo").unwrap(); + let list = PackageVariantList::new(&ctx, "foo", None).unwrap(); let entries = list.get_intersection(&VersionRange::any()).unwrap(); assert_eq!(entries.len(), 1); assert_eq!(entries[0].len(), 1); @@ -974,7 +1036,7 @@ mod tests { vec![("1.0", pkg(&["base-1"], &[&["maya-2024"], &["maya-2025"]]))], )]); let ctx = ctx_with(r, &["foo"]); - let list = PackageVariantList::new(&ctx, "foo").unwrap(); + let list = PackageVariantList::new(&ctx, "foo", None).unwrap(); let entries = list.get_intersection(&VersionRange::any()).unwrap(); assert_eq!(entries[0].len(), 2); let v0 = &entries[0].variants()[0]; @@ -995,7 +1057,7 @@ mod tests { ], )]); let ctx = ctx_with(r, &["foo"]); - let list = PackageVariantList::new(&ctx, "foo").unwrap(); + let list = PackageVariantList::new(&ctx, "foo", None).unwrap(); let entries = list.get_intersection(&VersionRange::parse("2+")).unwrap(); let versions: Vec = entries.iter().map(|e| e.version().to_string()).collect(); assert_eq!(versions, vec!["2.0", "3.0"]); diff --git a/docs/config.toml b/docs/config.toml index e3e16c9..28dbb91 100644 --- a/docs/config.toml +++ b/docs/config.toml @@ -125,7 +125,7 @@ weight = 10 name = "GitHub" pre = '' url = "https://github.com/doubleailes/rer" -post = "v0.1.0-rc.9" +post = "v1.0.0-rc.1" weight = 20 # Footer contents diff --git a/docs/content/_index.md b/docs/content/_index.md index 46882bd..99379fd 100644 --- a/docs/content/_index.md +++ b/docs/content/_index.md @@ -7,7 +7,7 @@ title = "rer — Rez En Rust" lead = "A faithful Rust port of rez's package solver — callable from Python via PyO3, resolves match rez 1:1." url = "/docs/getting-started/introduction/" url_button = "Get started" -repo_version = "GitHub v0.1.0-rc.9" +repo_version = "GitHub v1.0.0-rc.1" repo_license = "MIT-licensed." repo_url = "https://github.com/doubleailes/rer" diff --git a/docs/content/docs/getting-started/quick-start.md b/docs/content/docs/getting-started/quick-start.md index 941ca27..42bdb41 100644 --- a/docs/content/docs/getting-started/quick-start.md +++ b/docs/content/docs/getting-started/quick-start.md @@ -95,7 +95,7 @@ Add the resolver crate to your `Cargo.toml`: ```toml [dependencies] -rer-resolver = "0.1.0-rc.9" +rer-resolver = "1.0.0-rc.1" ``` Then call the solver against an in-memory repository: diff --git a/docs/content/docs/getting-started/rez-integration.md b/docs/content/docs/getting-started/rez-integration.md index 89f79e7..927ebea 100644 --- a/docs/content/docs/getting-started/rez-integration.md +++ b/docs/content/docs/getting-started/rez-integration.md @@ -149,10 +149,18 @@ been given: ```python import pyrer -def load_family(name): - """Return every PackageData for `name`, or [] if no such family.""" +def load_family(name, version_range=None): + """Return every PackageData for `name` (optionally filtered to + `version_range`), or [] if no such family. + + The `version_range` hint (issue #92) is a rez-syntax string — + `"2+<3"`, `"==2.0"`, etc. — that the shim can pass directly to + `iter_packages(range_=...)` so on-disk version dirs outside the + range are skipped before any `package.py` is opened. `None` + means "every version". + """ pkgs = [] - for pkg in iter_packages(name, paths=PACKAGE_PATHS): + for pkg in iter_packages(name, range_=version_range, paths=PACKAGE_PATHS): pkgs.append(pyrer.PackageData.from_rez(pkg)) return pkgs @@ -165,15 +173,19 @@ result = pyrer.solve( Semantics: -- The callback is called **at most once per family** in one solve - (results are cached internally), and **only for families the +- The callback is called **at most once per (family, range)** in one + solve (results are cached internally), and **only for families the solver actually exercises**. -- Returning `[]` means "no such family" — treated the same as a - family that was never added. +- Returning `[]` means "no such family in the requested range" — + treated the same as a family that was never added. - The `packages` argument is still accepted; entries supplied that way are pre-seeded into the cache and the callback is never asked for those families. Useful for a hybrid where you pre-load hot families and lazy-load the long tail. +- **Backward-compatible signature** (issue #92): a callback defined + as `def load_family(name):` keeps working — pyrer detects the + signature and only passes `version_range` when the callback can + accept it. - If the callback raises, the solve returns `result.status == "error"` with the exception message in `result.failure_description`. No exception escapes `pyrer.solve`. @@ -181,6 +193,51 @@ Semantics: family are dropped; a duplicate `(family, version)` from the callback surfaces as `status="error"`. +### The `version_range` hint (issue #92) — pre-filter at the shim + +When a request pins a version (`maya-2024+`, `python<3.12`, +`==1.5.0`), pyrer propagates that constraint immediately. It then +passes the resulting range to the `load_family` callback as the +`version_range` keyword argument. + +The shim can use it directly with rez's `iter_packages`: + +```python +def load_family(name, version_range=None): + return [ + pyrer.PackageData.from_rez(pkg) + for pkg in iter_packages(name, range_=version_range, paths=PACKAGE_PATHS) + ] +``` + +`rez.packages.iter_packages` already accepts a `range_` argument and +**skips entire version directories** that fall outside it, before +any `package.py` is opened. That cuts I/O proportionally to how +narrow the request is. + +The hint is **advisory**: + +- The shim **may** filter; pyrer re-validates anyway, so returning + the full set is correct but wasteful. +- The shim **must not** drop versions outside the hint without + reason — pyrer caches the loaded range and re-calls the loader + with a widened range if a backtrack later needs more. +- `version_range=None` means "the solver needs every version" — + always return the full family. + +**Impact** on a 132-package resolve at a 5,500-package studio +(per the issue): + +| Without hint | With hint | +|---:|---:| +| 2,637 packages loaded for 132 used (95% wasted) | ~400 packages loaded for 132 used (6.6× cut) | +| ~9 s `load_family` total | projected ~2 s | + +The hint compounds with the static-`package.py` parser (the +`pyrer.parse_static_package_py` fast path, when wired into the +shim): the parser makes each load cheap; the hint makes most +loads unnecessary. + ### When this actually helps The win is in I/O avoided, not in CPU. Specifically: diff --git a/tests/test_rich_api.py b/tests/test_rich_api.py index c20d2ba..ba72f76 100644 --- a/tests/test_rich_api.py +++ b/tests/test_rich_api.py @@ -564,6 +564,84 @@ def loader(name): assert "duplicate" in (result.failure_description or "").lower() +# --------------------------------------------------------------------------- +# load_family version_range hint (issue #92) +# --------------------------------------------------------------------------- + + +def test_load_family_range_hint_passed_for_pinned_request(): + """A `lib-2+<3` request should pass `version_range="2+<3"` to the + callback so the shim can pre-filter via `iter_packages(range_=...)`.""" + seen = [] + + def loader(name, version_range=None): + seen.append((name, version_range)) + if name == "lib": + return [_pkg("lib", "2.0.0"), _pkg("lib", "2.5.0")] + return [] + + result = pyrer.solve(["lib-2+<3"], None, load_family=loader) + assert result.status == "solved" + assert len(seen) == 1 + name, hint = seen[0] + assert name == "lib" + # The exact stringification is rez-syntax-ish; just verify the + # constraint is communicated. + assert hint is not None + assert "2" in hint and "<3" in hint + + +def test_load_family_legacy_one_arg_callback_still_works(): + """A pre-#92 callback that only accepts `name` must keep working — + pyrer falls back to the old call shape.""" + seen = [] + + def loader(name): # 1-arg, no version_range + seen.append(name) + if name == "lib": + return [_pkg("lib", "2.0.0")] + return [] + + result = pyrer.solve(["lib"], None, load_family=loader) + assert result.status == "solved" + assert seen == ["lib"] + + +def test_load_family_range_hint_string_format(): + """The hint should be a rez-style range string the shim can pass + directly to `iter_packages(range_=...)`.""" + captured_hint = [] + + def loader(name, version_range=None): + captured_hint.append((name, version_range)) + return [_pkg(name, "1.5.0"), _pkg(name, "2.0.0")] + + pyrer.solve(["foo-1+<2"], None, load_family=loader) + assert len(captured_hint) == 1 + name, hint = captured_hint[0] + assert name == "foo" + # rez accepts the hint as a string — make sure that's what we pass. + assert isinstance(hint, str) + + +def test_load_family_kwargs_callback(): + """A callback using `**kwargs` to accept future args should also work.""" + seen = [] + + def loader(name, **kwargs): + seen.append((name, kwargs.get("version_range"))) + if name == "lib": + return [_pkg("lib", "1.0.0")] + return [] + + result = pyrer.solve(["lib"], None, load_family=loader) + assert result.status == "solved" + assert len(seen) == 1 + # **kwargs accepts the hint argument + name, hint = seen[0] + assert name == "lib" + + def test_from_rez_used_in_solve(): """End-to-end: from_rez → solve produces the same result as constructor."""