Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API examples overhaul & roundtrip tests #3204

Merged
merged 21 commits into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/reusable_build_and_test_wheels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,9 @@ jobs:
# --release so we can inherit from some of the artifacts that maturin has just built before
# --target x86_64-unknown-linux-gnu because otherwise cargo loses the target cache... even though this is the target anyhow...
# --no-py-build because rerun-sdk is already built and installed
# NOTE: run with --release so we can reuse some of the "Build Wheel"'s job artifacts, hopefully
run: RUST_LOG=debug scripts/ci/run_e2e_roundtrip_tests.py --release --target x86_64-unknown-linux-gnu --no-py-build
run: |
RUST_LOG=debug tests/roundtrips.py --release --target x86_64-unknown-linux-gnu --no-py-build
RUST_LOG=debug docs/code-examples/roundtrips.py --release --target x86_64-unknown-linux-gnu --no-py-build
- name: Cache RRD dataset
if: needs.set-config.outputs.RUN_TESTS == 'true'
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/reusable_checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -337,4 +337,4 @@ jobs:

- name: Build code examples
shell: bash
run: ./tests/cpp/build_all_doc_examples.sh --werror
run: ./docs/code-examples/build_all.sh --werror
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,4 @@ screenshot*.png
web_demo

.nox/
out.rrd
*.rrd
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,4 @@ FetchContent_MakeAvailable(LoguruGitRepo) # defines target 'loguru::loguru'
add_subdirectory(rerun_cpp) # The Rerun C++ SDK library
add_subdirectory(examples/cpp/minimal)
add_subdirectory(tests/cpp)
add_subdirectory(docs/code-examples)
4 changes: 2 additions & 2 deletions crates/re_log_types/src/data_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1145,8 +1145,8 @@ impl DataTable {
c1.datatype() == c2.datatype(),
"Found discrepancy in row #{ri}: cells' datatypes don't match!\n{}",
similar_asserts::SimpleDiff::from_str(
&format!("{:?}", c1.datatype()),
&format!("{:?}", c2.datatype()),
&format!("{:?}:{:?}", c1.component_name(), c1.datatype()),
&format!("{:?}:{:?}", c2.component_name(), c2.datatype()),
"cell1",
"cell2"
)
Expand Down
84 changes: 75 additions & 9 deletions crates/re_sdk/src/recording_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,25 @@ use crate::sink::{LogSink, MemorySinkStorage};

// ---

/// Private environment variable meant for tests.
///
/// When set, all recording streams will write to disk at the path indicated by the env-var rather
/// than doing what they were asked to do (`connect()`, `buffered()`, even `save()`..!).
const ENV_FORCE_SAVE: &str = "_RERUN_TEST_FORCE_SAVE";

/// If private environment variable `_RERUN_TEST_FORCE_SAVE` is set, this will return a `FileSink`
/// pointing at the path that the env-var is set to.
///
/// Useful to save the result of any Rerun snippet to an rrd file, whether it supports it or not.
fn forced_sink() -> RecordingStreamResult<Option<Box<dyn LogSink>>> {
Ok(if let Ok(path) = std::env::var(ENV_FORCE_SAVE) {
re_log::warn!(?path, "forcing FileSink");
Some(Box::new(crate::sink::FileSink::new(path)?) as Box<dyn LogSink>)
} else {
None
})
}

/// Errors that can occur when creating/manipulating a [`RecordingStream`].
#[derive(thiserror::Error, Debug)]
pub enum RecordingStreamError {
Expand Down Expand Up @@ -184,7 +203,11 @@ impl RecordingStreamBuilder {
RecordingStream::new(
store_info,
batcher_config,
Box::new(crate::log_sink::BufferedSink::new()),
if let Some(sink) = forced_sink()? {
sink
} else {
Box::new(crate::log_sink::BufferedSink::new())
},
)
} else {
re_log::debug!("Rerun disabled - call to buffered() ignored");
Expand Down Expand Up @@ -216,7 +239,18 @@ impl RecordingStreamBuilder {

let (enabled, store_info, batcher_config) = self.into_args();
if enabled {
RecordingStream::new(store_info, batcher_config, Box::new(sink)).map(|rec| {
RecordingStream::new(
store_info,
batcher_config,
// NOTE: We still have to return a `MemorySinkStorage` that will never get written
// to.. beware!
if let Some(sink) = forced_sink()? {
sink
} else {
Box::new(sink)
},
)
.map(|rec| {
storage.rec = Some(rec.clone());
(rec, storage)
})
Expand Down Expand Up @@ -250,7 +284,11 @@ impl RecordingStreamBuilder {
RecordingStream::new(
store_info,
batcher_config,
Box::new(crate::log_sink::TcpSink::new(addr, flush_timeout)),
if let Some(sink) = forced_sink()? {
sink
} else {
Box::new(crate::log_sink::TcpSink::new(addr, flush_timeout))
},
)
} else {
re_log::debug!("Rerun disabled - call to connect() ignored");
Expand Down Expand Up @@ -278,7 +316,11 @@ impl RecordingStreamBuilder {
RecordingStream::new(
store_info,
batcher_config,
Box::new(crate::sink::FileSink::new(path)?),
if let Some(sink) = forced_sink()? {
sink
} else {
Box::new(crate::sink::FileSink::new(path)?)
},
)
} else {
re_log::debug!("Rerun disabled - call to save() ignored");
Expand Down Expand Up @@ -324,8 +366,12 @@ impl RecordingStreamBuilder {
) -> RecordingStreamResult<RecordingStream> {
let (enabled, store_info, batcher_config) = self.into_args();
if enabled {
let sink = crate::web_viewer::new_sink(open_browser, bind_ip, web_port, ws_port)
.map_err(RecordingStreamError::WebSink)?;
let sink = if let Some(sink) = forced_sink()? {
sink
} else {
crate::web_viewer::new_sink(open_browser, bind_ip, web_port, ws_port)
.map_err(RecordingStreamError::WebSink)?
};
RecordingStream::new(store_info, batcher_config, sink)
} else {
re_log::debug!("Rerun disabled - call to serve() ignored");
Expand Down Expand Up @@ -536,6 +582,7 @@ impl RecordingStream {
batcher_config: DataTableBatcherConfig,
sink: Box<dyn LogSink>,
) -> RecordingStreamResult<Self> {
let sink = forced_sink()?.unwrap_or(sink);
RecordingStreamInner::new(info, batcher_config, sink).map(|inner| Self {
inner: Arc::new(Some(inner)),
})
Expand Down Expand Up @@ -1097,7 +1144,12 @@ impl RecordingStream {
/// terms of data durability and ordering.
/// See [`Self::set_sink`] for more information.
pub fn connect(&self, addr: std::net::SocketAddr, flush_timeout: Option<std::time::Duration>) {
self.set_sink(Box::new(crate::log_sink::TcpSink::new(addr, flush_timeout)));
// NOTE: `forced_sink` is only used for tests, it's ok to unwrap.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

Really all of our sink APIs should uniformly return a RecordingStreamResult. Having connect actually fail if it can't talk to the server with an option for our current "keep retrying in a background thread" behavior would be a more predictable user experience.

self.set_sink(if let Some(sink) = forced_sink().unwrap() {
sink
} else {
Box::new(crate::log_sink::TcpSink::new(addr, flush_timeout))
});
}

/// Swaps the underlying sink for a [`crate::sink::MemorySink`] sink and returns the associated
Expand All @@ -1109,7 +1161,14 @@ impl RecordingStream {
pub fn memory(&self) -> MemorySinkStorage {
let sink = crate::sink::MemorySink::default();
let buffer = sink.buffer();
self.set_sink(Box::new(sink));

teh-cmc marked this conversation as resolved.
Show resolved Hide resolved
// NOTE: `forced_sink` is only used for tests, it's ok to unwrap.
self.set_sink(if let Some(sink) = forced_sink().unwrap() {
sink
} else {
Box::new(sink)
});

buffer
}

Expand All @@ -1123,7 +1182,14 @@ impl RecordingStream {
path: impl Into<std::path::PathBuf>,
) -> Result<(), crate::sink::FileSinkError> {
let sink = crate::sink::FileSink::new(path)?;
self.set_sink(Box::new(sink));

// NOTE: `forced_sink` is only used for tests, it's ok to unwrap.
self.set_sink(if let Some(sink) = forced_sink().unwrap() {
sink
} else {
Box::new(sink)
});

Ok(())
}

Expand Down
2 changes: 1 addition & 1 deletion crates/re_types/definitions/rerun/archetypes/image.fbs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ namespace rerun.archetypes;
/// \py -------
/// \py
/// \py ```python
/// \py \include:../../../../../docs/code-examples/image_simple_v2.py
/// \py \include:../../../../../docs/code-examples/image_simple.py
/// \py ```
///
/// \rs ## Example
Expand Down
2 changes: 1 addition & 1 deletion crates/re_types/source_hash.txt

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/re_types/src/archetypes/annotation_context.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 13 additions & 5 deletions crates/re_types/src/archetypes/arrows3d.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/re_types/src/archetypes/image.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/re_types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ pub use self::arrow_string::ArrowString;

pub mod external {
pub use arrow2;
pub use uuid;
}

// TODO(jleibs): Should all of this go into `tensor_data_ext`? Don't have a good way to export
Expand Down
24 changes: 24 additions & 0 deletions docs/code-examples/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
cmake_minimum_required(VERSION 3.16)

# Setup builds for examples
file(GLOB sources_list true ${CMAKE_CURRENT_SOURCE_DIR}/*.cpp)

add_custom_target(doc_examples)

foreach(SOURCE_PATH ${sources_list})
get_filename_component(SOURCE_NAME ${SOURCE_PATH} NAME_WLE)

if(${SOURCE_NAME} STREQUAL "CMakeFiles")
CONTINUE()
endif()

set(EXAMPLE_TARGET doc_example_${SOURCE_NAME})

add_executable(${EXAMPLE_TARGET} ${SOURCE_PATH})

set_default_warning_settings(rerun_example)
target_link_libraries(${EXAMPLE_TARGET} PRIVATE rerun_sdk)

add_dependencies(doc_examples ${EXAMPLE_TARGET})
endforeach()

25 changes: 25 additions & 0 deletions docs/code-examples/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# API Examples

These examples showcase common usage of each individual Rerun `Archetype`s.
teh-cmc marked this conversation as resolved.
Show resolved Hide resolved

Most of these example are automatically used as docstrings for the `Archetype` APIs in their respective SDKs.
teh-cmc marked this conversation as resolved.
Show resolved Hide resolved

## Usage

You can run each example individually using the following:

- **Python**: `python <example_name>.py`, e.g. `python point3d_random.py`.
- **Rust**: `cargo r -p code_examples --bin <example_name`, e.g.
teh-cmc marked this conversation as resolved.
Show resolved Hide resolved
`cargo r -p code_examples --bin point3d_random`.
- **C++**:
- `./docs/code-examples/build_all.sh` to compile all examples
- start a Rerun Viewer listening on the default port: `rerun`
- `./build/docs/code-examples/doc_example_<example_name>` to run, e.g. `./build/docs/code-examples/doc_example_point3d_random`
teh-cmc marked this conversation as resolved.
Show resolved Hide resolved

teh-cmc marked this conversation as resolved.
Show resolved Hide resolved
## Roundtrips

All API examples support cross-language roundtrip tests.

These tests check that A) all of our SDKs yield the exact same data when used the same way and B) act as regression tests, relying on the fact that it is extremely unlikely that all supported languages break in the exact same way at the exact same time.

See `./docs/code-examples/roundtrips.py --help`.
8 changes: 4 additions & 4 deletions docs/code-examples/annotation_context_arrows.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
namespace rr = rerun;

int main() {
auto rr_stream = rr::RecordingStream("rerun_example_annotation_context_rects");
rr_stream.connect("127.0.0.1:9876").throw_on_failure();
auto rec = rr::RecordingStream("rerun_example_annotation_context_rects");
rec.connect("127.0.0.1:9876").throw_on_failure();

// Log an annotation context to assign a label and color to each class
rr_stream.log(
rec.log(
"/",
rr::AnnotationContext({
rr::datatypes::AnnotationInfo(1, "red", rr::datatypes::Color(255, 0, 0)),
Expand All @@ -18,7 +18,7 @@ int main() {
);

// Log a batch of 2 arrows with different `class_ids`
rr_stream.log(
rec.log(
"arrows",
rr::Arrows3D({{1.0f, 0.0f, 0.0f}, {0.0f, 1.0f, 0.0f}}).with_class_ids({1, 2})
);
Expand Down
Loading