diff --git a/lib/apps/fabro-server/src/manifest_validation.rs b/lib/apps/fabro-server/src/manifest_validation.rs index 329049b51..acbe81a3d 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -103,10 +103,8 @@ pub fn validate_collected_workflow( &lowered.entrypoint, &settings, &HashMap::new(), - petri_check::with_model_overrides( - petri_check::launch_without_catalog(&settings), - run_overrides.and_then(|run| run.model.as_ref()), - ), + petri_check::launch_without_catalog(&settings), + run_overrides.and_then(|run| run.model.as_ref()), offline_runtime(run_overrides), false, ) diff --git a/lib/apps/fabro-server/src/petri_check.rs b/lib/apps/fabro-server/src/petri_check.rs index a08cd516f..0fee23d46 100644 --- a/lib/apps/fabro-server/src/petri_check.rs +++ b/lib/apps/fabro-server/src/petri_check.rs @@ -93,34 +93,30 @@ pub(crate) fn launch_without_catalog(settings: &WorkflowSettings) -> Launch { } } -/// The run's explicit model flags (`--model`, `--provider`), kept apart from -/// the default the settings resolved, so they outrank the file layers and -/// the graph's defaults during admission. -pub(crate) fn with_model_overrides( - mut launch: Launch, - overrides: Option<&RunModelLayer>, -) -> Launch { - if let Some(overrides) = overrides { - launch.model.clone_from(&overrides.name); - launch.provider.clone_from(&overrides.provider); - } - launch -} - /// The check request for `bundle`'s `entrypoint`: every file of every /// workflow in the bundle at its bundle-relative path, the run's inputs and -/// variables, the launch and the runtime. `unbound_is_warning` makes a -/// template that reads an input nothing binds a warning, for a validation -/// before the run's inputs exist; a run's admission never sets it. +/// variables, the launch and the runtime. `model_overrides` are the run's +/// explicit model flags (`--model`, `--provider`), bound as the launch's +/// model apart from the default the settings resolved, so they outrank the +/// file layers and the graph's defaults during admission. Every check binds +/// them here, so the create, validate and preflight paths judge the same +/// model. `unbound_is_warning` makes a template that reads an input nothing +/// binds a warning, for a validation before the run's inputs exist; a run's +/// admission never sets it. pub(crate) fn check_request( bundle: &WorkflowBundle, entrypoint: &ManifestPath, settings: &WorkflowSettings, vars: &HashMap, - launch: Launch, + mut launch: Launch, + model_overrides: Option<&RunModelLayer>, runtime: RuntimeSpec, unbound_is_warning: bool, ) -> Result { + if let Some(overrides) = model_overrides { + launch.model.clone_from(&overrides.name); + launch.provider.clone_from(&overrides.provider); + } let mut files = BTreeMap::new(); for workflow in bundle.workflows().values() { for (path, text) in &workflow.files { @@ -237,3 +233,71 @@ fn fabro_diagnostic(diagnostic: &Diagnostic) -> FabroDiagnostic { ..FabroDiagnostic::default() } } + +#[cfg(test)] +mod tests { + use super::*; + + fn request(launch: Launch, overrides: Option<&RunModelLayer>) -> CheckRequest { + check_request( + &WorkflowBundle::default(), + &ManifestPath::from_wire("workflow.fabro").expect("the path is valid"), + &WorkflowSettings::default(), + &HashMap::new(), + launch, + overrides, + RuntimeSpec::default(), + false, + ) + .expect("the request builds") + } + + fn settings_default() -> Launch { + Launch { + default_model: Some("settings-model".to_string()), + default_provider: Some("settings-provider".to_string()), + ..Launch::default() + } + } + + #[test] + fn model_flags_bind_as_the_launch_model_beside_the_default() { + let overrides = RunModelLayer { + name: Some("flag-model".to_string()), + provider: Some("flag-provider".to_string()), + ..RunModelLayer::default() + }; + + let launch = request(settings_default(), Some(&overrides)).launch; + + assert_eq!(launch.model.as_deref(), Some("flag-model")); + assert_eq!(launch.provider.as_deref(), Some("flag-provider")); + assert_eq!(launch.default_model.as_deref(), Some("settings-model")); + assert_eq!( + launch.default_provider.as_deref(), + Some("settings-provider") + ); + } + + #[test] + fn a_provider_flag_alone_leaves_the_launch_model_unset() { + let overrides = RunModelLayer { + provider: Some("flag-provider".to_string()), + ..RunModelLayer::default() + }; + + let launch = request(settings_default(), Some(&overrides)).launch; + + assert_eq!(launch.model, None); + assert_eq!(launch.provider.as_deref(), Some("flag-provider")); + } + + #[test] + fn no_model_flags_bind_only_the_default() { + let launch = request(settings_default(), None).launch; + + assert_eq!(launch.model, None); + assert_eq!(launch.provider, None); + assert_eq!(launch.default_model.as_deref(), Some("settings-model")); + } +} diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 33c3ffa2f..665d5e078 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -211,7 +211,8 @@ pub(crate) fn validate_prepared_manifest( &prepared.target_path, &prepared.settings, vars, - petri_check::with_model_overrides(launch, prepared.model_overrides.as_ref()), + launch, + prepared.model_overrides.as_ref(), runtime, unbound_is_warning, )?; diff --git a/lib/apps/fabro-server/src/server/petri_runs.rs b/lib/apps/fabro-server/src/server/petri_runs.rs index 1a5b99a0c..734a58025 100644 --- a/lib/apps/fabro-server/src/server/petri_runs.rs +++ b/lib/apps/fabro-server/src/server/petri_runs.rs @@ -275,13 +275,13 @@ pub(crate) async fn admit( repository, ); let dry_run = settings.run.execution.mode == RunMode::DryRun; - let launch = petri_check::with_model_overrides(launch, prepared.model_overrides()); let request = petri_check::check_request( prepared.workflow_bundle(), prepared.entrypoint(), settings, prepared.vars(), launch, + prepared.model_overrides(), runtime_spec( state, eligible,