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.

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

SDK DataLoaders 1: barebones Rust support #5327

Merged
merged 4 commits into from
Feb 29, 2024

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Feb 28, 2024

Exposes raw DataLoaders to the Rust SDK through 2 new methods: RecordingStream::log_file_from_path & RecordingStream::log_file_from_contents.

Everything streams asynchronously, as expected.

There's no way to configure the behavior of the DataLoader at all, yet. That comes next.

rec.log_file_from_path(filepath)?;

image

This highlighted some brittleness on the shutdown path, see:


Part of series of PR to expose configurable DataLoaders to our SDKs:

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 the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@teh-cmc teh-cmc added 🦀 Rust API Rust logging API include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages labels Feb 28, 2024
@teh-cmc teh-cmc force-pushed the cmc/sdk_dataloader_1_barebones_rust branch 2 times, most recently from ddbfc7f to 6439524 Compare February 28, 2024 11:26
@teh-cmc teh-cmc added the do-not-merge Do not merge this PR label Feb 28, 2024
@teh-cmc teh-cmc marked this pull request as draft February 28, 2024 11:30
Copy link

Size changes

Name main 5327/merge Change
re_sdk --no-default-features 117 crates 187 crates 70 crates

@teh-cmc
Copy link
Member Author

teh-cmc commented Feb 28, 2024

Size changes

Name main 5327/merge Change
re_sdk --no-default-features 117 crates 187 crates 70 crates

Guess we're gonna have to extract the DataLoader trait somewhere else then 😒

@teh-cmc
Copy link
Member Author

teh-cmc commented Feb 28, 2024

Size changes

Name main 5327/merge Change
re_sdk --no-default-features 117 crates 187 crates 70 crates

Guess we're gonna have to extract the DataLoader trait somewhere else then 😒

Eeeehhh I tried but it really gets awkward... I'll unfortunately defer to a feature flag for now.

@teh-cmc teh-cmc force-pushed the cmc/sdk_dataloader_1_barebones_rust branch from 6439524 to 48498ce Compare February 28, 2024 16:21
@teh-cmc teh-cmc marked this pull request as ready for review February 28, 2024 16:45
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Feb 29, 2024
@teh-cmc teh-cmc force-pushed the cmc/sdk_dataloader_1_barebones_rust branch from bcd19c1 to bcf4aa7 Compare February 29, 2024 08:40
@teh-cmc teh-cmc force-pushed the cmc/sdk_dataloader_1_barebones_rust branch from bcf4aa7 to 9e28fc9 Compare February 29, 2024 09:00
@Wumpf Wumpf self-requested a review February 29, 2024 10:18
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.

Looking great!
The only thing is that the example isn't really "practical" and looks almost more like a test. But @roym899 will fill that gap, so whatever :)

Comment on lines +54 to +56
// TODO(cmc): This is required because the way we handle RecordingStream clones in subtly
// broken. That's outside the scope of this PR, I'll fix this in a follow-up.
rec.disconnect();
Copy link
Member

Choose a reason for hiding this comment

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

do you think this might also be related to #5077 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't think so off the top of my head.

It's very related to #5335 and should have probably linked to it though 🙄

@teh-cmc teh-cmc merged commit ffe0a30 into main Feb 29, 2024
39 checks passed
@teh-cmc teh-cmc deleted the cmc/sdk_dataloader_1_barebones_rust branch February 29, 2024 12:02
teh-cmc added a commit that referenced this pull request Feb 29, 2024
Exposes raw `DataLoader`s to the Python SDK through 2 new methods:
`RecordingStream.log_file_from_path` &
`RecordingStream.log_file_from_contents`.

Everything streams asynchronously, as expected.

There's no way to configure the behavior of the `DataLoader` at all,
yet. That comes next.

```python
rr.log_file_from_path(filepath)
```

