Add definitions for remote store gRPC API as well as the logic for bu… #7674

zehiko · 2024-10-10T10:21:02Z

What

In this PR we aim to define initial version of the V0 storage node APIs in the form of grpc protobuf definition. We also want to:

settle on code generation
type conversions (between rerun internal types and grpc types

(currently) Example usage:

use re_storage_types::v0::{
    storage_node_client::StorageNodeClient, RegisterRecordingsRequest
};

...

  let mut client = StorageNodeClient::connect("http://127.0.0.1:51234")
        .await
        .unwrap();

    let resp = client
        .register_recordings(RegisterRecordingsRequest {
            description: "test".to_string(),
            obj_storage:  ObjectStore {
              url: "dna.rd",
              bucket_name: "my_s3_bucket",
            },
            typ: re_storage_types::v0::RecordingType::Rrd.into(),
        })
        .await
        .unwrap()
        .into_inner();

Checklist

I have read and agree to Contributor Guide and the Code of Conduct
I've included a screenshot or gif (if applicable)
I have tested the web demo (if applicable):
- Using examples from latest main build: rerun.io/viewer
- Using full set of examples from nightly build: rerun.io/viewer
The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
If applicable, add a new check to the release checklist!
If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

…ilding rust types from the proto definition We introduce re_remote_store_types with proto definition files for V0 remote store gRPC API. We also introduce re_remote_store_tpyes_builder that heaveily relies on tonic_build to build the actual rust code from protobuf files.

github-actions · 2024-10-10T10:21:41Z

Deployed docs

Commit	Link
`96e6dc6`	https://landing-b82dabj9z-rerun.vercel.app/docs

teh-cmc

Nice! A few last things but we're very close.

teh-cmc · 2024-10-11T07:56:17Z

crates/store/re_ws_comms/src/client.rs

@@ -16,6 +16,7 @@ pub fn viewer_to_server(
        // We set a very high limit, because we should be able to trust the server.
        // See https://github.com/rerun-io/rerun/issues/5268 for more
        max_incoming_frame_size: 2 * gigs,
+        delay_blocking: Duration::from_millis(10),


A comment explaining what this is and why 10ms would be helpful?

teh-cmc · 2024-10-11T07:57:49Z

crates/build/README.md

Wait a minute -- that's the wrong README (both the file and the contents)!

teh-cmc · 2024-10-11T08:00:52Z

crates/build/re_remote_store_types_builder/src/bin/build_re_remote_store_types.rs

+    assert!(
+        workspace_dir.join("CODE_OF_CONDUCT.md").exists(),
+        "failed to find workspace root"
+    );


Don't leave future reader extremely confused 😶

Suggested change

assert!(

workspace_dir.join("CODE_OF_CONDUCT.md").exists(),

"failed to find workspace root"

);

// Check for something that only exists in root:

assert!(

workspace_dir.join("CODE_OF_CONDUCT.md").exists(),

"failed to find workspace root"

);

teh-cmc · 2024-10-11T08:07:42Z

crates/build/re_remote_store_types_builder/src/bin/build_re_remote_store_types.rs

+    let definitions_dir_path = workspace_dir.join(PROTOBUF_DEFINITIONS_DIR_PATH);
+    let rust_generated_output_dir_path = workspace_dir.join(RUST_V0_OUTPUT_DIR_PATH);
+
+    re_log::info!("Running codegen for storage node types");


Suggested change

re_log::info!("Running codegen for storage node types");

re_log::info!(

definitions=?definitions_dir_path,

output=?rust_generated_output_dir_path,

"Running codegen for storage node types",

);

teh-cmc · 2024-10-11T08:10:26Z

crates/store/re_remote_store_types/proto/rerun/v0/common.proto

+message ComponentColumnSelector {
+    EntityPath entity_path = 1;
+    Component component = 2;
+    // TODO do we need join encoding?


Not for now at the very least -- we'll add it when/if we do.

teh-cmc · 2024-10-11T08:16:32Z

crates/store/re_remote_store_types/proto/rerun/v0/remote_store.proto

+    rpc ListRecordings(ListRecordingsRequest) returns (ListRecordingsResponse) {}
+    rpc Query(QueryRequest) returns (stream QueryResponse) {}
+    rpc GetRecordingMetadata(GetRecordingMetadataRequest) returns (GetRecordingMetadataResponse) {}
+    // TODO (zehiko) - should this be singular recording registration? Currently we can have 1 rrd => many recordings


Here and everywhere, this should be TODO(zehiko), not TODO (zehiko).
That's important for all of our TODO-based automated tools to work properly.

The fact that this wasn't caught by the CI is a bug -- I'll make a separate PR for that.

teh-cmc · 2024-10-11T08:17:18Z

crates/store/re_remote_store_types/src/lib.rs

+#![allow(clippy::doc_markdown)]
+#![allow(clippy::derive_partial_eq_without_eq)]
+#![allow(clippy::enum_variant_names)]
+#![allow(clippy::unwrap_used)]
+#![allow(clippy::wildcard_imports)]
+#![allow(clippy::manual_is_variant_and)]


You should be able to apply these only to the mod _v0 below.

teh-cmc · 2024-10-11T08:19:03Z

crates/store/re_remote_store_types/src/lib.rs

+        }
+    }
+
+    impl TryFrom<Query> for re_dataframe::external::re_chunk_store::QueryExpression {


Here and everywhere you shouldn't need re_dataframe::external::re_chunk_store::QueryExpression, simply re_dataframe::QueryExpression.

If that doesn't work then we have a re-export problem, and it's important to be fixed as these are supposed to be sane-to-use public APIs.

teh-cmc · 2024-10-11T08:22:15Z

crates/store/re_remote_store_types/src/lib.rs

+            #[allow(clippy::match_same_arms)]
+            let timeline = match timeline_name.as_str() {
+                "log_time" => Self::new_temporal(timeline_name),
+                "log_tick" => Self::new_sequence(timeline_name),
+                "frame" => Self::new_sequence(timeline_name),
+                "frame_nr" => Self::new_sequence(timeline_name),
+                _ => Self::new_temporal(timeline_name),
+            };


Oof, that's a huge issue.

This needs a TODO(cmc): QueryExpression::filtered_index gotta be a selector.

teh-cmc · 2024-10-11T08:25:46Z

crates/store/re_remote_store_types/src/lib.rs

+        // to chunk store query expression
+        let expected_qe = re_dataframe::external::re_chunk_store::QueryExpression {
+            view_contents: Some(BTreeMap::from([(
+                re_log_types::EntityPath::from("/somepath"),
+                Some(BTreeSet::from([
+                    re_dataframe::external::re_chunk::ComponentName::new("component"),
+                ])),
+            )])),
+            filtered_index: re_log_types::Timeline::new_temporal("log_time"),
+            filtered_index_range: Some(re_dataframe::external::re_chunk_store::IndexRange::new(
+                0, 100,
+            )),
+            filtered_index_values: Some(
+                vec![0, 1, 2]
+                    .into_iter()
+                    .map(re_log_types::TimeInt::new_temporal)
+                    .collect::<BTreeSet<_>>(),
+            ),
+            using_index_values: Some(
+                vec![3, 4, 5]
+                    .into_iter()
+                    .map(re_log_types::TimeInt::new_temporal)
+                    .collect::<BTreeSet<_>>(),
+            ),
+            filtered_point_of_view: Some(
+                re_dataframe::external::re_chunk_store::ComponentColumnSelector {
+                    entity_path: re_log_types::EntityPath::from("/somepath/c"),
+                    component: re_dataframe::external::re_chunk::ComponentName::new("component"),
+                    join_encoding: re_dataframe::external::re_chunk_store::JoinEncoding::default(),
+                },
+            ),
+            sparse_fill_strategy:
+                re_dataframe::external::re_chunk_store::SparseFillStrategy::default(),
+            selection: Some(vec![
+                re_dataframe::external::re_chunk_store::ComponentColumnSelector {
+                    entity_path: re_log_types::EntityPath::from("/somepath/c"),
+                    component: re_dataframe::external::re_chunk::ComponentName::new("component"),
+                    join_encoding: re_dataframe::external::re_chunk_store::JoinEncoding::default(),
+                }
+                .into(),
+            ]),
+        };
+
+        let query_expression: re_dataframe::external::re_chunk_store::QueryExpression =
+            query.try_into().unwrap();


A full roundtrip would be nice:

let query_grpc_before = /* ... */; let query_native = query_grpc_before.try_into(); let query_grpc_after = query_native.into(); assert_eq!(query_grpc_before, query_grpc_after);

at which point you can probably remove the handwritten expectations

zehiko self-assigned this Oct 10, 2024

zehiko added exclude from changelog PRs with this won't show up in CHANGELOG.md ⛃ re_datastore affects the datastore itself labels Oct 10, 2024

zehiko mentioned this pull request Oct 10, 2024

WIP: Initial V0 storage node API grpc spec #7634

Closed

6 tasks

zehiko added 4 commits October 10, 2024 12:30

remove unused deps (thanks machete)

ed3c07a

add a simple unit test for query expression conversion

727dfe7

linter fix

9f7c9f3

use ewebsock 0.7.0

4cf785f

teh-cmc self-requested a review October 11, 2024 07:53

prost_build gotta chill man

96e6dc6

teh-cmc requested changes Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add definitions for remote store gRPC API as well as the logic for bu… #7674

Add definitions for remote store gRPC API as well as the logic for bu… #7674

zehiko commented Oct 10, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading

teh-cmc left a comment

teh-cmc Oct 11, 2024

teh-cmc Oct 11, 2024

teh-cmc Oct 11, 2024

teh-cmc Oct 11, 2024

teh-cmc Oct 11, 2024

teh-cmc Oct 11, 2024

teh-cmc Oct 11, 2024

teh-cmc Oct 11, 2024

teh-cmc Oct 11, 2024

teh-cmc Oct 11, 2024

-    re_log::info!("Running codegen for storage node types");
+    re_log::info!(
+        definitions=?definitions_dir_path,
+        output=?rust_generated_output_dir_path,
+        "Running codegen for storage node types",
+    );

Add definitions for remote store gRPC API as well as the logic for bu… #7674

Are you sure you want to change the base?

Add definitions for remote store gRPC API as well as the logic for bu… #7674

Conversation

zehiko commented Oct 10, 2024 • edited by github-actions bot Loading

What

Checklist

github-actions bot commented Oct 10, 2024 • edited Loading

Deployed docs

teh-cmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zehiko commented Oct 10, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading