-
Notifications
You must be signed in to change notification settings - Fork 373
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
Introduce a new BinaryStreamSink that allows reading a stream of encoded bytes #6242
Conversation
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.
Neat utility, seems pretty straight forward
wondering if we should expose this new type of stream to C++ as well, but I reckon we don't have a usecase for it it, so better not to
Requesting changes because I'd love to see the test being automated - if this turns out to be too much work all good from my side
impl Command { | ||
fn flush() -> (Self, Receiver<()>) { | ||
let (tx, rx) = std::sync::mpsc::sync_channel(0); // oneshot | ||
(Self::Flush(tx), rx) | ||
} | ||
} |
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.
nit: I found 3 implementations of this, two of them using crossbeam one using mpsc like you here. I like that you bring balance to the world ;)
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.
Good enough for FileSink, good enough for me 🤷
let storage = BinaryStreamStorage::new(rec); | ||
|
||
// We always compress when writing to a stream | ||
// TODO(jleibs): Make this configurable |
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 harm in just exposing that as a flag on creation right away?
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.
Mostly that (1) it also applies to every other sink type and (2) I can't come up with a good reason that a user would ever set it to False.
I'd rather introduce them all together in a consistent way if and when we have a compelling use-case.
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.
nice work! super cool to have this running on ci
What
This effectively combines the guts of FileSink and MemorySink. Like the FileSink, this runs file-encoding on its own thread so that all we need to do when we're ready to read is move out the bytes.
The downside relative to memory_sink is the stream is a singular RRD. You have no way of accessing information about individual messages, draining from the backlog, etc.
This is designed to be as easy as possible to wire into a gradio output stream.
Example usage in a Gradio Component:
Checklist
main
build: rerun.io/viewernightly
build: rerun.io/viewerTo run all checks from
main
, comment on the PR with@rerun-bot full-check
.