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

New helper for sticking Serde-encodable data into arrow #2004

Merged
merged 7 commits into from
May 4, 2023

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Apr 29, 2023

What

We're about to add a bunch more arrow components for managing blueprints.

Long term it would be nice for this to all be arrow schemas, but all of our state can already be represented with Serde, and we can move faster by just shimming that data through the store.

This gives us a nice escape hatch by adding an arrow_field helper that lets us add any serde-compatible data as a field of a component.

Exa:

use re_log_types::serde_field::SerdeField;
use arrow2_convert::{ArrowDeserialize, ArrowField, ArrowSerialize, field::ArrowField};
use arrow2::datatypes::{DataType, Field};

#[derive(Clone, Debug, PartialEq, serde::Deserialize, serde::Serialize)]
struct SomeStruct {
    foo: String,
    bar: u32,
}

#[derive(Clone, Debug, PartialEq, ArrowField, ArrowSerialize, ArrowDeserialize)]
struct SomeArrowStruct {
    #[arrow_field(type = "SerdeField<SomeStruct>")]
    field: SomeStruct,
}

Checklist

PR Build Summary: https://build.rerun.io/pr/2004

@jleibs jleibs added 🏹 arrow concerning arrow 🧑‍💻 dev experience developer experience (excluding CI) labels Apr 29, 2023
@jleibs jleibs marked this pull request as ready for review April 29, 2023 03:18
@jleibs jleibs mentioned this pull request Apr 30, 2023
6 tasks
/// ])
/// );
/// ```
pub struct SerdeField<T>(std::marker::PhantomData<T>);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder how we should handle the fact that we use MsgPack as the "codec". There are alternatives, like bincode, we could be using, and might want to switch too in the future.

We could, for instance, add an enum Codec { MsgPack = 1 } and put that as the first byte of the encoded array.

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 think the "right" thing to do in theory is to add this to the arrow meta-data for the field. We've been wanting to use that for some other stuff as well so this might be a good opportunity to investigate the plumbing.

Comment on lines 21 to 32
/// #[derive(Clone, Debug, PartialEq, ArrowField, ArrowSerialize, ArrowDeserialize)]
/// struct SomeArrowStruct {
/// #[arrow_field(type = "SerdeField<SomeStruct>")]
/// field: SomeStruct,
/// }
///
/// assert_eq!(
/// SomeArrowStruct::data_type(),
/// DataType::Struct(vec![
/// Field::new("field", DataType::Binary, false),
/// ])
/// );
Copy link
Member

Choose a reason for hiding this comment

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

The extra struct layer will add a lot of unnecessary noise all over; and since people are most likely going to copy-paste this as a starting point, I'd rather show them how to avoid that straight from the start:

Suggested change
/// #[derive(Clone, Debug, PartialEq, ArrowField, ArrowSerialize, ArrowDeserialize)]
/// struct SomeArrowStruct {
/// #[arrow_field(type = "SerdeField<SomeStruct>")]
/// field: SomeStruct,
/// }
///
/// assert_eq!(
/// SomeArrowStruct::data_type(),
/// DataType::Struct(vec![
/// Field::new("field", DataType::Binary, false),
/// ])
/// );
/// #[derive(Clone, Debug, PartialEq, ArrowField, ArrowSerialize, ArrowDeserialize)]
/// #[arrow_field(transparent)]
/// struct SomeStructArrow(#[arrow_field(type = "SerdeField<SomeStruct>")] SomeStruct);
///
/// assert_eq!(SomeStructArrow::data_type(), DataType::Binary);

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I fully agree. Demoing that this can be done on just a sub-field is also helpful. But onboard with making the docstring shorter.

@emilk
Copy link
Member

emilk commented May 2, 2023

We should add crate::profile_function!() probes to the serialization and deserialization

@jleibs jleibs merged commit 14cdeb9 into main May 4, 2023
@jleibs jleibs deleted the jleibs/serde_field branch May 4, 2023 14:24
jleibs added a commit that referenced this pull request May 19, 2023
 - Uses the new serde-helper from: #2004 to stick space-views in the data-store
 - Refactors the frame loop to follow this basic logic:
   - Materialize the blueprint from the store
   - Save a snapshot of the materialized blueprint
   - Run most of the legacy code as is
   - If the blueprint has been modified then save the modifications back to the store
 - In the internals of the Python, this introduces a new `global_blueprint_stream()`
 - Adds a few python APIs that send data to the stream.
 - RecordingId is now a string, and we use a special condition of RecordingId == AppId to determine that a blueprint is the "default blueprint" for an app.
 - The default behavior of rr.init() now uses this special recording-id, which means in the common case your blueprint API calls do almost exactly what you want, but an expert can still opt out using `add_to_app_default_blueprint=False` (this might need a better name), in which case they get complete control over a new blueprint.
 - SpaceViewIds generated by the app use a hash of the spaceview name to avoid repeated appends.
 - The "selected blueprint" is determined based on app-id. There is a "currently selected" blueprint for each app, which defaults to the special global blueprint.
emilk pushed a commit that referenced this pull request May 22, 2023
 - Uses the new serde-helper from: #2004 to stick space-views in the data-store
 - Refactors the frame loop to follow this basic logic:
   - Materialize the blueprint from the store
   - Save a snapshot of the materialized blueprint
   - Run most of the legacy code as is
   - If the blueprint has been modified then save the modifications back to the store
 - In the internals of the Python, this introduces a new `global_blueprint_stream()`
 - Adds a few python APIs that send data to the stream.
 - RecordingId is now a string, and we use a special condition of RecordingId == AppId to determine that a blueprint is the "default blueprint" for an app.
 - The default behavior of rr.init() now uses this special recording-id, which means in the common case your blueprint API calls do almost exactly what you want, but an expert can still opt out using `add_to_app_default_blueprint=False` (this might need a better name), in which case they get complete control over a new blueprint.
 - SpaceViewIds generated by the app use a hash of the spaceview name to avoid repeated appends.
 - The "selected blueprint" is determined based on app-id. There is a "currently selected" blueprint for each app, which defaults to the special global blueprint.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏹 arrow concerning arrow 🧑‍💻 dev experience developer experience (excluding CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants