Skip to content

rust data loaders #460

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

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

nthiad
Copy link

@nthiad nthiad commented Jun 3, 2025

Changelog

Adds a foxglove_data_loader crate for writing foxglove data loader extensions in rust that compile to wasm.

Docs

FG-11831

Description

This PR adds a foxglove_data_loader crate with DataLoader and MessageIterator traits for user code to implement plus a foxglove_data_loader::export!() macro which exports user data loader implementation to the wasm component build.

The wasm output of builds made with this interface can be passed as wasmUrl to extensionContext.registerDataLoader().

Usage looks like:

use foxglove_data_loader::{
    reader, console, BackfillArgs, Channel, DataLoader, DataLoaderArgs, Initialization,
    Message, MessageIterator, MessageIteratorArgs, Schema, TimeRange,
};
foxglove_data_loader::export!(MyLoader);

struct MyLoader { /* ... */ }

impl DataLoader for MyLoader {
    type MessageIterator = MyIterator;
    type Error = anyhow::Error;

    fn new(args: DataLoaderArgs) -> Self {
        // args.paths is a Vec<String> of file paths you can reader::open()
        unimplemented![]
    }

    fn initialize(&self) -> Result<Initialization, Self::Error> {
        // the Initialization contains the channels, schemas, time range, and any problems
        unimplemented![]
    }

    fn create_iter(&self, args: MessageIteratorArgs) -> Result<Self::MessageIterator, Self::Error> {
        unimplemented![]
    }

    fn get_backfill(&self, args: BackfillArgs) -> Result<Vec<Message>, Self::Error> {
        unimplemented![]
    }
}

struct MyIterator { /* ... */ }

impl MessageIterator for MyIterator {
    type Error = anyhow::Error;

    fn next(&self) -> Option<Result<Message, Self::Error>> {
        unimplemented![]
    }
}

There is a data_loader::reader::open(&path) function that returns a Reader from the internal WIT. The inputs vec contains a list of path strings you can reader::open(). std::io::Read and std::io::Seek are implemented for the WIT Reader to make it easier to use with the rest of the rust ecosystem. For example, you can do things like:

let reader = foxglove_data_loader::reader::open(&inputs[0]);
let mut lines = BufReader::new(reader).lines();

Depends on https://github.com/foxglove/app/pull/9659

Fixes: FG-11694

Copy link

linear bot commented Jun 3, 2025

@defunctzombie
Copy link
Contributor

Once I have this in a source file - what do I do to build it?

@defunctzombie
Copy link
Contributor

The ::foxglove is to disambiguate an internally generated foxglove module derived from the WIT package name from the sdk itself. There might be a work-around for this.

My editor keeps removing the leading :: so yea would be nice to not have this leak into the user's code

@nthiad
Copy link
Author

nthiad commented Jun 6, 2025

Once I have this in a source file - what do I do to build it?

cargo add --path ~/dev/foxglove/sdk --no-default-features --features data_loader,derive
cargo build --release --target wasm32-unknown-unknown

My editor keeps removing the leading :: so yea would be nice to not have this leak into the user's code

I agree. The issue is that wit-bindgen defines the foxglove from the WIT line package foxglove:[email protected] in the current scope so I will need to do more hacks or patch upstream to get around this.

@nthiad
Copy link
Author

nthiad commented Jun 6, 2025

My editor keeps removing the leading :: so yea would be nice to not have this leak into the user's code

The ::foxglove disambiguation is no longer an issue on the latest commit.

@nthiad
Copy link
Author

nthiad commented Jun 7, 2025

I hid all the Guest traits and made DataLoader and MessageIterator internal details. Now you write your loader and iterator with impl and so long as it has the correct methods with the correct signatures, things will work.

@nthiad nthiad changed the title rust macro for data loaders rust data loaders Jun 7, 2025
@nthiad
Copy link
Author

nthiad commented Jun 7, 2025

If we have an example, we should include an example of doing mutable state with RefCell because the &self makes it look like you can't have mutable state, but that is not true, it just requires a bit more work. The &self refs we get from WIT directly and probably should keep that since there is a perf penalty for using mutability containers like RefCell or Mutex or RwLock.

@defunctzombie
Copy link
Contributor

Gave this a spin writing a basic mp4 data loader using this branch paired with https://github.com/foxglove/app/pull/9659. I've ticketed my feedback here:

Generally my vibe is this is a great pairing with the app-side experimental PR and something we can get folks tire kicking with.

As for what might block this PR merging I have the following thoughts:

  • In the app side we clearly mark these apis as experimental, is there some equivalent thing we can do here? We use the data_loader feature so maybe that's sufficient or is there some other idiomatic way to indicate this isn't a stable feature?
  • It would be nice to have some comments on the trail members. Stuff like fn time_range(&self) -> Result<loader::TimeRange, String>; what should I return?
  • /// And don't forget to call foxglove::data_loader_export() on your loader. nit: generally not a fan of "don't forget" language. Tell the user what to do.
  • nit: drop the "custom" everywhere it exists - what's custom about them?

@nthiad
Copy link
Author

nthiad commented Jun 9, 2025

In the app side we clearly mark these apis as experimental, is there some equivalent thing we can do here? We use the data_loader feature so maybe that's sufficient or is there some other idiomatic way to indicate this isn't a stable feature?

I haven't seen this as much in crates, mostly in rust itself, but here is one example:
https://github.com/async-rs/async-std/blob/v1.8.0/Cargo.toml#L38-L42

It's more complicated in our case because we are using features to gate against wasm-specific functionality already to to also gate for stability we could rename the feature data_loaders_unstable or data_loaders_experimental. Or another option could be to keep the name data_loaders but gate that behind something like wasm_unstable. I don't have a good handle of what naming scheme would be more common for this sort of thing.

@nthiad nthiad marked this pull request as ready for review June 14, 2025 01:06
@amacneil
Copy link
Contributor

In the app side we clearly mark these apis as experimental, is there some equivalent thing we can do here? We use the data_loader feature so maybe that's sufficient or is there some other idiomatic way to indicate this isn't a stable feature?

Since this is a separate crate, one way we can do this is just not publish the crate. Add a readme saying its experimental and must be imported as a git crate.

@nthiad
Copy link
Author

nthiad commented Jun 18, 2025

I've removed the ndjson example from this PR in favor of the location in create-foxglove-extension with my other PR so that we only have one copy of this example and so that they don't get out of sync. I'll add a follow-up PR to this crate with a different example and I'll apply all the example feedback here to the ndjson rust example in create-foxglove-extension.

@nthiad nthiad requested a review from achim-k June 18, 2025 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants