Add send_recording python api
#9148
Conversation
oxkitsune
added
🐍 Python API
oxkitsune
changed the title
send_recording python api
|
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
There was a problem hiding this comment.
Independent on whether we want to solve this differently in the future, the APIs are very valuable to have I believe.
There's some todos left though for documenting this out etc.
Also, I don't think we should close the linked issue until we implement this in all SDKs (right now the PR auto closes #2044)
rerun_py/rerun_sdk/rerun/sinks.py
Outdated
| @@ -472,6 +473,31 @@ def send_blueprint( | |||
| ) | |||
|
|
|||
|
|
|||
| def send_recording(recording: Recording, stream: RecordingStream | None = None) -> None: | |||
There was a problem hiding this comment.
Needs to be mentioned in gen_common_index.py
There was a problem hiding this comment.
Also, we need a snippet using this.
Ideally we'd add it to all languages if only to have the snippet comparision test do something - I figure at least for Rust we could whip up something?
There was a problem hiding this comment.
I've added snippets for both rust/py 👍
rerun_py/rerun_sdk/rerun/sinks.py
Outdated
| @@ -472,6 +473,31 @@ def send_blueprint( | |||
| ) | |||
|
|
|||
|
|
|||
| def send_recording(recording: Recording, stream: RecordingStream | None = None) -> None: | |||
There was a problem hiding this comment.
Do we need this Python function at all? Can this logic be done in the Rust implementation instead? @jleibs has recently implemented a bunch of APIs for the Python SDK that don't have a single line of Python in them (AFAIK?), and I would very much like to keep that trend...
There was a problem hiding this comment.
I looked into it, but it doesn’t seem super straightforward in this case. There are a few things that make it a bit tricky, e.g. RecordingStream wrapping PyRecordingStream
There was a problem hiding this comment.
Yes. This can only be done correctly once everything else is done correctly. By "correctly", I mean according to this issue:
oxkitsune
force-pushed
the
gijs/to-stream
branch
2 times, most recently
from
d4e6138 to
b727ab8
Compare
oxkitsune
force-pushed
the
gijs/to-stream
branch
from
b727ab8 to
ffa7c43
Compare
| @@ -488,6 +490,31 @@ def send_blueprint( | |||
| ) | |||
|
|
|||
|
|
|||
| def send_recording(embedded_recording: Recording, recording: RecordingStream | None = None) -> None: | |||
There was a problem hiding this comment.
This absolutely needs to also be a RecordingStream method.
Also, I don't know if this has been discussed, but our naming is all over the place, as this signature makes it explicit.
As they say, in for a penny, in for a pound. My actionable (if drastic) suggestion is either:
- either rename all
recording: RecordingStreamarguments of the stateful API tostream: RecordingStream, - or preferably remove those arguments altogether! (if you want to act on an existing recording stream, just use one of its method)
I'm strongly advocating for the latter option.
Related
Resolves #2044 somewhat
What
Adds api to send a recording loaded from RRD to a new recording stream, cloning the data in the process