diff --git a/CHANGELOG.md b/CHANGELOG.md index 764d4fda..8065f272 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,15 @@ # Unreleased +- Fix `re get datasets`, `re prune`, `re get custom-label-trend-report` and `re package upload` + failing for a whole tenant when any one of its datasets used an attribution method, model version, + extraction flag or dataset flag this CLI didn't recognise — even when the dataset being operated on + did not. Adds the missing values, and keeps any added in future as-is rather than rejecting them, + so `re package upload` still sends a dataset's attribution method, model version and extraction + flags back unchanged +- Fix `re package upload` ignoring `--dataset-creation-timeout` and waiting forever when a dataset + never finished being created +- Breaking (`reinfer-client` API): `DatasetFlag`, `AttributionMethod`, `GptModelVersion` and + `GptIxpFlag` each gain an `Unknown` variant, so exhaustive matches on them need an extra arm, and + `AttributionMethod`, `GptModelVersion` and `GptIxpFlag` are no longer `Copy` - Drop the autotools build dependency: libpff now builds from a release tarball that ships a pre-generated `configure`, so `autoconf`/`automake`/`libtool` (and the macOS MacPorts bootstrap) are no longer required to build from source diff --git a/api/Cargo.toml b/api/Cargo.toml index 26c24689..760246e9 100644 --- a/api/Cargo.toml +++ b/api/Cargo.toml @@ -20,7 +20,7 @@ once_cell = "1.16.0" ordered-float = { version = "3.9.1", features = ["serde"] } regex = "1.6.0" reqwest = { version = "0.11.12", default-features = false, features = ["blocking", "gzip", "json", "multipart", "native-tls-vendored"] } -serde = { version = "1.0.147", features = ["derive"] } +serde = { version = "1.0.164", features = ["derive"] } serde_json = { version = "1.0.87", features = ["unbounded_depth"] } serde_with = "2.0.1" thiserror = "1.0.37" diff --git a/api/src/lib.rs b/api/src/lib.rs index 56839ab7..4256a2a9 100644 --- a/api/src/lib.rs +++ b/api/src/lib.rs @@ -2592,7 +2592,7 @@ fn build_headers(config: &Config) -> Result { let mut headers = HeaderMap::new(); headers.insert( header::AUTHORIZATION, - HeaderValue::from_str(&format!("Bearer {}", &config.token.0)).map_err(|_| { + HeaderValue::from_str(&format!("Bearer {}", config.token.0)).map_err(|_| { Error::BadToken { token: config.token.0.clone(), } diff --git a/api/src/resources/dataset.rs b/api/src/resources/dataset.rs index ee0ca5cd..ba43fbfe 100644 --- a/api/src/resources/dataset.rs +++ b/api/src/resources/dataset.rs @@ -21,13 +21,20 @@ use std::{ use super::validation::ValidationResponse; #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] -#[serde(rename_all(serialize = "snake_case", deserialize = "snake_case"))] +#[serde(rename_all = "snake_case")] pub enum DatasetFlag { Gpt4, ExternalMoonLlm, Qos, ZeroShotLabels, Ixp, + ConversationalFilters, + GenerativeExtraction, + GenerativePrelabelling, + LlmAssistedLabelling, + /// A dataset flag added to the platform after this release, kept as-is. + #[serde(untagged)] + Unknown(Box), } #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] @@ -133,14 +140,16 @@ pub struct GptIxpModelConfig { pub attribution_method: AttributionMethod, } -#[derive( - Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize, Default, -)] +#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize, Default)] #[serde(rename_all = "snake_case")] pub enum AttributionMethod { #[default] TableHeuristic, WordIds, + TableFormattedWordIds, + /// An attribution method added to the platform after this release, kept as-is. + #[serde(untagged)] + Unknown(Box), } #[derive(Clone, Default, Debug, PartialEq, Serialize, Deserialize, Eq)] @@ -161,7 +170,7 @@ pub struct IterativeConfig { pub chunk_size: Option>>, } -#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize)] +#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize)] pub enum GptModelVersion { #[serde(rename = "gpt_4o_2024_05_13")] Gpt4o20240513, @@ -175,6 +184,11 @@ pub enum GptModelVersion { GeminiPro25, #[serde(rename = "gemini_3_1_pro_preview")] Gemini31ProPreview, + #[serde(rename = "gemini_3_1_flash_lite_preview")] + Gemini31FlashLitePreview, + /// A model version added to the platform after this release, kept as-is. + #[serde(untagged)] + Unknown(Box), } #[derive(Eq, Clone, Debug, PartialEq, Serialize, Deserialize)] @@ -212,16 +226,16 @@ pub enum TextImageInputConfigMode { #[serde(rename = "text_plus_image")] TextPlusImage, } -#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize)] +#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] pub enum GptIxpFlag { - #[serde(rename = "append_taxonomy_descriptions")] AppendTaxonomyDescriptions, - #[serde(rename = "append_type_descriptions")] AppendTypeDescriptions, - #[serde(rename = "append_group_descriptions")] AppendGroupDescriptions, - #[serde(rename = "append_field_descriptions")] AppendFieldDescriptions, + /// An extraction flag added to the platform after this release, kept as-is. + #[serde(untagged)] + Unknown(Box), } #[derive(Debug, Clone, Deserialize, Serialize, PartialEq)] @@ -728,4 +742,160 @@ mod tests { r#"{"filter":{"user_properties":{"string:Generation Tag":{"one_of":["72b01fe7-ef2e-481e-934d-bc2fe0ca9b06"]}}},"limit":20,"order":{"kind":"recent"}}"# ); } + + // Each table lists every value the platform currently sends, so that known + // values get a real variant instead of falling into `Unknown`. + + /// Asserts each value deserializes to the expected variant and serializes + /// back unchanged. + fn assert_round_trips(cases: &[(&str, T)]) + where + T: serde::de::DeserializeOwned + Serialize + PartialEq + std::fmt::Debug, + { + for (wire, expected) in cases { + let json = format!("\"{wire}\""); + let parsed: T = serde_json::from_str(&json) + .unwrap_or_else(|error| panic!("`{wire}` should deserialize: {error}")); + assert_eq!( + &parsed, expected, + "`{wire}` deserialized to the wrong variant" + ); + assert_eq!(serde_json::to_string(&parsed).unwrap(), json); + } + } + + #[test] + fn test_every_dataset_flag_round_trips() { + assert_round_trips(&[ + ("gpt4", DatasetFlag::Gpt4), + ("external_moon_llm", DatasetFlag::ExternalMoonLlm), + ("qos", DatasetFlag::Qos), + ("zero_shot_labels", DatasetFlag::ZeroShotLabels), + ("ixp", DatasetFlag::Ixp), + ("conversational_filters", DatasetFlag::ConversationalFilters), + ("generative_extraction", DatasetFlag::GenerativeExtraction), + ( + "generative_prelabelling", + DatasetFlag::GenerativePrelabelling, + ), + ("llm_assisted_labelling", DatasetFlag::LlmAssistedLabelling), + ( + "some_future_flag", + DatasetFlag::Unknown("some_future_flag".into()), + ), + ]); + } + + #[test] + fn test_every_attribution_method_round_trips() { + assert_round_trips(&[ + ("table_heuristic", AttributionMethod::TableHeuristic), + ("word_ids", AttributionMethod::WordIds), + ( + "table_formatted_word_ids", + AttributionMethod::TableFormattedWordIds, + ), + ( + "some_future_attribution", + AttributionMethod::Unknown("some_future_attribution".into()), + ), + ]); + } + + #[test] + fn test_every_model_version_round_trips() { + assert_round_trips(&[ + ("gpt_4o_2024_05_13", GptModelVersion::Gpt4o20240513), + ("gpt_5_1_2025_11_13", GptModelVersion::Gpt5120251113), + ("gpt_5_4_2026_03_05", GptModelVersion::Gpt5420260305), + ("gemini_2_5_flash", GptModelVersion::GeminiFlash25), + ("gemini_2_5_pro", GptModelVersion::GeminiPro25), + ( + "gemini_3_1_pro_preview", + GptModelVersion::Gemini31ProPreview, + ), + ( + "gemini_3_1_flash_lite_preview", + GptModelVersion::Gemini31FlashLitePreview, + ), + ("gpt_9", GptModelVersion::Unknown("gpt_9".into())), + ]); + } + + #[test] + fn test_every_gpt_ixp_flag_round_trips() { + assert_round_trips(&[ + ( + "append_taxonomy_descriptions", + GptIxpFlag::AppendTaxonomyDescriptions, + ), + ( + "append_type_descriptions", + GptIxpFlag::AppendTypeDescriptions, + ), + ( + "append_group_descriptions", + GptIxpFlag::AppendGroupDescriptions, + ), + ( + "append_field_descriptions", + GptIxpFlag::AppendFieldDescriptions, + ), + ( + "append_something_new", + GptIxpFlag::Unknown("append_something_new".into()), + ), + ]); + } + + /// Listing commands parse every dataset on the tenant at once, so one + /// dataset using newer values must not fail the whole response. + #[test] + fn test_deserialize_tenant_listing_with_newer_model_config_values() { + let response: GetAvailableResponse = serde_json::from_str( + r#"{"datasets":[ + {"id":"aaaaaaaaaaaaaaaa","name":"unrelated","owner":"proj","title":"Unrelated", + "description":"","created":"2026-01-01T00:00:00Z", + "last_modified":"2026-01-01T00:00:00Z","model_family":"english","source_ids":[], + "has_sentiment":false,"entity_defs":[],"general_fields":[],"label_defs":[], + "label_groups":[], + "_dataset_flags":["generative_extraction","a_flag_from_the_future"], + "_model_config":{"kind":"cm"}}, + {"id":"bbbbbbbbbbbbbbbb","name":"ixp-one","owner":"proj","title":"IXP", + "description":"","created":"2026-01-01T00:00:00Z", + "last_modified":"2026-01-01T00:00:00Z","model_family":"english","source_ids":[], + "has_sentiment":false,"entity_defs":[],"general_fields":[],"label_defs":[], + "label_groups":[],"_dataset_flags":["ixp"], + "_model_config":{"kind":"gpt_ixp","flags":["a_flag_from_the_future"], + "model_version":"a_model_from_the_future", + "attribution_method":"table_formatted_word_ids"}} + ]}"#, + ) + .expect("a tenant listing must parse even when a dataset uses newer values"); + + assert_eq!(response.datasets.len(), 2); + assert!(response.datasets[0].has_flag(DatasetFlag::GenerativeExtraction)); + assert!( + response.datasets[0].has_flag(DatasetFlag::Unknown("a_flag_from_the_future".into())) + ); + + let ModelConfig::GptIxp(config) = &response.datasets[1].model_config else { + panic!("expected a gpt_ixp config"); + }; + assert_eq!( + config.attribution_method, + AttributionMethod::TableFormattedWordIds + ); + assert_eq!( + config.model_version, + Some(GptModelVersion::Unknown("a_model_from_the_future".into())) + ); + + // `re package upload` sends the config straight back, so unrecognised + // values also have to re-serialize exactly as they arrived. + assert_eq!( + serde_json::to_string(&response.datasets[1].model_config).unwrap(), + r#"{"kind":"gpt_ixp","model_version":"a_model_from_the_future","flags":["a_flag_from_the_future"],"attribution_method":"table_formatted_word_ids"}"# + ); + } } diff --git a/cli/src/commands/create/annotations.rs b/cli/src/commands/create/annotations.rs index b4d1ca7e..d8e72a0c 100644 --- a/cli/src/commands/create/annotations.rs +++ b/cli/src/commands/create/annotations.rs @@ -180,10 +180,7 @@ pub fn upload_batch_of_annotations( ) }) .with_context(|| { - format!( - "Could not update labelling for comment `{}`", - &comment_uid.0 - ) + format!("Could not update labelling for comment `{}`", comment_uid.0) }); if let Err(error) = result { diff --git a/cli/src/commands/package/upload.rs b/cli/src/commands/package/upload.rs index 93cd1f2e..13b430c7 100644 --- a/cli/src/commands/package/upload.rs +++ b/cli/src/commands/package/upload.rs @@ -91,25 +91,41 @@ pub struct UploadPackageArgs { } fn wait_for_dataset_to_exist(dataset: &Dataset, client: &Client, timeout_s: u64) -> Result<()> { - let start_time = Instant::now(); + let full_name = dataset.full_name(); - while (start_time - Instant::now()).as_secs() <= timeout_s { + wait_until("dataset to be created", timeout_s, || { refresh_user_permissions(client, false)?; - let datasets = client.get_datasets()?; - let dataset_exists = datasets + Ok(client + .get_datasets()? .iter() - .map(|dataset| dataset.full_name()) - .contains(&dataset.full_name()); + .any(|dataset| dataset.full_name() == full_name)) + }) +} + +/// Polls `is_ready` every 500ms until it reports `true`, giving up with a +/// timeout error naming `what` once `timeout_s` has elapsed. Note the deadline +/// is only checked between polls, so a slow `is_ready` can overrun it by one +/// call. +/// +/// Kept separate from its only caller so the deadline handling can be tested +/// without a `Client`. +fn wait_until( + what: &str, + timeout_s: u64, + mut is_ready: impl FnMut() -> Result, +) -> Result<()> { + let start_time = Instant::now(); + let timeout = Duration::from_secs(timeout_s); - if dataset_exists { + while start_time.elapsed() <= timeout { + if is_ready()? { return Ok(()); - } else { - sleep(Duration::from_millis(500)); } + sleep(Duration::from_millis(500)); } - Err(anyhow!("Timeout waiting for dataset to be created")) + Err(anyhow!("Timeout waiting for {what}")) } fn create_ixp_dataset( @@ -1214,3 +1230,50 @@ fn get_ixp_progress_bar(total_comments: u64, statistics: &Arc) -> crate::progress::Options { bytes_units: false }, ) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn wait_until_keeps_polling_until_ready() { + let mut polls = 0; + + wait_until("dataset", 30, || { + polls += 1; + Ok(polls == 3) + }) + .expect("should succeed"); + + assert_eq!(polls, 3, "should keep polling while not yet ready"); + } + + #[test] + fn wait_until_gives_up_once_the_timeout_elapses() { + // The deadline used to be computed the wrong way round, which made the + // loop condition permanently true, so this looped forever instead of + // returning an error. Run it on its own thread so that reintroducing + // the bug fails the test instead of hanging the suite. + let (sender, receiver) = channel(); + + std::thread::spawn(move || { + let _ = sender.send(wait_until("dataset", 0, || Ok(false)).is_err()); + }); + + match receiver.recv_timeout(Duration::from_secs(10)) { + Ok(timed_out) => assert!(timed_out, "should report a timeout"), + Err(_) => panic!("wait_until did not return -- the deadline is never reached"), + } + } + + #[test] + fn wait_until_propagates_errors() { + let result = wait_until("dataset", 30, || Err(anyhow!("could not list datasets"))); + + assert_eq!( + result.unwrap_err().to_string(), + "could not list datasets", + "should surface the underlying error, not a timeout" + ); + } +}