From f250bcfcbfd9e7995ba01eb6ff335c20c7b2c00c Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Tue, 21 Nov 2023 21:56:12 +0100 Subject: [PATCH 1/5] Add a quiet version of query_timeless_components for use by blueprint --- crates/re_arrow_store/src/store_helpers.rs | 81 ++++++++++++++++++++ crates/re_viewer/src/app_blueprint.rs | 2 +- crates/re_viewport/src/space_view.rs | 4 +- crates/re_viewport/src/viewport_blueprint.rs | 8 +- 4 files changed, 88 insertions(+), 7 deletions(-) diff --git a/crates/re_arrow_store/src/store_helpers.rs b/crates/re_arrow_store/src/store_helpers.rs index 61594ac463c1..d84cce331968 100644 --- a/crates/re_arrow_store/src/store_helpers.rs +++ b/crates/re_arrow_store/src/store_helpers.rs @@ -92,6 +92,66 @@ impl DataStore { .map(|c| (row_id, c).into()) } + /// Get the latest value for a given [`re_types_core::Component`] and the associated [`RowId`]. + /// + /// This is a quiet version of [`query_latest_component`] which suppresses errors and sends them + /// to debug instead. + /// + /// This assumes that the row we get from the store only contains a single instance for this + /// component; it will return None and log a debug message otherwise. + /// + /// This should only be used for "mono-components" such as `Transform` and `Tensor`. + /// + /// This is a best-effort helper, it will merely logs debug messages on failure. + pub fn query_latest_component_quiet( + &self, + entity_path: &EntityPath, + query: &LatestAtQuery, + ) -> Option> { + re_tracing::profile_function!(); + + let (row_id, cells) = self.latest_at(query, entity_path, C::name(), &[C::name()])?; + let cell = cells.get(0)?.as_ref()?; + + cell.try_to_native_mono::() + .map_err(|err| { + if let re_log_types::DataCellError::LoggableDeserialize(err) = err { + let bt = err.backtrace().map(|mut bt| { + bt.resolve(); + bt + }); + + let err = Box::new(err) as Box; + if let Some(bt) = bt { + re_log::debug_once!( + "Couldn't deserialize component at {entity_path}#{}: {}\n{:#?}", + C::name(), + re_error::format(&err), + bt, + ); + } else { + re_log::debug_once!( + "Couldn't deserialize component at {entity_path}#{}: {}", + C::name(), + re_error::format(&err) + ); + } + return err; + } + + let err = Box::new(err) as Box; + re_log::debug_once!( + "Couldn't deserialize component at {entity_path}#{}: {}", + C::name(), + re_error::format(&err) + ); + + err + }) + .ok()? + .map(|c| (row_id, c).into()) + } + /// Call [`Self::query_latest_component`] at the given path, walking up the hierarchy until an instance is found. pub fn query_latest_component_at_closest_ancestor( &self, @@ -127,6 +187,27 @@ impl DataStore { let query = LatestAtQuery::latest(Timeline::default()); self.query_latest_component(entity_path, &query) } + + /// Get the latest value for a given [`re_types_core::Component`] and the associated [`RowId`], assuming it is timeless. + /// + /// This is a quiet version of [`query_timeless_component`] which suppresses errors and sends them + /// to debug instead. + /// + /// This assumes that the row we get from the store only contains a single instance for this + /// component; it will log a warning otherwise. + /// + /// This should only be used for "mono-components" such as `Transform` and `Tensor`. + /// + /// This is a best-effort helper, it will merely log debug on failure. + pub fn query_timeless_component_quiet( + &self, + entity_path: &EntityPath, + ) -> Option> { + re_tracing::profile_function!(); + + let query = LatestAtQuery::latest(Timeline::default()); + self.query_latest_component_quiet(entity_path, &query) + } } // --- Write --- diff --git a/crates/re_viewer/src/app_blueprint.rs b/crates/re_viewer/src/app_blueprint.rs index a523b83d9037..6f659dba087c 100644 --- a/crates/re_viewer/src/app_blueprint.rs +++ b/crates/re_viewer/src/app_blueprint.rs @@ -130,6 +130,6 @@ fn load_panel_state(path: &EntityPath, blueprint_db: &re_data_store::StoreDb) -> re_tracing::profile_function!(); blueprint_db .store() - .query_timeless_component::(path) + .query_timeless_component_quiet::(path) .map(|p| p.is_expanded) } diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 8e99851f5457..f3c47a623a2e 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -360,7 +360,7 @@ impl SpaceViewBlueprint { let individual_properties = ctx .blueprint .store() - .query_timeless_component::(&self.entity_path()) + .query_timeless_component_quiet::(&self.entity_path()) .map(|result| result.value.props); let resolved_properties = individual_properties.clone().unwrap_or_else(|| { @@ -401,7 +401,7 @@ impl SpaceViewBlueprint { tree.visit_children_recursively(&mut |path: &EntityPath| { if let Some(props) = blueprint .store() - .query_timeless_component::(path) + .query_timeless_component_quiet::(path) { let overridden_path = EntityPath::from(&path.as_slice()[props_path.len()..path.len()]); diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 32557b55d904..c13f34983ecc 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -334,7 +334,7 @@ pub fn load_space_view_blueprint( let mut space_view = blueprint_db .store() - .query_timeless_component::(path) + .query_timeless_component_quiet::(path) .map(|c| c.value.space_view); // Blueprint data migrations can leave us unable to parse the expected id from the source-data @@ -374,7 +374,7 @@ pub fn load_viewport_blueprint(blueprint_db: &re_data_store::StoreDb) -> Viewpor let auto_space_views = blueprint_db .store() - .query_timeless_component::(&VIEWPORT_PATH.into()) + .query_timeless_component_quiet::(&VIEWPORT_PATH.into()) .map_or_else( || { // Only enable auto-space-views if this is the app-default blueprint @@ -389,13 +389,13 @@ pub fn load_viewport_blueprint(blueprint_db: &re_data_store::StoreDb) -> Viewpor let space_view_maximized = blueprint_db .store() - .query_timeless_component::(&VIEWPORT_PATH.into()) + .query_timeless_component_quiet::(&VIEWPORT_PATH.into()) .map(|space_view| space_view.value) .unwrap_or_default(); let viewport_layout: ViewportLayout = blueprint_db .store() - .query_timeless_component::(&VIEWPORT_PATH.into()) + .query_timeless_component_quiet::(&VIEWPORT_PATH.into()) .map(|space_view| space_view.value) .unwrap_or_default(); From 7537c6d8c3d5db27b548f6e34469dcfb88555613 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Tue, 21 Nov 2023 22:43:43 +0100 Subject: [PATCH 2/5] Add new validation logic when loading persisted blueprints --- crates/re_viewer/src/blueprint_validation.rs | 62 ++++++++++++++++++++ crates/re_viewer/src/lib.rs | 1 + crates/re_viewer/src/store_hub.rs | 6 ++ 3 files changed, 69 insertions(+) create mode 100644 crates/re_viewer/src/blueprint_validation.rs diff --git a/crates/re_viewer/src/blueprint_validation.rs b/crates/re_viewer/src/blueprint_validation.rs new file mode 100644 index 000000000000..7053b7bb4c57 --- /dev/null +++ b/crates/re_viewer/src/blueprint_validation.rs @@ -0,0 +1,62 @@ +use re_arrow_store::LatestAtQuery; +use re_data_store::{EntityPropertiesComponent, StoreDb}; +use re_log_types::Timeline; +use re_types_core::Component; +use re_viewport::{ + blueprint::{AutoSpaceViews, SpaceViewComponent, SpaceViewMaximized, ViewportLayout}, + external::re_space_view::QueryExpressions, +}; + +use crate::blueprint::PanelView; + +fn validate_component(blueprint: &StoreDb) -> bool { + let query = LatestAtQuery::latest(Timeline::default()); + + if let Some(data_type) = blueprint.entity_db().data_store.lookup_datatype(&C::name()) { + if data_type != &C::arrow_datatype() { + // If the schemas don't match, we definitely have a problem + re_log::debug!( + "Unexpected datatype for component {:?}.\nFound: {:?}\nExpected: {:?}", + C::name(), + data_type, + C::arrow_datatype() + ); + return false; + } else { + // Otherwise, our usage of serde-fields means we still might have a problem + // this can go away once we stop using serde-fields. + // Walk the blueprint and see if any cells fail to deserialize for this component type. + for path in blueprint.entity_db().entity_paths() { + if let Some([Some(cell)]) = blueprint + .entity_db() + .data_store + .latest_at(&query, path, C::name(), &[C::name()]) + .map(|(_, cells)| cells) + { + if let Err(err) = cell.try_to_native_mono::() { + re_log::debug!( + "Failed to deserialize component {:?}: {:?}", + C::name(), + err + ); + return false; + } + } + } + } + } + true +} + +/// Because blueprints are both read and written the schema must match what +/// we expect to find or else we will run into all kinds of problems. +pub fn is_valid_blueprint(blueprint: &StoreDb) -> bool { + // TODO(jleibs): Generate this from codegen. + validate_component::(blueprint) + && validate_component::(blueprint) + && validate_component::(blueprint) + && validate_component::(blueprint) + && validate_component::(blueprint) + && validate_component::(blueprint) + && validate_component::(blueprint) +} diff --git a/crates/re_viewer/src/lib.rs b/crates/re_viewer/src/lib.rs index f0ad7dca4aac..4f6f21de1fb0 100644 --- a/crates/re_viewer/src/lib.rs +++ b/crates/re_viewer/src/lib.rs @@ -7,6 +7,7 @@ mod app; mod app_blueprint; mod app_state; mod background_tasks; +mod blueprint_validation; pub mod env_vars; #[cfg(not(target_arch = "wasm32"))] mod loading; diff --git a/crates/re_viewer/src/store_hub.rs b/crates/re_viewer/src/store_hub.rs index 3457abc1b601..b3e95af5bf3a 100644 --- a/crates/re_viewer/src/store_hub.rs +++ b/crates/re_viewer/src/store_hub.rs @@ -319,6 +319,8 @@ impl StoreHub { &mut self, app_id: &ApplicationId, ) -> anyhow::Result<()> { + use crate::blueprint_validation::is_valid_blueprint; + re_tracing::profile_function!(); let blueprint_path = default_blueprint_path(app_id)?; if blueprint_path.exists() { @@ -329,6 +331,10 @@ impl StoreHub { for store in bundle.drain_store_dbs() { if store.store_kind() == StoreKind::Blueprint && store.app_id() == Some(app_id) { + if !is_valid_blueprint(&store) { + re_log::warn!("Blueprint for {app_id} appears invalid - restoring to default. This is expected if you have just upgraded Rerun versions."); + continue; + } // We found the blueprint we were looking for; make it active. // borrow-checker won't let us just call `self.set_blueprint_for_app_id` re_log::debug!( From 79e9cbe20bc5444954b4166b95d0844573bdada1 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Tue, 21 Nov 2023 22:57:47 +0100 Subject: [PATCH 3/5] Remove code duplication --- crates/re_arrow_store/src/store_helpers.rs | 83 ++++++++-------------- crates/re_log/src/lib.rs | 2 +- 2 files changed, 30 insertions(+), 55 deletions(-) diff --git a/crates/re_arrow_store/src/store_helpers.rs b/crates/re_arrow_store/src/store_helpers.rs index d84cce331968..76be013cc9a9 100644 --- a/crates/re_arrow_store/src/store_helpers.rs +++ b/crates/re_arrow_store/src/store_helpers.rs @@ -38,15 +38,16 @@ impl DataStore { /// Get the latest value for a given [`re_types_core::Component`] and the associated [`RowId`]. /// /// This assumes that the row we get from the store only contains a single instance for this - /// component; it will log a warning otherwise. + /// component; it will generate a log message of `level` otherwise. /// /// This should only be used for "mono-components" such as `Transform` and `Tensor`. /// - /// This is a best-effort helper, it will merely log errors on failure. - pub fn query_latest_component( + /// This is a best-effort helper, it will merely log messages on failure. + pub fn query_latest_component_with_log_level( &self, entity_path: &EntityPath, query: &LatestAtQuery, + level: re_log::Level, ) -> Option> { re_tracing::profile_function!(); @@ -63,14 +64,16 @@ impl DataStore { let err = Box::new(err) as Box; if let Some(bt) = bt { - re_log::error_once!( + re_log::log_once!( + level, "Couldn't deserialize component at {entity_path}#{}: {}\n{:#?}", C::name(), re_error::format(&err), bt, ); } else { - re_log::error_once!( + re_log::log_once!( + level, "Couldn't deserialize component at {entity_path}#{}: {}", C::name(), re_error::format(&err) @@ -80,7 +83,8 @@ impl DataStore { } let err = Box::new(err) as Box; - re_log::error_once!( + re_log::log_once!( + level, "Couldn't deserialize component at {entity_path}#{}: {}", C::name(), re_error::format(&err) @@ -94,8 +98,22 @@ impl DataStore { /// Get the latest value for a given [`re_types_core::Component`] and the associated [`RowId`]. /// - /// This is a quiet version of [`query_latest_component`] which suppresses errors and sends them - /// to debug instead. + /// This assumes that the row we get from the store only contains a single instance for this + /// component; it will log a warning otherwise. + /// + /// This should only be used for "mono-components" such as `Transform` and `Tensor`. + /// + /// This is a best-effort helper, it will merely log errors on failure. + #[inline] + pub fn query_latest_component( + &self, + entity_path: &EntityPath, + query: &LatestAtQuery, + ) -> Option> { + self.query_latest_component_with_log_level(entity_path, query, re_log::Level::Warn) + } + + /// Get the latest value for a given [`re_types_core::Component`] and the associated [`RowId`]. /// /// This assumes that the row we get from the store only contains a single instance for this /// component; it will return None and log a debug message otherwise. @@ -103,53 +121,13 @@ impl DataStore { /// This should only be used for "mono-components" such as `Transform` and `Tensor`. /// /// This is a best-effort helper, it will merely logs debug messages on failure. + #[inline] pub fn query_latest_component_quiet( &self, entity_path: &EntityPath, query: &LatestAtQuery, ) -> Option> { - re_tracing::profile_function!(); - - let (row_id, cells) = self.latest_at(query, entity_path, C::name(), &[C::name()])?; - let cell = cells.get(0)?.as_ref()?; - - cell.try_to_native_mono::() - .map_err(|err| { - if let re_log_types::DataCellError::LoggableDeserialize(err) = err { - let bt = err.backtrace().map(|mut bt| { - bt.resolve(); - bt - }); - - let err = Box::new(err) as Box; - if let Some(bt) = bt { - re_log::debug_once!( - "Couldn't deserialize component at {entity_path}#{}: {}\n{:#?}", - C::name(), - re_error::format(&err), - bt, - ); - } else { - re_log::debug_once!( - "Couldn't deserialize component at {entity_path}#{}: {}", - C::name(), - re_error::format(&err) - ); - } - return err; - } - - let err = Box::new(err) as Box; - re_log::debug_once!( - "Couldn't deserialize component at {entity_path}#{}: {}", - C::name(), - re_error::format(&err) - ); - - err - }) - .ok()? - .map(|c| (row_id, c).into()) + self.query_latest_component_with_log_level(entity_path, query, re_log::Level::Debug) } /// Call [`Self::query_latest_component`] at the given path, walking up the hierarchy until an instance is found. @@ -190,11 +168,8 @@ impl DataStore { /// Get the latest value for a given [`re_types_core::Component`] and the associated [`RowId`], assuming it is timeless. /// - /// This is a quiet version of [`query_timeless_component`] which suppresses errors and sends them - /// to debug instead. - /// /// This assumes that the row we get from the store only contains a single instance for this - /// component; it will log a warning otherwise. + /// component; it will return None and log a debug message otherwise. /// /// This should only be used for "mono-components" such as `Transform` and `Tensor`. /// diff --git a/crates/re_log/src/lib.rs b/crates/re_log/src/lib.rs index 6916b295fdef..d54b5e4eed4c 100644 --- a/crates/re_log/src/lib.rs +++ b/crates/re_log/src/lib.rs @@ -30,7 +30,7 @@ pub use tracing::{debug, error, info, trace, warn}; // The `re_log::info_once!(…)` etc are nice helpers, but the `log-once` crate is a bit lacking. // In the future we should implement our own macros to de-duplicate based on the callsite, // similar to how the log console in a browser will automatically suppress duplicates. -pub use log_once::{debug_once, error_once, info_once, trace_once, warn_once}; +pub use log_once::{debug_once, error_once, info_once, log_once, trace_once, warn_once}; pub use { channel_logger::*, From 67fa0b7a01870d3a9a3db6ff4036e257b410ef1d Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Wed, 22 Nov 2023 14:05:35 +0100 Subject: [PATCH 4/5] Update crates/re_viewer/src/blueprint_validation.rs Co-authored-by: Clement Rey --- crates/re_viewer/src/blueprint_validation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_viewer/src/blueprint_validation.rs b/crates/re_viewer/src/blueprint_validation.rs index 7053b7bb4c57..4d031038a0fd 100644 --- a/crates/re_viewer/src/blueprint_validation.rs +++ b/crates/re_viewer/src/blueprint_validation.rs @@ -16,7 +16,7 @@ fn validate_component(blueprint: &StoreDb) -> bool { if data_type != &C::arrow_datatype() { // If the schemas don't match, we definitely have a problem re_log::debug!( - "Unexpected datatype for component {:?}.\nFound: {:?}\nExpected: {:?}", + "Unexpected datatype for component {:?}.\nFound: {:#?}\nExpected: {:#?}", C::name(), data_type, C::arrow_datatype() From 5a85ed646d8a7c585804909613689c461fa0929e Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Wed, 22 Nov 2023 15:24:39 +0100 Subject: [PATCH 5/5] Blueprint validation isn't used on web --- crates/re_viewer/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/re_viewer/src/lib.rs b/crates/re_viewer/src/lib.rs index 4f6f21de1fb0..d6aaed495fd8 100644 --- a/crates/re_viewer/src/lib.rs +++ b/crates/re_viewer/src/lib.rs @@ -7,6 +7,7 @@ mod app; mod app_blueprint; mod app_state; mod background_tasks; +#[cfg(not(target_arch = "wasm32"))] mod blueprint_validation; pub mod env_vars; #[cfg(not(target_arch = "wasm32"))]