Skip to content

Migrate SDK comms to gRPC #8838

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

Merged
merged 96 commits into from
Feb 10, 2025
Merged

Migrate SDK comms to gRPC #8838

merged 96 commits into from
Feb 10, 2025

Conversation

jprochazk
Copy link
Member

@jprochazk jprochazk commented Jan 28, 2025

Related

What

The goal of this PR is to completely delete re_sdk_comms and re_ws_comms. The prerequisite to this is to first migrate all of their usages over to our temporary gRPC server ("message proxy").

Step-by-step:

  • Expose gRPC variants of spawn/connect from all SDKs
  • Start a gRPC server instead of TCP server on native and connect to it by default
  • Start a gRPC server instead of the re_ws_comms server and connect to it by default
  • Remove all remaining usage of re_sdk_comms and re_ws_comms
  • Delete re_sdk_comms and re_ws_comms

I don't really know how to split this up, to be honest! There are tons of subtle but very strong dependencies between different parts of our SDKs, the Viewer, and the "server". Breaking it apart seems too difficult, and it'd also temporarily make some parts of rerun unusable, and thus untestable. So instead I'll be trying to keep individual commits somewhat sensible and reviewable.

Testing checklist

  • SDK spawn
    • C++
    • Rust
    • Python
  • SDK serve
    • C++
    • Rust
    • Python
  • rerun (default args) + SDK connect
    • C++
    • Rust
    • Python
  • rerun --web-viewer + SDK connect
    • C++
    • Rust
    • Python
  • Custom examples

Copy link

github-actions bot commented Jan 28, 2025

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link Manifest
3a4bdcc https://rerun.io/viewer/pr/8838 +nightly +main

Note: This comment is updated whenever you push a commit.

@emilk
Copy link
Member

emilk commented Feb 10, 2025

We should apply this fix too:

@jprochazk
Copy link
Member Author

jprochazk commented Feb 10, 2025

We should apply this fix too:

I'm not sure if it applies here. I explicitly added BlueprintActivationCommand to the list of things that get GC'd, so we never run into the scenario where a Viewer tries to activate a blueprint for which it has no data. The activation command is ordered after all its data, so it is safe to garbage collect.

The question is whether or not we want to garbage collect blueprints in the first place. It's reasonable to say that we don't, because it's not a ton of data compared to an actual recording.

Previously, all blueprint data, including activation commands,
would be garbage collected. It doesn't make much sense, because
blueprints use very little memory compared to the rest of a
typical recording.
@jprochazk
Copy link
Member Author

I ended up doing just that, so we no longer GC blueprints at all

@jleibs jleibs assigned jleibs and unassigned jleibs Feb 10, 2025
This way we can individually evolve those types.
Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

Generally looks good. I'm sure there are some edge-cases that are only going to turn up in testing.

The biggest potential API-regression I see is the loss of flush_timeout behavior -- it's not totally clear to me under what circumstances data potentially gets dropped if the connection between the client and server is broken before we call flush.

@jprochazk
Copy link
Member Author

One last check before merge

@rerun-bot full-check

Copy link

@jprochazk jprochazk merged commit 67e110b into main Feb 10, 2025
41 checks passed
@jprochazk jprochazk deleted the jan/migrate-grpc branch February 10, 2025 22:32
@emilk emilk mentioned this pull request Apr 11, 2025
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌊 C++ API C/C++ API specific include in changelog 🐍 Python API Python logging API 🦀 Rust API Rust logging API 🕸️ web regarding running the viewer in a browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate all TCP/WS comms to gRPC
7 participants