From 14ca2ee5fa51a51e5ea760c41c4dcd1e39b36943 Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Mon, 4 Mar 2024 12:07:00 +0100 Subject: [PATCH 1/7] stateful file logging APIs, matching standard logging APIs --- crates/re_sdk/src/recording_stream.rs | 49 +++++++++++++++++++++++---- crates/rerun_c/src/lib.rs | 28 +++------------ 2 files changed, 46 insertions(+), 31 deletions(-) diff --git a/crates/re_sdk/src/recording_stream.rs b/crates/re_sdk/src/recording_stream.rs index 89880630d7d8..7e4cc2d22035 100644 --- a/crates/re_sdk/src/recording_stream.rs +++ b/crates/re_sdk/src/recording_stream.rs @@ -1095,10 +1095,11 @@ impl RecordingStream { #[cfg(feature = "data_loaders")] pub fn log_file_from_path( &self, - settings: &re_data_source::DataLoaderSettings, filepath: impl AsRef, + entity_path_prefix: Option, + timeless: bool, ) -> RecordingStreamResult<()> { - self.log_file(settings, filepath, None) + self.log_file(filepath, None, entity_path_prefix, timeless) } /// Logs the given `contents` using all [`re_data_source::DataLoader`]s available. @@ -1112,20 +1113,27 @@ impl RecordingStream { #[cfg(feature = "data_loaders")] pub fn log_file_from_contents( &self, - settings: &re_data_source::DataLoaderSettings, filepath: impl AsRef, contents: std::borrow::Cow<'_, [u8]>, + entity_path_prefix: Option, + timeless: bool, ) -> RecordingStreamResult<()> { - self.log_file(settings, filepath, Some(contents)) + self.log_file(filepath, Some(contents), entity_path_prefix, timeless) } #[cfg(feature = "data_loaders")] fn log_file( &self, - settings: &re_data_source::DataLoaderSettings, filepath: impl AsRef, contents: Option>, + entity_path_prefix: Option, + timeless: bool, ) -> RecordingStreamResult<()> { + let Some(store_info) = self.store_info().clone() else { + re_log::warn!("Ignored call to log_file() because RecordingStream has not been properly initialized"); + return Ok(()); + }; + let filepath = filepath.as_ref(); let has_contents = contents.is_some(); @@ -1134,16 +1142,43 @@ impl RecordingStream { re_smart_channel::SmartChannelSource::File(filepath.into()), ); + let settings = crate::DataLoaderSettings { + store_id: store_info.store_id, + opened_store_id: None, + entity_path_prefix, + timepoint: (!timeless).then(|| { + self.with(|inner| { + // Get the current time on all timelines, for the current recording, on the current + // thread… + let mut now = self.now(); + + // …and then also inject the current recording tick into it. + let tick = inner + .tick + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + now.insert(Timeline::log_tick(), tick.into()); + + now + }) + .unwrap_or_default() + }), // timepoint: self.time, + }; + if let Some(contents) = contents { re_data_source::load_from_file_contents( - settings, + &settings, re_log_types::FileSource::Sdk, filepath, contents, &tx, )?; } else { - re_data_source::load_from_path(settings, re_log_types::FileSource::Sdk, filepath, &tx)?; + re_data_source::load_from_path( + &settings, + re_log_types::FileSource::Sdk, + filepath, + &tx, + )?; } drop(tx); diff --git a/crates/rerun_c/src/lib.rs b/crates/rerun_c/src/lib.rs index aff766b923f9..9e018e93702e 100644 --- a/crates/rerun_c/src/lib.rs +++ b/crates/rerun_c/src/lib.rs @@ -734,20 +734,10 @@ fn rr_log_file_from_path_impl( ) -> Result<(), CError> { let stream = recording_stream(stream)?; - let recording_id = stream - .store_info() - .ok_or_else(|| { - CError::new( - CErrorCode::RecordingStreamRuntimeFailure, - &format!("Couldn't load file {filepath:?}: no recording available"), - ) - })? - .store_id; - let settings = re_sdk::DataLoaderSettings::recommended(recording_id); - let filepath = filepath.as_str("filepath")?; stream - .log_file_from_path(&settings, filepath) + // TODO(cmc): expose settings + .log_file_from_path(filepath, None, true) .map_err(|err| { CError::new( CErrorCode::RecordingStreamRuntimeFailure, @@ -779,22 +769,12 @@ fn rr_log_file_from_contents_impl( ) -> Result<(), CError> { let stream = recording_stream(stream)?; - let recording_id = stream - .store_info() - .ok_or_else(|| { - CError::new( - CErrorCode::RecordingStreamRuntimeFailure, - &format!("Couldn't load file {filepath:?}: no recording available"), - ) - })? - .store_id; - let settings = re_sdk::DataLoaderSettings::recommended(recording_id); - let filepath = filepath.as_str("filepath")?; let contents = contents.as_bytes("contents")?; stream - .log_file_from_contents(&settings, filepath, std::borrow::Cow::Borrowed(contents)) + // TODO(cmc): expose settings + .log_file_from_contents(filepath, std::borrow::Cow::Borrowed(contents), None, true) .map_err(|err| { CError::new( CErrorCode::RecordingStreamRuntimeFailure, From 93a31e23022166daf1274136684d1b95747d2d01 Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Mon, 4 Mar 2024 12:07:29 +0100 Subject: [PATCH 2/7] update rust example --- examples/rust/log_file/src/main.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/examples/rust/log_file/src/main.rs b/examples/rust/log_file/src/main.rs index e7d84bf74b7c..ef183a228826 100644 --- a/examples/rust/log_file/src/main.rs +++ b/examples/rust/log_file/src/main.rs @@ -39,20 +39,23 @@ fn run(rec: &rerun::RecordingStream, args: &Args) -> anyhow::Result<()> { let mut settings = rerun::DataLoaderSettings::recommended(rec.store_info().unwrap().store_id); settings.entity_path_prefix = Some("log_file_example".into()); + let prefix = Some("log_file_example".into()); + for filepath in &args.filepaths { let filepath = filepath.as_path(); if !args.from_contents { // Either log the file using its path… - rec.log_file_from_path(&settings, filepath)?; + rec.log_file_from_path(filepath, prefix.clone(), true /* timeless */)?; } else { // …or using its contents if you already have them loaded for some reason. if filepath.is_file() { let contents = std::fs::read(filepath)?; rec.log_file_from_contents( - &settings, filepath, std::borrow::Cow::Borrowed(&contents), + prefix.clone(), + true, /* timeless */ )?; } } From 0811333008b1e6b64823afc91421e003e5006825 Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Mon, 4 Mar 2024 12:07:38 +0100 Subject: [PATCH 3/7] update python bindings and examples --- examples/python/external_data_loader/main.py | 2 +- rerun_py/rerun_sdk/rerun/_log.py | 16 ++---- rerun_py/src/python_bridge.rs | 54 ++++++-------------- 3 files changed, 19 insertions(+), 53 deletions(-) diff --git a/examples/python/external_data_loader/main.py b/examples/python/external_data_loader/main.py index e897ac7c81d3..b3a3237cf134 100755 --- a/examples/python/external_data_loader/main.py +++ b/examples/python/external_data_loader/main.py @@ -85,7 +85,7 @@ def set_time_from_args() -> None: timeline_name, time = parts rr.set_time_seconds(timeline_name, float(time)) - for time_str in args.time: + for time_str in args.sequence: parts = time_str.split("=") if len(parts) != 2: continue diff --git a/rerun_py/rerun_sdk/rerun/_log.py b/rerun_py/rerun_sdk/rerun/_log.py index ef9256418c5e..c0d185c59c47 100644 --- a/rerun_py/rerun_sdk/rerun/_log.py +++ b/rerun_py/rerun_sdk/rerun/_log.py @@ -288,9 +288,8 @@ def log_components( def log_file_from_path( file_path: str | Path, *, - recording_id: str | None = None, entity_path_prefix: str | None = None, - timeless: bool | None = None, + timeless: bool = False, recording: RecordingStream | None = None, ) -> None: r""" @@ -308,14 +307,11 @@ def log_file_from_path( file_path: Path to the file to be logged. - recording_id: - The recommended `RecordingId` to log the data to. - entity_path_prefix: What should the logged entity paths be prefixed with? timeless: - Should the logged data be timeless? + Should the logged data be timeless? (default: False) recording: Specifies the [`rerun.RecordingStream`][] to use. If left unspecified, @@ -326,7 +322,6 @@ def log_file_from_path( bindings.log_file_from_path( Path(file_path), - recording_id=recording_id, entity_path_prefix=entity_path_prefix, timeless=timeless, recording=recording, @@ -339,7 +334,6 @@ def log_file_from_contents( file_path: str | Path, file_contents: bytes, *, - recording_id: str | None = None, entity_path_prefix: str | None = None, timeless: bool | None = None, recording: RecordingStream | None = None, @@ -362,14 +356,11 @@ def log_file_from_contents( file_contents: Contents to be logged. - recording_id: - The recommended `RecordingId` to log the data to. - entity_path_prefix: What should the logged entity paths be prefixed with? timeless: - Should the logged data be timeless? + Should the logged data be timeless? (default: False) recording: Specifies the [`rerun.RecordingStream`][] to use. If left unspecified, @@ -381,7 +372,6 @@ def log_file_from_contents( bindings.log_file_from_contents( Path(file_path), file_contents, - recording_id=recording_id, entity_path_prefix=entity_path_prefix, timeless=timeless, recording=recording, diff --git a/rerun_py/src/python_bridge.rs b/rerun_py/src/python_bridge.rs index d8136d8113c1..537acb11222b 100644 --- a/rerun_py/src/python_bridge.rs +++ b/rerun_py/src/python_bridge.rs @@ -967,53 +967,40 @@ fn log_arrow_msg( #[pyfunction] #[pyo3(signature = ( file_path, - recording_id = None, entity_path_prefix = None, - timeless = None, - recording=None, + timeless = false, + recording = None, ))] fn log_file_from_path( py: Python<'_>, file_path: std::path::PathBuf, - recording_id: Option, entity_path_prefix: Option, - timeless: Option, + timeless: bool, recording: Option<&PyRecordingStream>, ) -> PyResult<()> { - log_file( - py, - file_path, - None, - recording_id, - entity_path_prefix, - timeless, - recording, - ) + log_file(py, file_path, None, entity_path_prefix, timeless, recording) } #[pyfunction] #[pyo3(signature = ( file_path, file_contents, - recording_id = None, entity_path_prefix = None, - timeless = None, - recording=None, + timeless = false, + recording = None, ))] fn log_file_from_contents( py: Python<'_>, file_path: std::path::PathBuf, file_contents: &[u8], - recording_id: Option, entity_path_prefix: Option, - timeless: Option, + timeless: bool, recording: Option<&PyRecordingStream>, ) -> PyResult<()> { log_file( py, file_path, Some(file_contents), - recording_id, entity_path_prefix, timeless, recording, @@ -1024,37 +1011,26 @@ fn log_file( py: Python<'_>, file_path: std::path::PathBuf, file_contents: Option<&[u8]>, - recording_id: Option, entity_path_prefix: Option, - timeless: Option, + timeless: bool, recording: Option<&PyRecordingStream>, ) -> PyResult<()> { let Some(recording) = get_data_recording(recording) else { return Ok(()); }; - let Some(recording_id) = recording - .store_info() - .map(|info| info.store_id.clone()) - .or(recording_id.map(|id| StoreId::from_string(StoreKind::Recording, id))) - else { - return Ok(()); - }; - - let settings = rerun::DataLoaderSettings { - store_id: recording_id, - opened_store_id: None, - entity_path_prefix: entity_path_prefix.map(Into::into), - timepoint: timeless.unwrap_or(false).then(TimePoint::timeless), - }; - if let Some(contents) = file_contents { recording - .log_file_from_contents(&settings, file_path, std::borrow::Cow::Borrowed(contents)) + .log_file_from_contents( + file_path, + std::borrow::Cow::Borrowed(contents), + entity_path_prefix.map(Into::into), + timeless, + ) .map_err(|err| PyRuntimeError::new_err(err.to_string()))?; } else { recording - .log_file_from_path(&settings, file_path) + .log_file_from_path(file_path, entity_path_prefix.map(Into::into), timeless) .map_err(|err| PyRuntimeError::new_err(err.to_string()))?; } From 98f9474e400527862f27ff42007a4aa6d42ffce0 Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Mon, 4 Mar 2024 12:26:29 +0100 Subject: [PATCH 4/7] inherit application_id too --- crates/re_data_source/src/data_loader/mod.rs | 29 ++++++++++++++++++-- crates/re_sdk/src/recording_stream.rs | 2 ++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/crates/re_data_source/src/data_loader/mod.rs b/crates/re_data_source/src/data_loader/mod.rs index 1e090cb23eac..8a1f327e704a 100644 --- a/crates/re_data_source/src/data_loader/mod.rs +++ b/crates/re_data_source/src/data_loader/mod.rs @@ -11,6 +11,8 @@ use re_log_types::{ArrowMsg, DataRow, EntityPath, LogMsg, TimePoint}; /// The loader is free to ignore some or all of these. /// /// External [`DataLoader`]s will be passed the following CLI parameters: +/// * `--application-id ` +/// * `--opened-application-id ` (if set) /// * `--recording-id ` /// * `--opened-recording-id ` (if set) /// * `--entity-path-prefix ` (if set) @@ -19,13 +21,21 @@ use re_log_types::{ArrowMsg, DataRow, EntityPath, LogMsg, TimePoint}; /// * `--sequence = = ...` (if `timepoint` contains sequence data) #[derive(Debug, Clone)] pub struct DataLoaderSettings { - /// The recommended [`re_log_types::StoreId`] to log the data to, based on the surrounding context. - pub store_id: re_log_types::StoreId, + /// The recommended [`re_log_types::ApplicationId`] to log the data to, based on the surrounding context. + pub application_id: Option, - /// The [`re_log_types::StoreId`] that is currently opened in the viewer, if any. + /// The [`re_log_types::ApplicationId`] that is currently opened in the viewer, if any. + // + // TODO(#5350): actually support this + pub opened_application_id: Option, + + /// The recommended [`re_log_types::StoreId`] to log the data to, based on the surrounding context. /// /// Log data to this recording if you want it to appear in a new recording shared by all /// data-loaders for the current loading session. + pub store_id: re_log_types::StoreId, + + /// The [`re_log_types::StoreId`] that is currently opened in the viewer, if any. // // TODO(#5350): actually support this pub opened_store_id: Option, @@ -41,6 +51,8 @@ impl DataLoaderSettings { #[inline] pub fn recommended(store_id: impl Into) -> Self { Self { + application_id: Default::default(), + opened_application_id: Default::default(), store_id: store_id.into(), opened_store_id: Default::default(), entity_path_prefix: Default::default(), @@ -51,6 +63,8 @@ impl DataLoaderSettings { /// Generates CLI flags from these settings, for external data loaders. pub fn to_cli_args(&self) -> Vec { let Self { + application_id, + opened_application_id, store_id, opened_store_id, entity_path_prefix, @@ -59,8 +73,17 @@ impl DataLoaderSettings { let mut args = Vec::new(); + if let Some(application_id) = application_id { + args.extend(["--application-id".to_owned(), format!("{application_id}")]); + } args.extend(["--recording-id".to_owned(), format!("{store_id}")]); + if let Some(opened_application_id) = opened_application_id { + args.extend([ + "--opened-application-id".to_owned(), + format!("{opened_application_id}"), + ]); + } if let Some(opened_store_id) = opened_store_id { args.extend([ "--opened-recording-id".to_owned(), diff --git a/crates/re_sdk/src/recording_stream.rs b/crates/re_sdk/src/recording_stream.rs index 7e4cc2d22035..04a62ec8b988 100644 --- a/crates/re_sdk/src/recording_stream.rs +++ b/crates/re_sdk/src/recording_stream.rs @@ -1143,6 +1143,8 @@ impl RecordingStream { ); let settings = crate::DataLoaderSettings { + application_id: Some(store_info.application_id.clone()), + opened_application_id: None, store_id: store_info.store_id, opened_store_id: None, entity_path_prefix, From 64110ac550a01ddbb166daa3cb4709bff848d8f9 Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Mon, 4 Mar 2024 12:26:39 +0100 Subject: [PATCH 5/7] update rust example to handle app id --- examples/rust/external_data_loader/src/main.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/examples/rust/external_data_loader/src/main.rs b/examples/rust/external_data_loader/src/main.rs index fbda74d64d54..e85fb54e7b82 100644 --- a/examples/rust/external_data_loader/src/main.rs +++ b/examples/rust/external_data_loader/src/main.rs @@ -29,6 +29,10 @@ struct Args { #[argh(positional)] filepath: std::path::PathBuf, + /// optional recommended ID for the application + #[argh(option)] + application_id: Option, + /// optional recommended ID for the recording #[argh(option)] recording_id: Option, @@ -74,7 +78,11 @@ fn main() -> anyhow::Result<()> { let text = format!("## Some Rust code\n```rust\n{body}\n```\n"); let rec = { - let mut rec = rerun::RecordingStreamBuilder::new("rerun_example_external_data_loader"); + let mut rec = rerun::RecordingStreamBuilder::new( + args.application_id + .as_deref() + .unwrap_or("rerun_example_external_data_loader"), + ); if let Some(recording_id) = args.recording_id.as_ref() { rec = rec.recording_id(recording_id); }; From 0ef2fa13fe3239f6f5cbdada04d0de47eb4529f2 Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Mon, 4 Mar 2024 12:26:44 +0100 Subject: [PATCH 6/7] update py example to handle app id --- examples/python/external_data_loader/main.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/examples/python/external_data_loader/main.py b/examples/python/external_data_loader/main.py index b3a3237cf134..3ae4e381ed05 100755 --- a/examples/python/external_data_loader/main.py +++ b/examples/python/external_data_loader/main.py @@ -31,6 +31,7 @@ """ ) parser.add_argument("filepath", type=str) +parser.add_argument("--application-id", type=str, help="optional recommended ID for the application") parser.add_argument("--recording-id", type=str, help="optional recommended ID for the recording") parser.add_argument("--entity-path-prefix", type=str, help="optional prefix for all entity paths") parser.add_argument( @@ -59,7 +60,10 @@ def main() -> None: if not is_file or not is_python_file: exit(rr.EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE) - rr.init("rerun_example_external_data_loader", recording_id=args.recording_id) + app_id = "rerun_example_external_data_loader" + if args.application_id is not None: + app_id = args.application_id + rr.init(app_id, recording_id=args.recording_id) # The most important part of this: log to standard output so the Rerun Viewer can ingest it! rr.stdout() From c8706c4a018af417c2e420b6fa233eb2f5c81cd5 Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Mon, 4 Mar 2024 17:41:31 +0100 Subject: [PATCH 7/7] Update crates/re_data_source/src/data_loader/mod.rs Co-authored-by: Andreas Reich --- crates/re_data_source/src/data_loader/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_data_source/src/data_loader/mod.rs b/crates/re_data_source/src/data_loader/mod.rs index 8a1f327e704a..24d2c74bb711 100644 --- a/crates/re_data_source/src/data_loader/mod.rs +++ b/crates/re_data_source/src/data_loader/mod.rs @@ -11,7 +11,7 @@ use re_log_types::{ArrowMsg, DataRow, EntityPath, LogMsg, TimePoint}; /// The loader is free to ignore some or all of these. /// /// External [`DataLoader`]s will be passed the following CLI parameters: -/// * `--application-id ` +/// * `--application-id ` /// * `--opened-application-id ` (if set) /// * `--recording-id ` /// * `--opened-recording-id ` (if set)