-
Notifications
You must be signed in to change notification settings - Fork 283
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
Refactor re_sdk::Session
#1528
Refactor re_sdk::Session
#1528
Conversation
The Python API have different needs from the Rust API
less re-compiling
@@ -0,0 +1,243 @@ | |||
use std::net::SocketAddr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following file is the old session.rs
, with a few small refactorings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅 that was a long one!
Couple of small issues/questions but overall looks like a great improvement.
@@ -25,7 +24,7 @@ py_folders := "examples rerun_py scripts" | |||
py-dev-env: | |||
#!/usr/bin/env bash | |||
echo "Setting up Python virtual environment in venv" | |||
# set -euxo pipefail | |||
set -euxo pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit curious why this was commented out before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm suspecting it was a mistake
@@ -41,9 +41,6 @@ def lint_line(line: str) -> Optional[str]: | |||
if "todo!()" in line: | |||
return 'todo!() should be written as todo!("$details")' | |||
|
|||
if "dbg!(" in line and not line.startswith("//"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason we're removing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is (better) checked by clippy
in clippy.toml
Co-authored-by: Jeremy Leibs <jeremy@rerun.io>
Co-authored-by: Jeremy Leibs <jeremy@rerun.io>
Backstory
Session
was originally designed for internal use by the Python API. It was then reused as the main interface for the Rust API. However, the needs of the Python API and the Rust API are quite different:Session
s, while in Python we currently have one global sessionSesssion
once, while in Python we have methods to change the recording id etc as the session is aliveSession
needs to be ergonomic and user-facing for our Rust API, while in Python is an implementation detailWhat this PR does
This PR does two major things:
PythonSession
for internal use inrerun_py
. It is a slightly simplified version of the oldSession
.Session
is now vastly simpler, with only a few methods. It can be constructed with aSessionBuilder
.This separation allow us to more easily evolve the Python and Rust APIs independently.
The Rust
Session
is now associated with oneLogSink
for the duration of its lifetime. To connect to something else you create a newSession
.Session
is alsoClone
,Send
andSync
so it can easily be used from multiple threads.Why tho?
I want to be able to easily hook up the
tracing
log stream to send its log events to Rerun, and for that I need to get anArc<dyn LogSink>
, which lead me down this rabbit hole. EachSession
needs to start by sending off aBeginRecordingMsg
, and its only after that is sent that one should be able to share aSession
, hence this refactor to a builder-pattern.Checklist