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

Keep track of the RRD protocol version and display it where relevant #6324

Merged
merged 12 commits into from
May 14, 2024

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented May 14, 2024

This augments StoreInfo with an in-memory only (!) store_version field.
Our Decoder and StreamDecoder have been patched to fill out this field using the RRD version present in the stream's header.

This version is now visible in the UI, using the CLI, and in the analytics.

Viewer:
image

CLI:
image

Analytics:
image

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!

To run all checks from main, comment on the PR with @rerun-bot full-check.

@teh-cmc teh-cmc added enhancement New feature or request ⛃ re_datastore affects the datastore itself 📺 re_viewer affects re_viewer itself 📊 analytics telemetry analytics include in changelog labels May 14, 2024
Comment on lines +199 to +209
let StoreInfo {
application_id,
recording_id,
store_source,
store_version,
rust_version,
llvm_version,
python_version,
is_official_example,
app_id_starts_with_rerun_example,
} = store_info;
Copy link
Member Author

Choose a reason for hiding this comment

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

Got bitten by the lack of de-structuring here

@teh-cmc
Copy link
Member Author

teh-cmc commented May 14, 2024

@rerun-bot full-check

Copy link

crates/re_data_ui/src/entity_db.rs Outdated Show resolved Hide resolved
@@ -361,6 +363,13 @@ pub struct StoreInfo {
pub started: Time,

pub store_source: StoreSource,

/// The Rerun version used to encoded the RRD data.
Copy link
Member

Choose a reason for hiding this comment

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

I think we might need to be more specific here by what we mean when we say "encoding".

There is both the outer-encoding of the RRD framing, and the inner-encoding of the arrow schemas.

As implemented this is the Rerun version used to TRANSMIT the encoded data.

If you for example:

  • Create an RRD with 0.15
  • Load the RRD into an 0.16 viewer
  • Re-save the RRD trough the viewer

You will end up with 0.16 in the stream, that still has 0.15-encoded payloads.

I actually think we would be better off tracking this explicitly all the way through so that this version can be used as a concise proxy for the encoded arrow schemas as well.

Copy link
Member Author

@teh-cmc teh-cmc May 14, 2024

Choose a reason for hiding this comment

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

Regarding the two layers of framing, I think we're better off considering they are always one and the same until we really push for stabilization of our protocol. It keeps things simple.

The fact that re-saving the data overrides the original version seems like a bug, it should re-use the original version. I should fix that.

(Also I'm realizing that it is possible to indefinitely append data to an existing recording using a different version of the SDK each time, is not? In which case all of this really is best effort anyhow, unless we want to keep track of a set of versions?!)

Copy link
Member

Choose a reason for hiding this comment

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

I think we're better off considering they are always one and the same until we really push for stabilization of our protocol. It keeps things simple.

Awesome. This makes sense to me.

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 made it so that the Encoder now requires a CrateVersion to be explicitly stated.

When saving from the viewer, that version is now picked up from the existing StoreInfo in the EntityDb (if any).

Seems to work fine for the things I've tested, though I'm sure there are crazy edge cases lurking somewhere... Rerun versioning is hard 🤪

crates/re_data_loader/src/load_file.rs Outdated Show resolved Hide resolved
@teh-cmc teh-cmc added the do-not-merge Do not merge this PR label May 14, 2024
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label May 14, 2024
@teh-cmc teh-cmc requested a review from jleibs May 14, 2024 17:39
@rerun-io rerun-io deleted a comment from github-actions bot May 14, 2024
@teh-cmc
Copy link
Member Author

teh-cmc commented May 14, 2024

@rerun-bot full-check

Copy link

Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

Nice

@teh-cmc teh-cmc merged commit c19a08f into main May 14, 2024
66 of 67 checks passed
@teh-cmc teh-cmc deleted the cmc/what_version_is_it_tell_me_bro branch May 14, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📊 analytics telemetry analytics enhancement New feature or request include in changelog ⛃ re_datastore affects the datastore itself 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Surface SDK version info from channel into StoreInfo
3 participants