mirror of
https://github.com/storytold/lightcraft.git
synced 2026-10-26 11:09:23 -06:00
feat(engine): #1046 typed error kinds for cancelled work and missing models
EngineError gains Cancelled and ModelMissing { model: AiModel } and a
kind() -> ErrorKind, so frontends decide on what an error is rather than
on its words. The face detector, SAM 3 model checks and the Lightroom
job produce them at the source; the error text is unchanged, so control
channel and MCP clients see the same `error` strings.
Background results that cross the wire as JSON get an additive flag
instead: SAM 3 download status `cancelled`, Apple Photos import outcome
`cancelled: true` next to `skipped`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
76b5377868
commit
e4c36749ef
@@ -308,9 +308,12 @@ pub enum Outcome {
|
||||
Imported(Imported),
|
||||
/// Photos (or osascript) failed: why, for the user.
|
||||
Failed(String),
|
||||
/// Photos wasn't asked: the export that reserved the import was cancelled, wrote no files or
|
||||
/// stopped early.
|
||||
Skipped(String),
|
||||
/// Photos wasn't asked: the export that reserved the import was cancelled (`cancelled`), wrote
|
||||
/// no files or stopped early. `why` is for people; code decides on `cancelled`.
|
||||
Skipped {
|
||||
why: String,
|
||||
cancelled: bool,
|
||||
},
|
||||
}
|
||||
|
||||
/// One import: reserved by an export still writing its files, running, or finished.
|
||||
@@ -402,9 +405,12 @@ impl ImportJob {
|
||||
v["running"] = json!(false);
|
||||
v["error"] = json!(e);
|
||||
}
|
||||
Some(Outcome::Skipped(why)) => {
|
||||
Some(Outcome::Skipped { why, cancelled }) => {
|
||||
v["running"] = json!(false);
|
||||
v["skipped"] = json!(why);
|
||||
if cancelled {
|
||||
v["cancelled"] = json!(true);
|
||||
}
|
||||
}
|
||||
}
|
||||
v
|
||||
@@ -660,7 +666,7 @@ impl Reservation {
|
||||
None
|
||||
};
|
||||
if let Some(why) = skipped {
|
||||
job.finish(Outcome::Skipped(why.into()));
|
||||
job.finish(Outcome::Skipped { why: why.into(), cancelled });
|
||||
return Err(job.json());
|
||||
}
|
||||
if let Err(e) = check(&paths, &self.album) {
|
||||
@@ -700,7 +706,7 @@ impl Reservation {
|
||||
impl Drop for Reservation {
|
||||
fn drop(&mut self) {
|
||||
if let Some(job) = self.job.take() {
|
||||
job.finish(Outcome::Skipped("the export ended before writing its files".into()));
|
||||
job.finish(Outcome::Skipped { why: "the export ended before writing its files".into(), cancelled: false });
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1009,8 +1015,10 @@ mod tests {
|
||||
// nothing written, or cancelled: Photos isn't asked, and the slot is free again
|
||||
let out = imports.reserve(runner.clone(), "").unwrap().run(&[json!({"skipped": "x"})], false);
|
||||
assert!(out["skipped"].as_str().unwrap().contains("wrote no files"), "{out}");
|
||||
assert_eq!(out.get("cancelled"), None, "{out}");
|
||||
let out = imports.reserve(runner.clone(), "").unwrap().run(&exported, true);
|
||||
assert!(out["skipped"].as_str().unwrap().contains("cancelled"), "{out}");
|
||||
assert_eq!(out["cancelled"], true, "the app decides on the flag, not on the words: {out}");
|
||||
assert_eq!(calls.lock().unwrap().len(), 1);
|
||||
assert!(imports.running().is_none());
|
||||
let _ = std::fs::remove_dir_all(dir);
|
||||
|
||||
@@ -18,7 +18,7 @@ use lightcraft_meta::{Region, RegionKind};
|
||||
use serde_json::{Value, json};
|
||||
|
||||
use super::{CommandSpec, bad, cmd, has_selection};
|
||||
use crate::{Result, Session};
|
||||
use crate::{AiModel, EngineError, Result, Session};
|
||||
|
||||
const C: &str = "faces.detect";
|
||||
/// Regions made by this command say so in their description; only those are replaced on a new run.
|
||||
@@ -34,15 +34,39 @@ const EXISTING_IOU: f64 = 0.3;
|
||||
/// What to tell someone who has not downloaded the detector yet.
|
||||
const NOT_INSTALLED: &str = "finding faces needs the YuNet face detector (232 KB, MIT): download it in Settings > Faces";
|
||||
|
||||
/// Why [`detector`] has no detector to give.
|
||||
#[derive(Debug)]
|
||||
pub(crate) enum NoDetector {
|
||||
/// Not downloaded yet: a frontend offers the download ([`crate::ErrorKind::ModelMissing`]).
|
||||
NotInstalled,
|
||||
/// Nowhere to keep it, or there but unusable: says what to do.
|
||||
Unusable(String),
|
||||
}
|
||||
|
||||
impl NoDetector {
|
||||
/// Command `cmd`'s error for it. The message reads as before kinds existed (an invalid-parameters
|
||||
/// error), so control-channel and MCP clients see the same `error` text.
|
||||
pub(crate) fn into_error(self, cmd: &str) -> EngineError {
|
||||
match self {
|
||||
Self::NotInstalled => EngineError::ModelMissing { model: AiModel::FaceDetector, msg: bad(cmd, NOT_INSTALLED).to_string() },
|
||||
Self::Unusable(why) => bad(cmd, why),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// The installed YuNet, loaded once and kept (a new one is loaded when the model folder changes).
|
||||
/// (also used by the background scan, which aligns faces by the detector's landmarks and finds faces in photos that have none)
|
||||
pub(crate) fn detector(s: &Session) -> std::result::Result<Arc<Detector>, String> {
|
||||
pub(crate) fn detector(s: &Session) -> std::result::Result<Arc<Detector>, NoDetector> {
|
||||
static LOADED: Mutex<Option<(PathBuf, Arc<Detector>)>> = Mutex::new(None);
|
||||
let home = s.face_models_dir.as_deref().ok_or("this build has nowhere to keep face models (the desktop app does)")?.join(YUNET_ID);
|
||||
let home = s
|
||||
.face_models_dir
|
||||
.as_deref()
|
||||
.ok_or_else(|| NoDetector::Unusable("this build has nowhere to keep face models (the desktop app does)".into()))?
|
||||
.join(YUNET_ID);
|
||||
let path = home.join("model.onnx");
|
||||
// installing writes the manifest last, so a folder without one is not (yet) a model
|
||||
if !path.is_file() || !home.join("face-model.json").is_file() {
|
||||
return Err(NOT_INSTALLED.into());
|
||||
return Err(NoDetector::NotInstalled);
|
||||
}
|
||||
let mut loaded = LOADED.lock().unwrap_or_else(PoisonError::into_inner);
|
||||
if let Some((p, d)) = loaded.as_ref()
|
||||
@@ -50,12 +74,14 @@ pub(crate) fn detector(s: &Session) -> std::result::Result<Arc<Detector>, String
|
||||
{
|
||||
return Ok(d.clone());
|
||||
}
|
||||
let len = std::fs::metadata(&path).map_err(|e| format!("could not read the detector: {e}"))?.len();
|
||||
let len = std::fs::metadata(&path).map_err(|e| NoDetector::Unusable(format!("could not read the detector: {e}")))?.len();
|
||||
if len > MAX_MODEL_BYTES {
|
||||
return Err("the detector's file is far bigger than YuNet: remove it in Settings > Faces and download it again".into());
|
||||
return Err(NoDetector::Unusable("the detector's file is far bigger than YuNet: remove it in Settings > Faces and download it again".into()));
|
||||
}
|
||||
let bytes = std::fs::read(&path).map_err(|e| format!("could not read the detector: {e}"))?;
|
||||
let d = Arc::new(Detector::new(&bytes).map_err(|e| format!("{e}: remove the detector in Settings > Faces and download it again"))?);
|
||||
let bytes = std::fs::read(&path).map_err(|e| NoDetector::Unusable(format!("could not read the detector: {e}")))?;
|
||||
let d = Arc::new(
|
||||
Detector::new(&bytes).map_err(|e| NoDetector::Unusable(format!("{e}: remove the detector in Settings > Faces and download it again")))?,
|
||||
);
|
||||
*loaded = Some((path, d.clone()));
|
||||
Ok(d)
|
||||
}
|
||||
@@ -103,7 +129,7 @@ fn detect(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
nms_iou: p.get("nmsIou").and_then(Value::as_f64).map_or(defaults.nms_iou, |v| v as f32),
|
||||
max_faces: p.get("maxFaces").and_then(Value::as_u64).map_or(defaults.max_faces, |v| usize::try_from(v).unwrap_or(defaults.max_faces)),
|
||||
};
|
||||
let detector = detector(s).map_err(|e| bad(C, e))?;
|
||||
let detector = detector(s).map_err(|e| e.into_error(C))?;
|
||||
let started = Instant::now();
|
||||
let (mut results, mut ops) = (Vec::new(), Vec::new());
|
||||
for id in s.targets(p) {
|
||||
|
||||
@@ -84,29 +84,29 @@ fn resolve_ai(s: &mut Session, shape: &mut MaskShape, c: &str) -> Result<Later>
|
||||
match shape {
|
||||
MaskShape::Object { seg: Some(_), .. } | MaskShape::Prompt { seg: Some(_), .. } => {}
|
||||
MaskShape::Object { hint, exclude, .. } if !hint.is_empty() && background => {
|
||||
s.segmenter.model_dir().map_err(ai_err)?;
|
||||
s.segmenter.require_model()?;
|
||||
return Ok(Later::Clicks(std::mem::take(hint), std::mem::take(exclude)));
|
||||
}
|
||||
MaskShape::Object { hint, exclude, seg, .. } if !hint.is_empty() => {
|
||||
*seg = Some(s.segment_clicks(id, &clicks_of(hint, exclude)).map_err(ai_err)?);
|
||||
*seg = Some(segment(s, |s| s.segment_clicks(id, &clicks_of(hint, exclude)))?);
|
||||
}
|
||||
MaskShape::Object { .. } => {
|
||||
// the clicks come later: refuse an Object mask that could never be computed (no
|
||||
// model) rather than leave an empty one; in the app, analyze the photo meanwhile
|
||||
s.segmenter.model_dir().map_err(ai_err)?;
|
||||
s.segmenter.require_model()?;
|
||||
if background {
|
||||
s.segment_prepare(id).map_err(ai_err)?;
|
||||
segment(s, |s| s.segment_prepare(id))?;
|
||||
}
|
||||
}
|
||||
MaskShape::Prompt { text, .. } if background => {
|
||||
s.segmenter.model_dir().map_err(ai_err)?;
|
||||
s.segmenter.require_model()?;
|
||||
if text.trim().is_empty() {
|
||||
return Err(ai_err("describe what to select"));
|
||||
}
|
||||
return Ok(Later::Text(text.clone()));
|
||||
}
|
||||
MaskShape::Prompt { text, seg, .. } => {
|
||||
let found = s.segment_text(id, text).map_err(ai_err)?;
|
||||
let found = segment(s, |s| s.segment_text(id, text))?;
|
||||
*seg = Some(found.ok_or_else(|| ai_err(format!("nothing matching “{text}” was found in this photo")))?);
|
||||
}
|
||||
_ => {}
|
||||
@@ -118,7 +118,7 @@ fn resolve_ai(s: &mut Session, shape: &mut MaskShape, c: &str) -> Result<Later>
|
||||
fn queue_later(s: &mut Session, later: Later, mask: u32, comp: usize, mut out: Value) -> Result<Value> {
|
||||
if let Later::Clicks(hint, exclude) = later {
|
||||
let id = s.active().ok_or_else(|| ai_err("no active photo"))?;
|
||||
s.segment_clicks_later(id, mask, comp, hint, exclude).map_err(ai_err)?;
|
||||
segment(s, |s| s.segment_clicks_later(id, mask, comp, hint, exclude))?;
|
||||
out["pending"] = json!(true);
|
||||
}
|
||||
Ok(out)
|
||||
@@ -129,6 +129,13 @@ fn ai_err(msg: impl Into<String>) -> crate::EngineError {
|
||||
crate::EngineError::Other(msg.into())
|
||||
}
|
||||
|
||||
/// A segmentation call: the model is checked first, so a missing one is [`crate::EngineError::ModelMissing`]
|
||||
/// (the segmenter's own errors are text).
|
||||
fn segment<T>(s: &mut Session, f: impl FnOnce(&mut Session) -> std::result::Result<T, String>) -> Result<T> {
|
||||
s.segmenter.require_model()?;
|
||||
f(s).map_err(ai_err)
|
||||
}
|
||||
|
||||
pub(crate) fn clicks_of(include: &[Point], exclude: &[Point]) -> Vec<Click> {
|
||||
include.iter().map(|p| Click { at: *p, include: true }).chain(exclude.iter().map(|p| Click { at: *p, include: false })).collect()
|
||||
}
|
||||
@@ -167,7 +174,7 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
let name = str_param(p, "name").map(str::to_string);
|
||||
if let Later::Text(text) = &later {
|
||||
let id = s.active().ok_or_else(|| bad("mask.add", "no active photo"))?;
|
||||
s.segment_text_later(id, None, "add", name, text).map_err(ai_err)?;
|
||||
segment(s, |s| s.segment_text_later(id, None, "add", name, text))?;
|
||||
return Ok(json!({"pending": true}));
|
||||
}
|
||||
let next = s.active().and_then(|id| s.develop_of(id)).map(|d| d.next_mask_id()).unwrap_or(1);
|
||||
@@ -205,7 +212,7 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
let later = resolve_ai(s, &mut shape, "mask.addComponent")?;
|
||||
if let Later::Text(text) = &later {
|
||||
let op = p.get("op").and_then(Value::as_str).unwrap_or("add");
|
||||
s.segment_text_later(id, Some(mid), op, None, text).map_err(ai_err)?;
|
||||
segment(s, |s| s.segment_text_later(id, Some(mid), op, None, text))?;
|
||||
return Ok(json!({"pending": true}));
|
||||
}
|
||||
let mut comp = 0;
|
||||
@@ -365,10 +372,10 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
}
|
||||
if s.segmenter.background && !hint.is_empty() {
|
||||
let out = json!({"include": hint.len(), "exclude": ex.len(), "pending": true});
|
||||
s.segment_clicks_later(id, mid, k, hint, ex).map_err(ai_err)?;
|
||||
segment(s, |s| s.segment_clicks_later(id, mid, k, hint, ex))?;
|
||||
return Ok(out);
|
||||
}
|
||||
let seg = if hint.is_empty() { None } else { Some(s.segment_clicks(id, &clicks_of(&hint, &ex)).map_err(ai_err)?) };
|
||||
let seg = if hint.is_empty() { None } else { Some(segment(s, |s| s.segment_clicks(id, &clicks_of(&hint, &ex)))?) };
|
||||
let out = json!({"include": hint.len(), "exclude": ex.len()});
|
||||
masks_edit(s, c, "Object Mask", |masks, _| {
|
||||
let i = find(masks, mid, c)?;
|
||||
@@ -401,7 +408,7 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
.rposition(|x| matches!(x.shape, MaskShape::Object { .. } | MaskShape::Prompt { .. }))
|
||||
.ok_or_else(|| bad(c, "the mask has no Object or Describe selection"))?,
|
||||
};
|
||||
let started = s.segment_detail(id, mid, k).map_err(ai_err)?;
|
||||
let started = segment(s, |s| s.segment_detail(id, mid, k))?;
|
||||
Ok(json!({"started": started}))
|
||||
}
|
||||
),
|
||||
@@ -415,7 +422,7 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
|s, _| {
|
||||
let c = "segment.prepare";
|
||||
let id = s.active().ok_or_else(|| bad(c, "no active photo"))?;
|
||||
s.segment_prepare(id).map_err(ai_err)?;
|
||||
segment(s, |s| s.segment_prepare(id))?;
|
||||
Ok(json!({"busy": s.segmenter.busy()}))
|
||||
}
|
||||
),
|
||||
@@ -700,7 +707,7 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
|s, p| {
|
||||
let c = "spot.findBlemishes";
|
||||
let id = s.active().ok_or_else(|| bad(c, "no active photo"))?;
|
||||
let detector = super::face_detect::detector(s).map_err(|e| bad(c, e))?;
|
||||
let detector = super::face_detect::detector(s).map_err(|e| e.into_error(c))?;
|
||||
// the uncropped photo, so positions are the spots' own coordinates
|
||||
let job = s.render_job(id, 1600, 1600, false, false).ok_or_else(|| bad(c, "no photo"))?;
|
||||
let img = job.run().rendered.map_err(|e| bad(c, e))?.image;
|
||||
|
||||
@@ -0,0 +1,68 @@
|
||||
//! What kind of error a command returned, for code that decides on it (#1046).
|
||||
//!
|
||||
//! A frontend that reacts to an error (offers a model download, says "cancelled" in its own words
|
||||
//! instead of showing a failure) matches [`EngineError::kind`], never the message: messages are
|
||||
//! for people, get reworded, and will be translated.
|
||||
|
||||
use crate::EngineError;
|
||||
|
||||
/// A model a feature needs and LightCraft downloads only when the person agrees.
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
|
||||
pub enum AiModel {
|
||||
/// YuNet, which finds faces (`faces.detect`; installed in Settings ▸ Faces).
|
||||
FaceDetector,
|
||||
/// SAM 3, behind Object and Describe masks (`segment.model.download`).
|
||||
Sam3,
|
||||
}
|
||||
|
||||
/// The kind of an [`EngineError`] (or of an error a frontend raised itself: [`ErrorKind::Other`]).
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
|
||||
pub enum ErrorKind {
|
||||
UnknownCommand,
|
||||
Disabled,
|
||||
BadParams,
|
||||
Catalog,
|
||||
NotSaved,
|
||||
LibraryInUse,
|
||||
LightroomCatalog,
|
||||
/// The work was cancelled: not a failure.
|
||||
Cancelled,
|
||||
/// The feature needs this model, which is not installed (or still downloading).
|
||||
ModelMissing(AiModel),
|
||||
Other,
|
||||
}
|
||||
|
||||
impl EngineError {
|
||||
/// What kind of error this is: decide on this, not on the message.
|
||||
pub fn kind(&self) -> ErrorKind {
|
||||
match self {
|
||||
Self::UnknownCommand(_) => ErrorKind::UnknownCommand,
|
||||
Self::Disabled(..) => ErrorKind::Disabled,
|
||||
Self::BadParams { .. } => ErrorKind::BadParams,
|
||||
Self::Catalog(_) => ErrorKind::Catalog,
|
||||
Self::NotSaved(_) => ErrorKind::NotSaved,
|
||||
Self::LibraryInUse(_) => ErrorKind::LibraryInUse,
|
||||
Self::LightroomCatalog { .. } => ErrorKind::LightroomCatalog,
|
||||
Self::Cancelled(_) => ErrorKind::Cancelled,
|
||||
Self::ModelMissing { model, .. } => ErrorKind::ModelMissing(*model),
|
||||
Self::Other(_) => ErrorKind::Other,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn the_kind_does_not_depend_on_the_message() {
|
||||
let cancelled = EngineError::Cancelled("Lightroom-Import abgebrochen".into());
|
||||
assert_eq!(cancelled.kind(), ErrorKind::Cancelled);
|
||||
assert_eq!(cancelled.to_string(), "Lightroom-Import abgebrochen", "the message is shown as is");
|
||||
let missing = EngineError::ModelMissing { model: AiModel::Sam3, msg: "any wording".into() };
|
||||
assert_eq!(missing.kind(), ErrorKind::ModelMissing(AiModel::Sam3));
|
||||
assert_eq!(missing.to_string(), "any wording");
|
||||
// an `Other` whose text reads like a cancellation or a missing model is still `Other`
|
||||
assert_eq!(EngineError::Other("The SAM 3 model is not installed; cancelled".into()).kind(), ErrorKind::Other);
|
||||
}
|
||||
}
|
||||
@@ -29,6 +29,7 @@ pub mod demo;
|
||||
pub mod denoise;
|
||||
pub mod devices;
|
||||
pub mod display;
|
||||
mod errors;
|
||||
pub mod export;
|
||||
pub mod face_download;
|
||||
#[cfg(not(target_arch = "wasm32"))]
|
||||
@@ -68,6 +69,7 @@ pub mod walk;
|
||||
use std::sync::Arc;
|
||||
|
||||
pub use cmd::{CommandInfo, CommandSpec, command_specs, find_command};
|
||||
pub use errors::{AiModel, ErrorKind};
|
||||
pub use fonts::{CRAFT_FONTS, CraftFont};
|
||||
use lightcraft_catalog::{Catalog, Filter, Op, PhotoId, Sort};
|
||||
use lightcraft_develop::DevelopSettings;
|
||||
@@ -98,6 +100,13 @@ pub enum EngineError {
|
||||
/// to import with `library.importLightroom`, when one was found.
|
||||
#[error("{} is a Lightroom Classic catalog, not a LightCraft library; import it into a LightCraft library with File ▸ Import Lightroom Catalog…{}", path.display(), catalog.as_ref().map_or(String::new(), |c| format!(" ({})", c.display())))]
|
||||
LightroomCatalog { path: std::path::PathBuf, catalog: Option<std::path::PathBuf> },
|
||||
/// The work was cancelled (by the person, an agent or a newer request): not a failure.
|
||||
#[error("{0}")]
|
||||
Cancelled(String),
|
||||
/// The feature needs `model`, which is not installed (or still downloading): a frontend
|
||||
/// offers its download (terms first) on [`ErrorKind::ModelMissing`], not on the message.
|
||||
#[error("{msg}")]
|
||||
ModelMissing { model: AiModel, msg: String },
|
||||
#[error("{0}")]
|
||||
Other(String),
|
||||
}
|
||||
|
||||
@@ -24,6 +24,12 @@ fn error(message: impl Into<String>) -> crate::EngineError {
|
||||
crate::EngineError::BadParams { cmd: COMMAND.into(), msg: message.into() }
|
||||
}
|
||||
|
||||
/// The import was cancelled ([`crate::ErrorKind::Cancelled`]: not a failure). The message is the
|
||||
/// one it always had, so control-channel and MCP clients see the same `error` text.
|
||||
fn cancelled() -> crate::EngineError {
|
||||
crate::EngineError::Cancelled(error("Lightroom import cancelled").to_string())
|
||||
}
|
||||
|
||||
fn stage_archive(archive_dir: Option<&Path>, data: &mut CatalogImport) -> crate::Result<Option<PathBuf>> {
|
||||
let archive_path = archive_dir.map(|dir| crate::lightroom_archive::store_best_effort(dir, data).map_err(error)).transpose()?.flatten();
|
||||
if archive_dir.is_some() && archive_path.is_none() {
|
||||
@@ -180,14 +186,16 @@ impl LightroomJob {
|
||||
/// Read/probe/stage all source data off the owner thread.
|
||||
pub fn prepare(&mut self, cancel: &AtomicBool) -> crate::Result<PreparedLightroom> {
|
||||
if cancel.load(Ordering::Relaxed) {
|
||||
return Err(error("Lightroom import cancelled"));
|
||||
return Err(cancelled());
|
||||
}
|
||||
self.import.total.store(1, Ordering::Relaxed);
|
||||
self.import.done.store(0, Ordering::Relaxed);
|
||||
let mut data = lightroom_catalog::read_with_progress(&self.path, cancel, &self.import.total, &self.import.done).map_err(error)?;
|
||||
// (reading stops with a message when `cancel` is set: the flag says it was cancelled)
|
||||
let mut data = lightroom_catalog::read_with_progress(&self.path, cancel, &self.import.total, &self.import.done)
|
||||
.map_err(|e| if cancel.load(Ordering::Relaxed) { cancelled() } else { error(e) })?;
|
||||
lightroom_catalog::validate_for_job(&data).map_err(error)?;
|
||||
if cancel.load(Ordering::Relaxed) {
|
||||
return Err(error("Lightroom import cancelled"));
|
||||
return Err(cancelled());
|
||||
}
|
||||
self.import.done.fetch_add(1, Ordering::Relaxed);
|
||||
let paths: Vec<String> = data
|
||||
@@ -206,7 +214,7 @@ impl LightroomJob {
|
||||
let normal = self.import.prepare(&paths, cancel);
|
||||
if cancel.load(Ordering::Relaxed) {
|
||||
normal.rollback();
|
||||
return Err(error("Lightroom import cancelled"));
|
||||
return Err(cancelled());
|
||||
}
|
||||
let archive_path = stage_archive(self.archive_dir.as_deref(), &mut data)?;
|
||||
let index_path = self.archive_dir.as_ref().map(|dir| dir.join("lightroom-index.json"));
|
||||
@@ -229,7 +237,7 @@ impl LightroomJob {
|
||||
pub fn commit_prepared(s: &mut crate::Session, mut prepared: PreparedLightroom) -> crate::Result<LightroomCompletion> {
|
||||
if prepared.cancelled {
|
||||
prepared.normal.rollback();
|
||||
return Err(error("Lightroom import cancelled"));
|
||||
return Err(cancelled());
|
||||
}
|
||||
if !prepared.token.matches(s) {
|
||||
prepared.normal.rollback();
|
||||
@@ -340,11 +348,26 @@ mod tests {
|
||||
let mut session = crate::Session::new();
|
||||
let before = session.catalog.to_snapshot();
|
||||
let prepared = cancelled_prepared(&session);
|
||||
assert!(commit_prepared(&mut session, prepared).is_err());
|
||||
let Err(e) = commit_prepared(&mut session, prepared) else { panic!("a cancelled import commits nothing") };
|
||||
assert_eq!(e.kind(), crate::ErrorKind::Cancelled, "{e}");
|
||||
assert_eq!(session.catalog.to_snapshot(), before);
|
||||
assert!(session.undo.is_empty());
|
||||
}
|
||||
|
||||
/// Cancelling while the catalog is read is [`crate::ErrorKind::Cancelled`] whatever the message
|
||||
/// says, and a failure that is not a cancellation is not one, even when its text says "cancelled".
|
||||
#[test]
|
||||
fn a_cancelled_read_is_cancelled_by_kind() {
|
||||
let mut session = crate::Session::new();
|
||||
let mut job = LightroomJob::new(&mut session, PathBuf::from("/nonexistent/cancelled.lrcat"), false).unwrap();
|
||||
let Err(e) = job.prepare(&AtomicBool::new(true)) else { panic!("cancelled before it started") };
|
||||
assert_eq!(e.kind(), crate::ErrorKind::Cancelled, "{e}");
|
||||
// the `error` text clients see is unchanged
|
||||
assert_eq!(e.to_string(), "invalid parameters for `library.importLightroom`: Lightroom import cancelled");
|
||||
let Err(e) = job.prepare(&AtomicBool::new(false)) else { panic!("no such catalog") };
|
||||
assert_ne!(e.kind(), crate::ErrorKind::Cancelled, "{e}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn library_identity_rejects_late_completion_after_reopen() {
|
||||
let mut session = crate::Session::new();
|
||||
|
||||
@@ -20,6 +20,8 @@ pub struct Status {
|
||||
pub file: String,
|
||||
/// The last download's error (cleared when a new one starts).
|
||||
pub error: Option<String>,
|
||||
/// The last download was cancelled (its `error` says so in words): decide on this, not on the text.
|
||||
pub cancelled: bool,
|
||||
/// The last download finished and the files are in place.
|
||||
pub finished: bool,
|
||||
}
|
||||
@@ -28,6 +30,7 @@ pub struct Status {
|
||||
struct Text {
|
||||
file: String,
|
||||
error: Option<String>,
|
||||
cancelled: bool,
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
@@ -68,15 +71,17 @@ impl Downloader {
|
||||
{
|
||||
last.file.clone_from(&t.file);
|
||||
last.error.clone_from(&t.error);
|
||||
last.cancelled = t.cancelled;
|
||||
}
|
||||
// (only the session's thread locks `last`)
|
||||
let (file, error) = self.last.try_lock().map(|l| (l.file.clone(), l.error.clone())).unwrap_or_default();
|
||||
let (file, error, cancelled) = self.last.try_lock().map(|l| (l.file.clone(), l.error.clone(), l.cancelled)).unwrap_or_default();
|
||||
Status {
|
||||
running: s.running.load(Ordering::SeqCst),
|
||||
done: s.done.load(Ordering::SeqCst),
|
||||
total: s.total.load(Ordering::SeqCst),
|
||||
file,
|
||||
error,
|
||||
cancelled,
|
||||
finished: s.finished.load(Ordering::SeqCst),
|
||||
}
|
||||
}
|
||||
@@ -135,6 +140,7 @@ impl Downloader {
|
||||
};
|
||||
let r =
|
||||
std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| fetch::download(files, &mirrors, &dir, &opts, &s.cancel, &mut on_progress)));
|
||||
let cancelled = matches!(r, Ok(Err(fetch::DownloadError::Cancelled)));
|
||||
let error = match r {
|
||||
Ok(Ok(())) => {
|
||||
s.finished.store(true, Ordering::SeqCst);
|
||||
@@ -147,7 +153,10 @@ impl Downloader {
|
||||
if let Some(e) = &error {
|
||||
log::warn!("SAM 3 download: {e}");
|
||||
}
|
||||
lock(&s.text).error = error;
|
||||
let mut text = lock(&s.text);
|
||||
text.cancelled = cancelled;
|
||||
text.error = error;
|
||||
drop(text);
|
||||
drop(guard);
|
||||
});
|
||||
match spawned {
|
||||
|
||||
@@ -35,7 +35,7 @@ pub const MODEL_BYTES: u64 = 3_439_938_512;
|
||||
/// The model's licence (not LightCraft's): shown before downloading.
|
||||
pub const LICENSE_NAME: &str = "SAM License (Meta)";
|
||||
pub const LICENSE_URL: &str = "https://github.com/facebookresearch/sam3/blob/main/LICENSE";
|
||||
/// How errors about a missing model start (the UI offers the download on it).
|
||||
/// How errors about a missing model start (frontends decide on [`crate::ErrorKind::ModelMissing`], not on this text).
|
||||
pub const NOT_INSTALLED: &str = "The SAM 3 model is not installed";
|
||||
/// Longest a waiting command (CLI, MCP) waits for the model (loading on a CPU can take minutes).
|
||||
#[cfg_attr(not(feature = "sam"), allow(dead_code))]
|
||||
@@ -111,6 +111,8 @@ pub struct DownloadStatus {
|
||||
pub total: u64,
|
||||
pub file: String,
|
||||
pub error: Option<String>,
|
||||
/// The last download was cancelled (it resumes next time): decide on this, not on `error`'s text.
|
||||
pub cancelled: bool,
|
||||
pub finished: bool,
|
||||
}
|
||||
|
||||
@@ -215,7 +217,15 @@ impl Segmenter {
|
||||
#[cfg(feature = "sam")]
|
||||
{
|
||||
let s = self.download.status();
|
||||
DownloadStatus { running: s.running, done: s.done, total: s.total, file: s.file, error: s.error, finished: s.finished }
|
||||
DownloadStatus {
|
||||
running: s.running,
|
||||
done: s.done,
|
||||
total: s.total,
|
||||
file: s.file,
|
||||
error: s.error,
|
||||
cancelled: s.cancelled,
|
||||
finished: s.finished,
|
||||
}
|
||||
}
|
||||
#[cfg(not(feature = "sam"))]
|
||||
DownloadStatus::default()
|
||||
@@ -234,27 +244,36 @@ impl Segmenter {
|
||||
|
||||
/// The model folder, if the model is there; otherwise an error that says what to do.
|
||||
pub fn model_dir(&self) -> Result<PathBuf, String> {
|
||||
self.require_model().map_err(|e| e.to_string())
|
||||
}
|
||||
|
||||
/// [`Segmenter::model_dir`] for commands: a model that is not installed (or still downloading) is
|
||||
/// [`crate::EngineError::ModelMissing`], so a frontend offers the download without reading the message.
|
||||
pub fn require_model(&self) -> crate::Result<PathBuf> {
|
||||
let other = |why: &str| crate::EngineError::Other(why.into());
|
||||
if !Self::AVAILABLE {
|
||||
return Err("AI masks are not available in this build".into());
|
||||
return Err(other("AI masks are not available in this build"));
|
||||
}
|
||||
#[cfg(feature = "sam")]
|
||||
if lightcraft_segment::remote::configured().is_some() {
|
||||
return Ok(PathBuf::new());
|
||||
}
|
||||
let dir = self.dir.clone().ok_or("no folder is set for the SAM 3 model")?;
|
||||
let dir = self.dir.clone().ok_or_else(|| other("no folder is set for the SAM 3 model"))?;
|
||||
if self.installed() {
|
||||
return Ok(dir);
|
||||
}
|
||||
let d = self.download_status();
|
||||
if d.running {
|
||||
let msg = if d.running {
|
||||
let pct = d.done.saturating_mul(100).checked_div(d.total).unwrap_or(0);
|
||||
return Err(format!("{NOT_INSTALLED} yet: it is downloading ({pct} %)."));
|
||||
}
|
||||
Err(format!(
|
||||
"{NOT_INSTALLED}. Download it (about {:.1} GB, {LICENSE_NAME}) when LightCraft offers it, with `segment.model.download {{\"acknowledged\": true}}`, or put model.safetensors, vocab.json and merges.txt in {}.",
|
||||
MODEL_BYTES as f64 / 1e9,
|
||||
dir.display()
|
||||
))
|
||||
format!("{NOT_INSTALLED} yet: it is downloading ({pct} %).")
|
||||
} else {
|
||||
format!(
|
||||
"{NOT_INSTALLED}. Download it (about {:.1} GB, {LICENSE_NAME}) when LightCraft offers it, with `segment.model.download {{\"acknowledged\": true}}`, or put model.safetensors, vocab.json and merges.txt in {}.",
|
||||
MODEL_BYTES as f64 / 1e9,
|
||||
dir.display()
|
||||
)
|
||||
};
|
||||
Err(crate::EngineError::ModelMissing { model: crate::AiModel::Sam3, msg })
|
||||
}
|
||||
|
||||
/// Start downloading the model (on a background thread; see `segment.model.download`).
|
||||
|
||||
@@ -205,10 +205,24 @@ fn detect_without_the_model_says_where_to_get_it() {
|
||||
let d = temp("nodetector");
|
||||
let mut s = Session::with_demo();
|
||||
// no folder for models at all, then a folder that has none yet
|
||||
assert!(s.execute("faces.detect", &json!({})).is_err());
|
||||
let e = s.execute("faces.detect", &json!({})).unwrap_err();
|
||||
assert_ne!(e.kind(), crate::ErrorKind::ModelMissing(crate::AiModel::FaceDetector), "nothing to download into: {e}");
|
||||
s.face_models_dir = Some(d.join("models"));
|
||||
let e = s.execute("faces.detect", &json!({})).unwrap_err().to_string();
|
||||
assert!(e.contains("Settings > Faces"), "{e}");
|
||||
let e = s.execute("faces.detect", &json!({})).unwrap_err();
|
||||
// the app offers the download on the kind (#1046), the person reads the message
|
||||
assert_eq!(e.kind(), crate::ErrorKind::ModelMissing(crate::AiModel::FaceDetector), "{e}");
|
||||
assert!(
|
||||
e.to_string().starts_with("invalid parameters for `faces.detect`: ") && e.to_string().contains("Settings > Faces"),
|
||||
"the `error` text clients see is unchanged: {e}"
|
||||
);
|
||||
// a detector that is there but unusable also points to Settings > Faces, and is not a missing one
|
||||
let home = d.join("models").join(lightcraft_faces::known::YUNET_ID);
|
||||
std::fs::create_dir_all(&home).unwrap();
|
||||
std::fs::write(home.join("model.onnx"), b"not a model").unwrap();
|
||||
std::fs::write(home.join("face-model.json"), b"{}").unwrap();
|
||||
let e = s.execute("faces.detect", &json!({})).unwrap_err();
|
||||
assert!(e.to_string().contains("Settings > Faces"), "{e}");
|
||||
assert_eq!(e.kind(), crate::ErrorKind::BadParams, "{e}");
|
||||
let _ = std::fs::remove_dir_all(&d);
|
||||
}
|
||||
|
||||
|
||||
@@ -115,6 +115,7 @@ fn a_failing_download_ends_with_an_error_and_never_blocks() {
|
||||
}
|
||||
let st = s.segmenter.download_status();
|
||||
assert!(!st.finished);
|
||||
assert!(!st.cancelled, "a failure is not a cancel");
|
||||
let e = st.error.unwrap();
|
||||
assert!(e.contains("not found"), "{e}");
|
||||
assert!(!s.segmenter.installed());
|
||||
@@ -272,9 +273,35 @@ fn a_running_model_download_shows_in_activity_and_cancels() {
|
||||
assert!(t.elapsed() < Duration::from_secs(10), "the download never stopped");
|
||||
std::thread::sleep(Duration::from_millis(10));
|
||||
}
|
||||
let e = s.segmenter.download_status().error.unwrap_or_default();
|
||||
let st = s.segmenter.download_status();
|
||||
assert!(st.cancelled, "the app tells a cancel by the flag (#1046): {st:?}");
|
||||
let e = st.error.unwrap_or_default();
|
||||
assert!(e.contains("cancelled"), "{e}");
|
||||
assert!(!s.segmenter.installed());
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
let _ = std::fs::remove_file(&mirrors);
|
||||
}
|
||||
|
||||
/// Frontends decide on the error's kind, not its words (#1046): a missing model is
|
||||
/// `ModelMissing(Sam3)` whether the app queues requests (background) or a command waits (CLI, MCP);
|
||||
/// a build without AI masks is not a missing model (there is nothing to download).
|
||||
#[test]
|
||||
fn a_missing_model_is_model_missing_by_kind() {
|
||||
use crate::{AiModel, ErrorKind};
|
||||
for background in [true, false] {
|
||||
let mut s = Session::with_demo();
|
||||
s.segmenter.background = background;
|
||||
s.segmenter.dir = Some(tmp(&format!("kind-{background}")));
|
||||
for (cmd, p) in
|
||||
[("mask.add", json!({"kind": "object"})), ("mask.add", json!({"kind": "prompt", "text": "sky"})), ("segment.prepare", json!({}))]
|
||||
{
|
||||
let e = s.execute(cmd, &p).unwrap_err();
|
||||
if Segmenter::AVAILABLE {
|
||||
assert_eq!(e.kind(), ErrorKind::ModelMissing(AiModel::Sam3), "{cmd} (background {background}): {e}");
|
||||
assert!(e.to_string().starts_with(NOT_INSTALLED), "the message is unchanged: {e}");
|
||||
} else {
|
||||
assert_eq!(e.kind(), ErrorKind::Other, "{cmd}: {e}");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user