![image](https://github.com/rerun-io/rerun/assets/2910679/0fe2d39c-f069-44a6-b836-e31001b3adaa)


---

Part of series of PR to expose configurable `DataLoader`s to our SDKs:
- #5327 
- #5328 
- #5330
- #5337
- #5351
- #5355
teh-cmc added a commit that referenced this pull request Feb 29, 2024
Exposes raw `DataLoader`s to the C++ SDK through 2 new methods:
`RecordingStream::log_file_from_path` &
`RecordingStream::log_file_from_contents`.

Everything streams asynchronously, as expected.

There's no way to configure the behavior of the `DataLoader` at all,
yet. That comes next.

:warning: I'm carrying bytes around in a `string_view` (see `rr_bytes`)
and related, and the internet has been unhelpful to tell e whether I
should or shouldn't.

```cpp
rec.log_file_from_path(filepath);
```

![image](https://github.com/rerun-io/rerun/assets/2910679/1933f08b-1b2b-483b-9576-ce2e0421cb62)



---

Part of series of PR to expose configurable `DataLoader`s to our SDKs:
- #5327 
- #5328 
- #5330
- #5337
- #5351
- #5355

---------

Co-authored-by: Andreas Reich <andreas@rerun.io>
teh-cmc added a commit that referenced this pull request Feb 29, 2024
Introduces `RecordingStream::clone_weak`, so `DataLoader` threads
started from the SDK don't prevent the recording from flushing once
`main` goes out of scope, all the while making sure that `DataLoader`s
run to completion.


![image](https://github.com/rerun-io/rerun/assets/2910679/94bf7b16-cf9b-40ce-a190-d255328af3f8)


- Mitigates #5335 

---

Part of series of PR to expose configurable `DataLoader`s to our SDKs:
- #5327 
- #5328 
- #5330
- #5337
- #5351
- #5355
teh-cmc added a commit that referenced this pull request Feb 29, 2024
Adds new `RecommendedLoadSettings` that gets passed to all `DataLoader`s
-- builtin and external -- in order to customize their behaviors.

This includes:
- A recommended recording ID
- The ID of the currently opened recording in the viewer (not
implemented)
  - Related: #5350
- A recommended entity path prefix
- A recommended timepoint

```bash
cargo r -p rerun-loader-rust-file -- run_wasm/src/main.rs  --recording-id this-one --entity-path-prefix a/b/c  --time sim_time=1000 --time wall_time=1709204046 --sequence sim_frame=42 | rerun -
```

![image](https://github.com/rerun-io/rerun/assets/2910679/631d9798-c198-4e86-b6f8-32d0b849e7b2)


Checks:
- [x] external loader ran manually (`loader | rerun`)
- [x] external loader via rerun (`rerun xxx.rs`)
- [x] log_file with external loader (`log_file xxx.rs`)

---

Part of series of PR to expose configurable `DataLoader`s to our SDKs:
- #5327 
- #5328 
- #5330
- #5337
- #5351
- #5355
teh-cmc added a commit that referenced this pull request Mar 1, 2024
Introduces the new `DataLoaderSettings` business to Python and update
examples accordingly (`external_data_loader` & `log_file`).

```bash
python examples/python/external_data_loader/main.py --recording-id this-one --entity-path-prefix a/b/c  --time sim_time=1000 --time wall_time=1709204046 --sequence sim_frame=42 examples/python/dna/main.py | rerun -
```

![image](https://github.com/rerun-io/rerun/assets/2910679/bfda567d-3d16-42cd-be8e-8b1a0767a784)



Checks:
- [x] external loader ran manually (`python loader | rerun`)
- [x] external loader via rerun (`rerun xxx.py`)
- [x] log_file with external loader (`log_file xxx.py`)

---

Part of series of PR to expose configurable `DataLoader`s to our SDKs:
- #5327 
- #5328 
- #5330
- #5337
- #5351
- #5355
- #5361
teh-cmc added a commit that referenced this pull request Mar 4, 2024
This makes the `log_file` APIs behave more like the standard `log` APIs;
i.e. they inherit the state of their associated `RecordingStream`
(app_id, rec_id, timepoint, etc...).

Also inherit the application ID while we're at it.

Makes the API much nicer to use _and_ much more consistent with the
rest.

Checks:
- [x] external loader ran manually (`python loader | rerun`)
- [x] external loader via rerun (`rerun xxx.py`)
- [x] log_file with external loader (`log_file xxx.py`)
- [x] external loader ran manually (`loader | rerun`)
- [x] external loader via rerun (`rerun xxx.rs`)
- [x] log_file with external loader (`log_file xxx.rs`)

---

Part of series of PR to expose configurable `DataLoader`s to our SDKs:
- #5327 
- #5328 
- #5330
- #5337
- #5351
- #5355
- #5379
- #5361
- #5388

---------

Co-authored-by: Andreas Reich <andreas@rerun.io>
teh-cmc added a commit that referenced this pull request Mar 4, 2024
Introduces the new `DataLoaderSettings` business to C++ and update
examples accordingly (`external_data_loader` & `log_file`).

```bash
./build/debug/examples/cpp/log_file/example_log_file --recording-id this-one --entity-path-prefix a/b/c  --time sim_time=1000 --time wall_time=1709204046 --sequence sim_frame=42 rerun_cpp/tests/main.cpp | rerun -
```

![image](https://github.com/rerun-io/rerun/assets/2910679/b979a24c-29b6-473b-91b1-de3832bea436)


Checks:
- [x] external loader ran manually (`loader.exe | rerun`)
- [x] external loader via rerun (`rerun xxx.cpp`)
- [x] log_file with external loader (`log_file xxx.cpp`)

---

Part of series of PR to expose configurable `DataLoader`s to our SDKs:
- #5327 
- #5328 
- #5330
- #5337
- #5351
- #5355
- #5379
- #5361
- #5388
teh-cmc added a commit that referenced this pull request Mar 6, 2024
I guess that's good enough 🤷. I don't know, my brain has been
completely friend by C++ non-sense all day.

This includes a fix to make sure that a viewer that was spawned from the
python SDK is still allowed to spawn dataloaders implemented in python
(`RERUN_APP_ONLY` shenaniganeries).

- Fixes #4526 

---

Part of series of PR to expose configurable `DataLoader`s to our SDKs:
- #5327 
- #5328 
- #5330
- #5337
- #5351
- #5355
- #5379
- #5361
- #5388
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages 🦀 Rust API Rust logging API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants