Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Spawn via $PATH 2: Redesign clap integration and clean up all examples #3997

Merged
merged 7 commits into from
Oct 26, 2023
Merged

Spawn via $PATH 2: Redesign clap integration and clean up all examples #3997

merged 7 commits into from
Oct 26, 2023

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Oct 25, 2023 •
edited by github-actions bot
Loading

  • Get rid of the old thread-based spawn functionality.
  • Redesign RerunArgs to get rid of the awful callback while still dealing with Tokio's TLS shenanigans.
  • Update all tests and examples.

The new RerunArgs combined with the new spawn from PATH now make for a pretty nice experience:

let args = Args::parse();
let (rec, _serve_guard) = args.rerun.init("my_app")?;
// do stuff with rec

Spawn via $PATH series:

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 demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

This was referenced Oct 25, 2023
Merged
Merged
@teh-cmc teh-cmc force-pushed the cmc/binary_spawn_2_rerunargs branch from ec8de22 to b9dd5c8 Compare October 25, 2023 09:37
@teh-cmc teh-cmc added the 🧑‍💻 dev experience developer experience (excluding CI) label Oct 25, 2023
@teh-cmc teh-cmc marked this pull request as ready for review October 25, 2023 09:47
@teh-cmc teh-cmc added the do-not-merge Do not merge this PR label Oct 25, 2023
@teh-cmc teh-cmc force-pushed the cmc/binary_spawn_1_rust branch from 91f6edb to 72b1eca Compare October 25, 2023 14:58
@teh-cmc teh-cmc force-pushed the cmc/binary_spawn_2_rerunargs branch from b9dd5c8 to c9f9336 Compare October 25, 2023 15:09
@teh-cmc teh-cmc mentioned this pull request Oct 25, 2023
Merged
4 tasks
@Wumpf Wumpf self-requested a review October 26, 2023 09:27
Wumpf
Wumpf approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

this look veeeery nice and clean, also makes all the samples more lightweight by making them spawn instead of run.

But that return tuple bugs me a lot. Can we talk about alternatives later maybe?

crates/rerun/src/clap.rs
run(RecordingStream::disabled());
return Ok(());
}
pub fn init(&self, application_id: &str) -> anyhow::Result<(RecordingStream, ServeGuard)> {
Copy link
Member

Choose a reason for hiding this comment

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

The ergonomics of getting a tuple back where one part is super important and useful and the other an odd token I have to care around (but only when doing serve) are a bit odd. Even worse if I can init with serve from a scope now, it will not exit that scope iff I do serve (since the ServeGuard gets dropped). That's quite surprising behavior.
Suggestion for possible alternative: Given that the serve guard can't be killed, we might as well put it on a thread_local variable instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried a lot of things, including putting the guard in a thread local, but the maybe-owned tokio runtime just makes things weird no matter what...

We do want the user to be able to drop the guard, it's the only to shutdown all the web stuff that we've spawned in the background :/

examples/rust/dna/src/main.rs
Comment on lines 15 to +16
let rec = rerun::RecordingStreamBuilder::new("rerun_example_dna_abacus")
.connect(rerun::default_server_addr(), rerun::default_flush_timeout())?;
.spawn(rerun::default_flush_timeout())?;
Copy link
Member

Choose a reason for hiding this comment

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

also need to fix this up in rust.md (the tutorial)

Copy link
Member Author

Choose a reason for hiding this comment

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

They are so, so many places where we state the same things.. :(

Base automatically changed from cmc/binary_spawn_1_rust to main October 26, 2023 11:25
teh-cmc added a commit that referenced this pull request Oct 26, 2023
@teh-cmc
Spawn via $PATH 1: Rust implementation (#3996)
c7d26a1 
Introduces standalone `rerun::spawn(SpawnOptions)` API that allows one
to start a Rerun Viewer process ready to listen for TCP connections, as
well as the associated `RecordingStream` integration.



https://github.com/rerun-io/rerun/assets/2910679/24eb5647-38c1-4049-b249-dc6e00e4ff54



---

Spawn via `$PATH` series:
- #3996
- #3997
- #3998
@teh-cmc teh-cmc force-pushed the cmc/binary_spawn_2_rerunargs branch from c9f9336 to db012ec Compare October 26, 2023 11:32
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Oct 26, 2023
@teh-cmc teh-cmc merged commit c1a91d7 into main Oct 26, 2023
28 checks passed
@teh-cmc teh-cmc deleted the cmc/binary_spawn_2_rerunargs branch October 26, 2023 11:43
teh-cmc added a commit that referenced this pull request Oct 26, 2023
@teh-cmc
Spawn via $PATH 3: C and C++ implementations (#3998)
2f0fce7 
- Introduces standalone `rr_spawn(rr_spawn_options)` C API that allows
one to start a Rerun Viewer process ready to listen for TCP connections,
as well as the associated `rr_recording_stream` integration.
- Introduces standalone `rerun::spawn()` C++ API that allows one to
start a Rerun Viewer process ready to listen for TCP connections, as
well as the associated `RecordingStream` integration.



https://github.com/rerun-io/rerun/assets/2910679/8ae5003a-78b2-4e75-91d3-6dc4b8dd22ac



---

Spawn via `$PATH` series:
- #3996
- #3997
- #3998

---

- Fixes #3757 
- Fixes #3942
teh-cmc added a commit that referenced this pull request Oct 26, 2023
@teh-cmc
Spawn via $PATH 4: Updated docs & misc clean up (#4013)
55d25ac 
Last one.

Spawn via `$PATH` series:
- #3996
- #3997
- #3998
- #4013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wumpf Wumpf Wumpf approved these changes

Assignees
No one assigned
Labels
🧑‍💻 dev experience developer experience (excluding CI) include in changelog 🦀 Rust API Rust logging API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rust SDK: remove old threaded spawn in favor of fork-exec + binary artifacts
2 participants
@teh-cmc @Wumpf